Re: [Intel-gfx] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls

[Thomas Zimmermann]

Hi

Am 24.06.21 um 10:51 schrieb Jani Nikula:

On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmerm...@suse.de> wrote:

Hi

Am 24.06.21 um 10:06 schrieb Jani Nikula:

On Thu, 24 Jun 2021, Thomas Zimmermann <tzimmerm...@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;

-               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;
+       }

- if (!dev->irq_enabled)

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.

Best regards
Thomas

It's simply more readable to me as the condition is simpler. But option 2 is also ok.

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 (!drm_dev_has_vblank(dev))
                return -EOPNOTSUPP;

@@ -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 (!drm_dev_has_vblank(dev))
                return -EOPNOTSUPP;

- if (!dev->irq_enabled) crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id); - if (!dev->irq_enabled) 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

OpenPGP_signature
Description: OpenPGP digital signature

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx