thr3ads.net - Nouveau - [Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls [Jun 2021]

If this information is useful, please help other people find it:
Share via:

Jani Nikula

2021-Jun-24 08:51 UTC

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at suse.de>
wrote:> Hi
>
> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at suse.de>
wrote:
>>> diff --git a/drivers/gpu/drm/drm_vblank.c
b/drivers/gpu/drm/drm_vblank.c
>>> index 3417e1ac7918..10fe16bafcb6 100644
>>> --- a/drivers/gpu/drm/drm_vblank.c
>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device
*dev, void *data,
>>>   	unsigned int pipe_index;
>>>   	unsigned int flags, pipe, high_pipe;
>>>   
>>> -	if (!dev->irq_enabled)
>>> -		return -EOPNOTSUPP;
>>> +#if defined(CONFIG_DRM_LEGACY)
>>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>>> +		if (!dev->irq_enabled)
>>> +			return -EOPNOTSUPP;
>>> +	} else /* if DRIVER_MODESET */
>>> +#endif
>>> +	{
>>> +		if (!drm_dev_has_vblank(dev))
>>> +			return -EOPNOTSUPP;
>>> +	}
>> 
>> Sheesh I hate this kind of inline #ifdefs.
>> 
>> Two alternate suggestions that I believe should be as just efficient:
>
> Or how about:
>
> static bool drm_wait_vblank_supported(struct drm_device *dev)
>
> {
>
> if defined(CONFIG_DRM_LEGACY)
> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>
> 		return dev->irq_enabled;
>
> #endif
> 	return drm_dev_has_vblank(dev);
>
> }
>
>
> ?
>
> It's inline, but still readable.
It's definitely better than the original, but it's unclear to me why
you'd prefer this over option 2) below. I guess the only reason I can
think of is emphasizing the conditional compilation. However,
IS_ENABLED() is widely used in this manner specifically to avoid inline
#if, and the compiler optimizes it away.

BR,
Jani.

>
> Best regards
> Thomas
>
>> 
>> 1) The more verbose:
>> 
>> #if defined(CONFIG_DRM_LEGACY)
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> 		return dev->irq_enabled;
>> 	else
>> 		return drm_dev_has_vblank(dev);
>> }
>> #else
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	return drm_dev_has_vblank(dev);
>> }
>> #endif
>> 
>> 2) The more compact:
>> 
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>> {
>> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) &&
unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>> 		return dev->irq_enabled;
>> 	else
>> 		return drm_dev_has_vblank(dev);
>> }
>> 
>> Then, in drm_wait_vblank_ioctl().
>> 
>> 	if (!drm_wait_vblank_supported(dev))
>> 		return -EOPNOTSUPP;
>> 
>> The compiler should do the right thing without any explicit inline
>> keywords etc.
>> 
>> BR,
>> Jani.
>> 
>>>   
>>>   	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>>   		return -EINVAL;
>>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct
drm_device *dev, void *data,
>>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>   		return -EOPNOTSUPP;
>>>   
>>> -	if (!dev->irq_enabled)
>>> +	if (!drm_dev_has_vblank(dev))
>>>   		return -EOPNOTSUPP;
>>>   
>>>   	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct
drm_device *dev, void *data,
>>>   	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>   		return -EOPNOTSUPP;
>>>   
>>> -	if (!dev->irq_enabled)
>>> +	if (!drm_dev_has_vblank(dev))
>>>   		return -EOPNOTSUPP;
>>>   
>>>   	crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
>> 
-- 
Jani Nikula, Intel Open Source Graphics Center

Thomas Zimmermann

2021-Jun-24 09:07 UTC

head link

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

Hi

Am 24.06.21 um 10:51 schrieb Jani Nikula:> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at suse.de>
wrote:
>> Hi
>>
>> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at
suse.de> wrote:
>>>> diff --git a/drivers/gpu/drm/drm_vblank.c
b/drivers/gpu/drm/drm_vblank.c
>>>> index 3417e1ac7918..10fe16bafcb6 100644
>>>> --- a/drivers/gpu/drm/drm_vblank.c
>>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>>> @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct
drm_device *dev, void *data,
>>>>    	unsigned int pipe_index;
>>>>    	unsigned int flags, pipe, high_pipe;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> -		return -EOPNOTSUPP;
>>>> +#if defined(CONFIG_DRM_LEGACY)
>>>> +	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
>>>> +		if (!dev->irq_enabled)
>>>> +			return -EOPNOTSUPP;
>>>> +	} else /* if DRIVER_MODESET */
>>>> +#endif
>>>> +	{
>>>> +		if (!drm_dev_has_vblank(dev))
>>>> +			return -EOPNOTSUPP;
>>>> +	}
>>>
>>> Sheesh I hate this kind of inline #ifdefs.
>>>
>>> Two alternate suggestions that I believe should be as just
efficient:
>>
>> Or how about:
>>
>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>
>> {
>>
>> if defined(CONFIG_DRM_LEGACY)
>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>
>> 		return dev->irq_enabled;
>>
>> #endif
>> 	return drm_dev_has_vblank(dev);
>>
>> }
>>
>>
>> ?
>>
>> It's inline, but still readable.
> 
> It's definitely better than the original, but it's unclear to me
why
> you'd prefer this over option 2) below. I guess the only reason I can
> think of is emphasizing the conditional compilation. However,
> IS_ENABLED() is widely used in this manner specifically to avoid inline
> #if, and the compiler optimizes it away.
It's simply more readable to me as the condition is simpler. But option 
2 is also ok.

