Hi Am 24.06.21 um 14:36 schrieb Jani Nikula: > On Thu, 24 Jun 2021, Thierry Reding wrote: >> 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 wrote: >>>>> Hi >>>>> >>>>> Am 24.06.21 um 10:06 schrieb Jani Nikula: >>>>>> On Thu, 24 Jun 2021, Thomas Zimmermann 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. > > Looks like a winner to me. :) That's the most readable. But I just remembered that irq_enabled will likely become legacy-only in the device structure. We'll need an ifdef variant then. :/ Best regards Thomas > > BR, > Jani. > > -- 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