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.
Either option looks good to me.
Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
Thanks for doing that!
Liviu
>
> 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
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
