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