Best regards
Thomas
> 
> BR,
> Jani.
> 
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> 1) The more verbose:
>>>
>>> #if defined(CONFIG_DRM_LEGACY)
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>> 		return dev->irq_enabled;
>>> 	else
>>> 		return drm_dev_has_vblank(dev);
>>> }
>>> #else
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	return drm_dev_has_vblank(dev);
>>> }
>>> #endif
>>>
>>> 2) The more compact:
>>>
>>> static bool drm_wait_vblank_supported(struct drm_device *dev)
>>> {
>>> 	if  (IS_ENABLED(CONFIG_DRM_LEGACY) &&
unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
>>> 		return dev->irq_enabled;
>>> 	else
>>> 		return drm_dev_has_vblank(dev);
>>> }
>>>
>>> Then, in drm_wait_vblank_ioctl().
>>>
>>> 	if (!drm_wait_vblank_supported(dev))
>>> 		return -EOPNOTSUPP;
>>>
>>> The compiler should do the right thing without any explicit inline
>>> keywords etc.
>>>
>>> BR,
>>> Jani.
>>>
>>>>    
>>>>    	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
>>>>    		return -EINVAL;
>>>> @@ -2023,7 +2031,7 @@ int drm_crtc_get_sequence_ioctl(struct
drm_device *dev, void *data,
>>>>    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> +	if (!drm_dev_has_vblank(dev))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>>    	crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
>>>> @@ -2082,7 +2090,7 @@ int drm_crtc_queue_sequence_ioctl(struct
drm_device *dev, void *data,
>>>>    	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>> -	if (!dev->irq_enabled)
>>>> +	if (!drm_dev_has_vblank(dev))
>>>>    		return -EOPNOTSUPP;
>>>>    
>>>>    	crtc = drm_crtc_find(dev, file_priv,
queue_seq->crtc_id);
>>>
> 
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 N?rnberg, Germany
(HRB 36809, AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20210624/a788a14e/attachment-0001.sig>

Thierry Reding

2021-Jun-24 12:03 UTC

head link

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann
wrote:> Hi
> 
> Am 24.06.21 um 10:51 schrieb Jani Nikula:
> > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at suse.de>
wrote:
> > > Hi
> > > 
> > > Am 24.06.21 um 10:06 schrieb Jani Nikula:
> > > > On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann at
suse.de> wrote:
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c
b/drivers/gpu/drm/drm_vblank.c
> > > > > index 3417e1ac7918..10fe16bafcb6 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct
drm_device *dev, void *data,
> > > > >    	unsigned int pipe_index;
> > > > >    	unsigned int flags, pipe, high_pipe;
> > > > > -	if (!dev->irq_enabled)
> > > > > -		return -EOPNOTSUPP;
> > > > > +#if defined(CONFIG_DRM_LEGACY)
> > > > > +	if  (unlikely(drm_core_check_feature(dev,
DRIVER_LEGACY))) {
> > > > > +		if (!dev->irq_enabled)
> > > > > +			return -EOPNOTSUPP;
> > > > > +	} else /* if DRIVER_MODESET */
> > > > > +#endif
> > > > > +	{
> > > > > +		if (!drm_dev_has_vblank(dev))
> > > > > +			return -EOPNOTSUPP;
> > > > > +	}
> > > > 
> > > > Sheesh I hate this kind of inline #ifdefs.
> > > > 
> > > > Two alternate suggestions that I believe should be as just
efficient:
> > > 
> > > Or how about:
> > > 
> > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > > 
> > > {
> > > 
> > > if defined(CONFIG_DRM_LEGACY)
> > > 	if  (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > > 
> > > 		return dev->irq_enabled;
> > > 
> > > #endif
> > > 	return drm_dev_has_vblank(dev);
> > > 
> > > }
> > > 
> > > 
> > > ?
> > > 
> > > It's inline, but still readable.
> > 
> > It's definitely better than the original, but it's unclear to
me why
> > you'd prefer this over option 2) below. I guess the only reason I
can
> > think of is emphasizing the conditional compilation. However,
> > IS_ENABLED() is widely used in this manner specifically to avoid
inline
> > #if, and the compiler optimizes it away.
> 
> It's simply more readable to me as the condition is simpler. But option
2 is
> also ok.
Perhaps do something like this, then:

	if (IS_ENABLED(CONFIG_DRM_LEGACY)) {
		if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
			return dev->irq_enabled;
	}

	return drm_dev_has_vblank(dev);

That's about just as readable as the variant involving the preprocessor
but has all the benefits of not using the preprocessor.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20210624/07d50b4b/attachment-0001.sig>

Nouveau - Jun 2021 - [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls