Thierry Reding
2021-Jun-24 12:03 UTC
[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>
Jani Nikula
2021-Jun-24 12:36 UTC
[Nouveau] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls
On Thu, 24 Jun 2021, Thierry Reding <thierry.reding at gmail.com> 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 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.Looks like a winner to me. :) BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center