Hi Am 24.06.21 um 14:36 schrieb Jani Nikula:
On Thu, 24 Jun 2021, Thierry Reding <thierry.reding@xxxxxxxxx> 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 <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.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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
