Re: [Intel-gfx] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

Thomas Zimmermann
2021-06-24T09:08:02Z

[-- Attachment #1.1.1: Type: text/plain, Size: 4052 bytes --]

Hi

Am 24.06.21 um 10:51 schrieb Jani Nikula:
> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 24.06.21 um 10:06 schrieb Jani Nikula:
>>> On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmermann@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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]