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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux