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. :)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
