[PATCH 0/5] drm/vblank: Deferred Enable and Disable

[PATCH 0/5] drm/vblank: Deferred Enable and Disable

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

# Abstract

The purpose of this series is to introduce a way for drivers to do blocking work
as part of their vblank enable/disable callbacks. Today, this isn't possible
since drm_crtc_funcs->(enable|disable)_vblank() can be called from atomic
context. The proposed solution is to defer drm_vblank_enable() and
vblank_disable_fn() to a workqueue from within the DRM vblank core, along with
introducing new crtc callbacks where drivers can do blocking work.

A drm-misc-next based branch with this series applied is available here:
https://gitlab.freedesktop.org/leoli/drm-misc/-/commits/drm-misc-next

# Considerations

Why not defer within the driver callbacks? It can introduce concurrency between
the deferred work, and access to HW upon the callback's return. See
drm_vblank_enable()'s call to drm_update_vblank_count() that reads the HW vblank
counter and scanout position, for example. If the underlying HW remains
accessible when vblanks are disabled, then this wouldn't be an issue. But that's
not always the case (amdgpu_dm being an example, see patch 4/5's commit
description). One may be tempted to spin-wait on the driver's enable worker to
complete from the callbacks that access HW, (*)but waiting on a deferred
process-context worker while possibly in atomic context is not ideal.

Doesn't deferring from the DRM vblank core have the same issue? Yes and no (and
here is where I think some additional review is required):

Since the entirety of drm_vblank_enable/disable_and_save() is deferred, access
to HW counter and scanout position from within is synchronized with the
enable/disable callbacks. However, it is still possible for the caller of
drm_vblank_get() to run concurrently with the deferred enable worker that it
scheduled. This is not an issue with vblank_put(), since HW is already in an
enabled state, and it's OK for HW to disabled a little later.

In cases where the vblank_getter wants to ensure that vblank counts will
increment (e.g. for waiting on a specific vblank), this shouldn't be an issue:
HW vblanks will be enabled eventually, and the counter will progress (albeit
with some additional delay). This seems the case for all vblank_get() calls from
within DRM core, with one exception addressed in patch 1/5.

But if the vblank_getter requires HW to be enabled upon return (e.g. programming
something that depends on HW being enabled), a new
drm_crtc_vblank_wait_deferred_enable() helper is provided. Drivers can use it to
wait for the enable work to complete before proceeding. This doesn't have the
same concern as (*) above, since wait_deferred_enable() must run from process
context. And if the driver needs deferred enable/disable work, it sounds
reasonable to ask it to also do such work from process context.

# Some more context

This is actually a redo of a previous attempt to introduce a "vblank_prepare()"
driver callback that the DRM core calls prior to entering atomic context. It was
dropped because it has synchronization issues -- there was nothing preventing a
previous vblank_put() from "undoing" the prepare work while there is a pending
vblank_get() (IOW vblank_prepare() and vblank_disable() runs concurrently)(*).
It also asks all callers of vblank_get() to be vblank_prepare()-aware, which
isn't great. This series can be found here:

https://lore.kernel.org/dri-devel/20260127194143.176248-1-sunpeng.li@amd.com/

(*) With the pre/post_enable/disable() callbacks for deferred vblank
    enable/disable, drivers can acquire/release their own locks, as long as
    they're properly paired.

Leo Li (5):
  drm/vblank: Add drm_crtc_vblank_is_off() helper
  drm/vblank: Introduce deferred vblank enable/disable
  drm/amd/display: Refactor amdgpu_dm_crtc_set_vblank
  drm/amd/display: Implement deferred vblanks on IPS platforms
  drm/vblank: Add some debugging trace events

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   8 +
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 263 ++++++++++++++++--
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.h    |   4 -
 drivers/gpu/drm/drm_atomic_helper.c           |  11 +-
 drivers/gpu/drm/drm_drv.c                     |   5 +
 drivers/gpu/drm/drm_internal.h                |   1 +
 drivers/gpu/drm/drm_trace.h                   | 112 ++++++++
 drivers/gpu/drm/drm_vblank.c                  | 227 ++++++++++++++-
 include/drm/drm_crtc.h                        |  34 +++
 include/drm/drm_device.h                      |   6 +
 include/drm/drm_vblank.h                      |  20 ++
 11 files changed, 642 insertions(+), 49 deletions(-)

-- 
2.52.0

[PATCH 1/5] drm/vblank: Add drm_crtc_vblank_is_off() helper

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

Add and use an drm internal helper to check if vblanks have been turned
off (via drm_crtc_vblank_off()) for a CRTC, rather than relying on the
return value of drm_crtc_vblank_get().

This is in preparation of introducing deferred vblank enable/disable, as
vblank_get() will not return the expected error code when the driver
defers vblank enable.

No functional change is intended.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++------
 drivers/gpu/drm/drm_internal.h      |  1 +
 drivers/gpu/drm/drm_vblank.c        | 17 +++++++++++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 26953ed6b53e8..102937e3ff35b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -48,6 +48,7 @@
 
 #include "drm_crtc_helper_internal.h"
 #include "drm_crtc_internal.h"
+#include "drm_internal.h"
 
 /**
  * DOC: overview
@@ -1259,7 +1260,7 @@ drm_atomic_helper_commit_crtc_disable(struct drm_device *dev, struct drm_atomic_
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
-		int ret;
+		bool vblank_off;
 
 		/* Shut down everything that needs a full modeset. */
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
@@ -1287,19 +1288,17 @@ drm_atomic_helper_commit_crtc_disable(struct drm_device *dev, struct drm_atomic_
 		if (!drm_dev_has_vblank(dev))
 			continue;
 
-		ret = drm_crtc_vblank_get(crtc);
+		vblank_off = drm_crtc_vblank_is_off(crtc);
 		/*
 		 * Self-refresh is not a true "disable"; ensure vblank remains
 		 * enabled.
 		 */
 		if (new_crtc_state->self_refresh_active)
-			WARN_ONCE(ret != 0,
+			WARN_ONCE(vblank_off,
 				  "driver disabled vblank in self-refresh\n");
 		else
-			WARN_ONCE(ret != -EINVAL,
+			WARN_ONCE(!vblank_off,
 				  "driver forgot to call drm_crtc_vblank_off()\n");
-		if (ret == 0)
-			drm_crtc_vblank_put(crtc);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_crtc_disable);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f893b1e3a596e..7d10515fe2ed5 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -131,6 +131,7 @@ static inline void drm_vblank_destroy_worker(struct drm_vblank_crtc *vblank)
 int drm_vblank_worker_init(struct drm_vblank_crtc *vblank);
 void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank);
 void drm_handle_vblank_works(struct drm_vblank_crtc *vblank);
+bool drm_crtc_vblank_is_off(struct drm_crtc *crtc);
 
 /* IOCTLS */
 int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f78bf37f1e0a7..983c131b23694 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2318,3 +2318,20 @@ void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
 }
 EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
+
+/**
+ * drm_crtc_vblank_is_off - check if vblank is off on a CRTC
+ * @crtc: CRTC in question
+ *
+ * Return true if vblanks is currently turned off via drm_crtc_vblank_off().
+ * Return False otherwise.
+ */
+bool drm_crtc_vblank_is_off(struct drm_crtc *crtc)
+{
+	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+
+	/* Spinlock adds the necessary barriers */
+	guard(spinlock_irqsave)(&crtc->dev->vbl_lock);
+	return vblank->inmodeset && !vblank->enabled;
+}
+EXPORT_SYMBOL(drm_crtc_vblank_is_off);
-- 
2.52.0

[PATCH 2/5] drm/vblank: Introduce deferred vblank enable/disable

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

Some drivers need to do blocking work as part of enabling/disabling
their vblank reporting mechanism in hardware(HW). Acquiring a mutex, for
example. However, the driver callbacks can be called from atomic
context, and therefore cannot block.

One solution is to have drivers defer their blocking work from their
enable/disable callbacks However, it can introduce concurrency between
the deferred work, and access to HW upon the callback's return. For
example, see drm_vblank_enable()'s call to drm_update_vblank_count()
that reads the HW vblank counter and scanout position immediately after
the driver callback returns. If the underlying HW remains accessible
when vblanks are disabled, then this wouldn't be an issue, but that's
not always the case.

This patch introduces a new per-device workqueue for deferring
drm_vblank_enable/disable_and_save(). Drivers can advertise their need
for deferred vblank by implementing the new
&drm_crtc_funcs->(pre|post)_(enable|disable)_vblank() callbacks, in
which they can do blocking work. DRM vblank will only defer if one or
more of these callbacks are implemented. Otherwise, the existing
non-deferred path will be used.

With deferred vblank enable, it is still possible that HW vblanks are
not ready by the time drm_vblank_get() returns. In cases where the
caller wants to ensure that vblank counts will increment (e.g. for
waiting on a specific vblank), this shouldn't be an issue: HW vblanks
will be enabled eventually, and the counter will progress (albeit with
some additional delay). But if the caller requires HW to be enabled upon
return (e.g. programming something that depends on HW being active), a
new drm_crtc_vblank_wait_deferred_enable() helper is provided. Drivers
can use it to wait for the enable work to complete before proceeding.

The &drm_crtc_funcs->(pre|post)_(enable|disable)_vblank() are also
inserted into drm_vblank_on and off(), as they call
drm_vblank_enable/disable(). I don't see a case where they're called
from atomic context, hence why they're not deferred. If that turns out
to be wrong (for a driver that needs to defer), we can change
enable/disable to be deferred there as well.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_drv.c    |   5 +
 drivers/gpu/drm/drm_vblank.c | 188 +++++++++++++++++++++++++++++++++--
 include/drm/drm_crtc.h       |  34 +++++++
 include/drm/drm_device.h     |   6 ++
 include/drm/drm_vblank.h     |  20 ++++
 5 files changed, 242 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 2915118436ce8..84884231f1f0e 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -696,6 +696,11 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
 	mutex_destroy(&dev->master_mutex);
 	mutex_destroy(&dev->clientlist_mutex);
 	mutex_destroy(&dev->filelist_mutex);
+
+	if (dev->deferred_vblank_wq) {
+		flush_workqueue(dev->deferred_vblank_wq);
+		destroy_workqueue(dev->deferred_vblank_wq);
+	}
 }
 
 static int drm_dev_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 983c131b23694..db17800f58e03 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -127,8 +127,11 @@
  * maintains a vertical blanking use count to ensure that the interrupts are not
  * disabled while a user still needs them. To increment the use count, drivers
  * call drm_crtc_vblank_get() and release the vblank reference again with
- * drm_crtc_vblank_put(). In between these two calls vblank interrupts are
- * guaranteed to be enabled.
+ * drm_crtc_vblank_put(). If drivers do not implement the deferred vblank
+ * callbacks (see &drm_crtc_funcs.pre_enable_vblank and related callbacks), in
+ * between these two calls vblank interrupts are guaranteed to be enabled.
+ * Otherwise, drivers have to wait for deferred enable via
+ * drm_crtc_vblank_wait_deferred_enable() in-between get() and put().
  *
  * On many hardware disabling the vblank interrupt cannot be done in a race-free
  * manner, see &drm_vblank_crtc_config.disable_immediate and
@@ -524,6 +527,9 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
 	timer_delete_sync(&vblank->disable_timer);
 }
 
+static void drm_vblank_deferred_enable_worker(struct work_struct *work);
+static void drm_vblank_deferred_disable_worker(struct work_struct *work);
+
 /**
  * drm_vblank_init - initialize vblank support
  * @dev: DRM device
@@ -548,6 +554,12 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
 	if (!dev->vblank)
 		return -ENOMEM;
 
+	dev->deferred_vblank_wq = alloc_workqueue("drm_vblank_deferred_wq",
+						  WQ_HIGHPRI | WQ_UNBOUND,
+						  0);
+	if (!dev->deferred_vblank_wq)
+		return -ENOMEM;
+
 	dev->num_crtcs = num_crtcs;
 
 	for (i = 0; i < num_crtcs; i++) {
@@ -567,6 +579,16 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
 		ret = drm_vblank_worker_init(vblank);
 		if (ret)
 			return ret;
+
+		INIT_WORK(&vblank->enable_work,
+			  drm_vblank_deferred_enable_worker);
+		INIT_DELAYED_WORK(&vblank->disable_work,
+				  drm_vblank_deferred_disable_worker);
+
+		init_completion(&vblank->enable_done);
+		/* Initialize in the completed state, the first deferred vblank
+		 * enable will reinitialize this. */
+		complete_all(&vblank->enable_done);
 	}
 
 	return 0;
@@ -595,6 +617,29 @@ bool drm_dev_has_vblank(const struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_dev_has_vblank);
 
+/**
+ * drm_crtc_needs_deferred_vblank - If crtc needs deferred vblank enable/disable
+ * @crtc: which CRTC to check
+ *
+ * If the driver implements any of the pre/post enable/disable vblank hooks
+ * where blocking work can be done, then vblank enable/disable need to be
+ * deferred.
+ *
+ * Returns:
+ * True if vblank enable/disable needs to be deferred, false otherwise.
+ */
+static bool drm_crtc_needs_deferred_vblank(const struct drm_crtc *crtc)
+{
+	const struct drm_crtc_funcs *funcs;
+
+	if (!crtc)
+		return false;
+
+	funcs = crtc->funcs;
+	return (funcs->pre_enable_vblank || funcs->post_enable_vblank ||
+		funcs->pre_disable_vblank || funcs->post_disable_vblank);
+}
+
 /**
  * drm_crtc_vblank_waitqueue - get vblank waitqueue for the CRTC
  * @crtc: which CRTC's vblank waitqueue to retrieve
@@ -1208,10 +1253,88 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 	return ret;
 }
 
+static void drm_vblank_deferred_enable_worker(struct work_struct *work)
+{
+	struct drm_vblank_crtc *vblank =
+		container_of(work, struct drm_vblank_crtc, enable_work);
+	struct drm_device *dev = vblank->dev;
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, vblank->pipe);
+	unsigned long irqflags;
+	int ret;
+
+	if (drm_WARN_ON(dev, !crtc))
+		return;
+
+	if (crtc->funcs->pre_enable_vblank)
+		crtc->funcs->pre_enable_vblank(crtc);
+
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+
+	/* Deferred enable should not error */
+	ret = drm_vblank_enable(dev, vblank->pipe);
+	drm_WARN(dev, ret, "CRTC-%d deferred vblank enable failed with %d\n",
+		 crtc->index, ret);
+	/* Deferred enable completed */
+	complete_all(&vblank->enable_done);
+
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+
+	if (crtc->funcs->post_enable_vblank)
+		crtc->funcs->post_enable_vblank(crtc);
+}
+
+static void drm_vblank_deferred_disable_worker(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct drm_vblank_crtc *vblank =
+		container_of(dwork, struct drm_vblank_crtc, disable_work);
+	struct drm_device *dev = vblank->dev;
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, vblank->pipe);
+
+	if (drm_WARN_ON(dev, !crtc))
+		return;
+
+	if (crtc->funcs->pre_disable_vblank)
+		crtc->funcs->pre_disable_vblank(crtc);
+
+	vblank_disable_fn(&vblank->disable_timer);
+
+	if (crtc->funcs->post_disable_vblank)
+		crtc->funcs->post_disable_vblank(crtc);
+}
+
+/**
+ * drm_crtc_vblank_wait_deferred_enable - wait for deferred enable to complete
+ *
+ * @crtc: the CRTC to wait on
+ *
+ * After vblank_get() queues a vblank_enable() worker, wait for the worker to
+ * complete the enable. Drivers that defer vblank enable and use this to wait on
+ * HW vblank enable before continuing with programming that might race with it.
+ *
+ * If the CRTC does not need deferred enable, this function does nothing.
+ *
+ * Can block, and therefore must be called from process context.
+ */
+void drm_crtc_vblank_wait_deferred_enable(struct drm_crtc *crtc)
+{
+	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+
+	if (!drm_crtc_needs_deferred_vblank(crtc))
+		return;
+
+	if (!wait_for_completion_timeout(&vblank->enable_done,
+	    msecs_to_jiffies(1000)))
+		drm_err(crtc->dev, "CRTC-%d: Timed out waiting for deferred vblank enable\n",
+			 drm_crtc_index(crtc));
+}
+EXPORT_SYMBOL(drm_crtc_vblank_wait_deferred_enable);
+
 int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
 	unsigned long irqflags;
+	bool needs_deferred_enable;
 	int ret = 0;
 
 	if (!drm_dev_has_vblank(dev))
@@ -1220,12 +1343,21 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return -EINVAL;
 
+	needs_deferred_enable =
+		drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe));
+
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
-		ret = drm_vblank_enable(dev, pipe);
+		if (needs_deferred_enable) {
+			/* Arm completion before queueing deferred enable */
+			reinit_completion(&vblank->enable_done);
+			queue_work(dev->deferred_vblank_wq, &vblank->enable_work);
+		} else {
+			ret = drm_vblank_enable(dev, pipe);
+		}
 	} else {
-		if (!vblank->enabled) {
+		if ((!needs_deferred_enable && !vblank->enabled)) {
 			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
 		}
@@ -1255,6 +1387,7 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
 	int vblank_offdelay = vblank->config.offdelay_ms;
+	bool needs_deferred_disable;
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
@@ -1262,13 +1395,28 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 	if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0))
 		return;
 
+	needs_deferred_disable =
+		drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe));
+
 	/* Last user schedules interrupt disable */
-	if (atomic_dec_and_test(&vblank->refcount)) {
-		if (!vblank_offdelay)
-			return;
-		else if (vblank_offdelay < 0)
+	if (!atomic_dec_and_test(&vblank->refcount))
+		return;
+
+	if (!vblank_offdelay)
+		return;
+	else if (vblank_offdelay < 0) {
+		if (needs_deferred_disable)
+			mod_delayed_work(dev->deferred_vblank_wq,
+					 &vblank->disable_work,
+					 0);
+		else
 			vblank_disable_fn(&vblank->disable_timer);
-		else if (!vblank->config.disable_immediate)
+	} else if (!vblank->config.disable_immediate) {
+		if (needs_deferred_disable)
+			mod_delayed_work(dev->deferred_vblank_wq,
+					 &vblank->disable_work,
+					 msecs_to_jiffies(vblank_offdelay));
+		else
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((vblank_offdelay * HZ) / 1000));
 	}
@@ -1348,6 +1496,9 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
+	if (crtc->funcs->pre_disable_vblank)
+		crtc->funcs->pre_disable_vblank(crtc);
+
 	/*
 	 * Grab event_lock early to prevent vblank work from being scheduled
 	 * while we're in the middle of shutting down vblank interrupts
@@ -1394,6 +1545,9 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 
 	spin_unlock_irq(&dev->event_lock);
 
+	if (crtc->funcs->post_disable_vblank)
+		crtc->funcs->post_disable_vblank(crtc);
+
 	/* Will be reset by the modeset helpers when re-enabling the crtc by
 	 * calling drm_calc_timestamping_constants(). */
 	vblank->hwmode.crtc_clock = 0;
@@ -1489,6 +1643,9 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
+	if (crtc->funcs->pre_enable_vblank)
+		crtc->funcs->pre_enable_vblank(crtc);
+
 	spin_lock_irq(&dev->vbl_lock);
 	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
 		    pipe, vblank->enabled, vblank->inmodeset);
@@ -1510,6 +1667,9 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
 	if (atomic_read(&vblank->refcount) != 0 || !vblank->config.offdelay_ms)
 		drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
 	spin_unlock_irq(&dev->vbl_lock);
+
+	if (crtc->funcs->post_enable_vblank)
+		crtc->funcs->post_enable_vblank(crtc);
 }
 EXPORT_SYMBOL(drm_crtc_vblank_on_config);
 
@@ -1962,8 +2122,14 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 
-	if (disable_irq)
-		vblank_disable_fn(&vblank->disable_timer);
+	if (disable_irq) {
+		if (drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe)))
+			mod_delayed_work(dev->deferred_vblank_wq,
+					 &vblank->disable_work,
+					 0);
+		else
+			vblank_disable_fn(&vblank->disable_timer);
+	}
 
 	return true;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 66278ffeebd68..1088b63b64816 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -892,6 +892,40 @@ struct drm_crtc_funcs {
 	 */
 	void (*disable_vblank)(struct drm_crtc *crtc);
 
+	/**
+	 * @pre_enable_vblank:
+	 *
+	 * Optional callback to do blocking work prior to vblank_enable(). If
+	 * set, vblank enable/disable will be deferred to a single-threaded
+	 * worker.
+	 */
+	void (*pre_enable_vblank)(struct drm_crtc *crtc);
+
+	/**
+	 * @post_enable_vblank:
+	 *
+	 * Optional callback to do blocking work after vblank_enable(). If set,
+	 * vblank enable/disable will be deferred to a single-threaded worker.
+	 */
+	void (*post_enable_vblank)(struct drm_crtc *crtc);
+
+	/**
+	 * @pre_disable_vblank:
+	 *
+	 * Optional callback to do blocking work prior to vblank_disable(). If
+	 * set, vblank enable/disable will be deferred to a single-threaded
+	 * worker.
+	 */
+	void (*pre_disable_vblank)(struct drm_crtc *crtc);
+
+	/**
+	 * @post_disable_vblank:
+	 *
+	 * Optional callback to do blocking work after vblank_disable(). If set,
+	 * vblank enable/disable will be deferred to a single-threaded worker.
+	 */
+	void (*post_disable_vblank)(struct drm_crtc *crtc);
+
 	/**
 	 * @get_vblank_timestamp:
 	 *
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index bc78fb77cc279..c0e22f8d868b2 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -333,6 +333,12 @@ struct drm_device {
 	 */
 	spinlock_t event_lock;
 
+	/**
+	 * @deferred_vblank_wq: Workqueue used for deferred vblank
+	 * enable/disable work.
+	 */
+	struct workqueue_struct *deferred_vblank_wq;
+
 	/** @num_crtcs: Number of CRTCs on this device */
 	unsigned int num_crtcs;
 
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1b..6bd3cbf1bb831 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -282,6 +282,25 @@ struct drm_vblank_crtc {
 	 * @vblank_timer: Holds the state of the vblank timer
 	 */
 	struct drm_vblank_crtc_timer vblank_timer;
+
+	/**
+	 * @enable_work: Deferred enable work for this vblank CRTC
+	 */
+	struct work_struct enable_work;
+
+	/**
+	 * @disable_work: Delayed disable work for this vblank CRTC
+	 */
+	struct delayed_work disable_work;
+
+	/**
+	 * @enable_done: Signals completion of deferred vblank enable
+	 *
+	 * If deferred enable work is needed, it is reinitialized before
+	 * queueing the enable worker, and completed after the deferred
+	 * drm_vblank_enable() completes.
+	 */
+	struct completion enable_done;
 };
 
 struct drm_vblank_crtc *drm_crtc_vblank_crtc(struct drm_crtc *crtc);
@@ -302,6 +321,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
 bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
 int drm_crtc_vblank_get(struct drm_crtc *crtc);
 void drm_crtc_vblank_put(struct drm_crtc *crtc);
+void drm_crtc_vblank_wait_deferred_enable(struct drm_crtc *crtc);
 int drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
 void drm_crtc_vblank_off(struct drm_crtc *crtc);
 void drm_crtc_vblank_reset(struct drm_crtc *crtc);
-- 
2.52.0

[PATCH 3/5] drm/amd/display: Refactor amdgpu_dm_crtc_set_vblank

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

[Why]

In preparation of implementing deferred vblank work, refactor
amdgpu_dm_crtc_set_vblank to split out the actual enabling/disabling of
vblank interrupts + restoring vblank counts/timestamps from the rest.

[How]

* Move the vblank_control_work init and queueing out into
  amdgpu_dm_crtc_queue_vblank_work()
* Move crtc->enabled check out to parent function
  amdgpu_dm_crtc_enable_vblank()
* Call amdgpu_dm_crtc_queue_vblank_work() from parent functions
  amdgpu_dm_crtc_(enable|disable)_vblank()
* As a drive-by cleanup, make amdgpu_dm_crtc_(enable|disable)_vblank()
  static; they're not called from anywhere else.

No functional changes are intended.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 88 +++++++++++++------
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.h    |  4 -
 2 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 130190e8a1b29..3df38f3cb7423 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -293,20 +293,12 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
 	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
 	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
 	struct amdgpu_display_manager *dm = &adev->dm;
-	struct vblank_control_work *work;
 	int irq_type;
 	int rc = 0;
 
-	if (enable && !acrtc->base.enabled) {
-		drm_dbg_vbl(crtc->dev,
-				"Reject vblank enable on unconfigured CRTC %d (enabled=%d)\n",
-				acrtc->crtc_id, acrtc->base.enabled);
-		return -EINVAL;
-	}
-
 	irq_type = amdgpu_display_crtc_idx_to_irq_type(adev, acrtc->crtc_id);
 
-	if (enable) {
+	if (enable && crtc->enabled) {
 		struct dc *dc = adev->dm.dc;
 		struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 		struct psr_settings *psr = &acrtc_state->stream->link->psr_settings;
@@ -387,39 +379,79 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
 			return rc;
 	}
 #endif
+	return 0;
+}
 
-	if (amdgpu_in_reset(adev))
-		return 0;
-
-	if (dm->vblank_control_workqueue) {
-		work = kzalloc_obj(*work, GFP_ATOMIC);
-		if (!work)
-			return -ENOMEM;
+static int amdgpu_dm_crtc_queue_vblank_work(struct amdgpu_display_manager *dm,
+					    struct amdgpu_crtc *acrtc,
+					    struct dm_crtc_state *acrtc_state,
+					    bool enable)
+{
+	struct vblank_control_work *work;
 
-		INIT_WORK(&work->work, amdgpu_dm_crtc_vblank_control_worker);
-		work->dm = dm;
-		work->acrtc = acrtc;
-		work->enable = enable;
+	work = kzalloc_obj(*work, GFP_ATOMIC);
+	if (!work)
+		return -ENOMEM;
 
-		if (acrtc_state->stream) {
-			dc_stream_retain(acrtc_state->stream);
-			work->stream = acrtc_state->stream;
-		}
+	INIT_WORK(&work->work, amdgpu_dm_crtc_vblank_control_worker);
+	work->dm = dm;
+	work->acrtc = acrtc;
+	work->enable = enable;
 
-		queue_work(dm->vblank_control_workqueue, &work->work);
+	if (acrtc_state->stream) {
+		dc_stream_retain(acrtc_state->stream);
+		work->stream = acrtc_state->stream;
 	}
 
+	queue_work(dm->vblank_control_workqueue, &work->work);
+
 	return 0;
 }
 
-int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
+static int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
 {
-	return amdgpu_dm_crtc_set_vblank(crtc, true);
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct amdgpu_display_manager *dm = &adev->dm;
+	int ret;
+
+	if (!crtc->enabled) {
+		drm_dbg_vbl(crtc->dev,
+				"Reject vblank enable on unconfigured CRTC %d (enabled=%d)\n",
+				crtc->index, crtc->enabled);
+		return -EINVAL;
+	}
+
+	ret = amdgpu_dm_crtc_set_vblank(crtc, true);
+	if (ret)
+		return ret;
+
+	if (amdgpu_in_reset(adev))
+		return 0;
+
+	if (dm->vblank_control_workqueue)
+		return amdgpu_dm_crtc_queue_vblank_work(dm, acrtc,
+							acrtc_state, true);
+
+	return 0;
 }
 
-void amdgpu_dm_crtc_disable_vblank(struct drm_crtc *crtc)
+static void amdgpu_dm_crtc_disable_vblank(struct drm_crtc *crtc)
 {
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct amdgpu_display_manager *dm = &adev->dm;
+
 	amdgpu_dm_crtc_set_vblank(crtc, false);
+
+	if (amdgpu_in_reset(adev))
+		return;
+
+	if (dm->vblank_control_workqueue)
+		amdgpu_dm_crtc_queue_vblank_work(dm, acrtc,
+						 acrtc_state, false);
 }
 
 static void amdgpu_dm_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.h
index c1212947a77b8..655a6c4f83fb8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.h
@@ -39,10 +39,6 @@ bool amdgpu_dm_crtc_vrr_active_irq(struct amdgpu_crtc *acrtc);
 
 bool amdgpu_dm_crtc_vrr_active(const struct dm_crtc_state *dm_state);
 
-int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc);
-
-void amdgpu_dm_crtc_disable_vblank(struct drm_crtc *crtc);
-
 int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
 			struct drm_plane *plane,
 			uint32_t link_index);
-- 
2.52.0

[PATCH 4/5] drm/amd/display: Implement deferred vblanks on IPS platforms

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

[Why]

Newer generations of Display Core Next (DCN) hardware allow for the
entire DCN block to be power-gated in a feature called Idle Power States
(IPS). Once DCN is in IPS, HW register access will timeout. Therefore,
allowing/disallowing IPS requires synchronization with the rest of the
driver through the big dc_lock mutex. It happens that writing vblank
interrupt control and reading counter/scanout position registers
requires IPS disallow. Since that requires the dc_lock, it turns into a
blocking operation.

The immediately obvious (not)solution is to disable IPS. But since it
provides sizable power savings, and it's not going away for future APUs,
it's not a good option.

The other (non)solution is to get rid of the dc_lock or make it not
block. But since all hardware (HW) and Display Micro-controller (DMCU)
access is invalid when DCN is in IPS (because everything is gated), it
needs to be synchronized with all other HW/DMCU access. It happens that
the dc_lock is responsible for that. I don't think replacing it with a
spinlock is a valid solution either. Consider link training: IPS cannot
be allowed while that is happening, yet the timescale of link training
can be in the 100ms. A spinlock would spin too long here.

So let's implement the newly introduced deferred vblank enable/disable
callbacks in DRM vblank core.

[How]

Implement the deferred CRTC callbacks to request the DRM vblank core to
defer vblank enable/disable. Only implement for ASICs that have IPS
support. Otherwise, keep the non-deferred behavior.

To ensure interrupts are enabled before programming flips (otherwise the
page flip interrupt for event delivery may not fire), call
drm_crtc_vblank_wait_deferred_enable() in the commit_planes code when
getting a reference on vblanks.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   8 +
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 177 +++++++++++++++++-
 2 files changed, 184 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a8e4e3ab5e402..0405466666e2f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10268,6 +10268,14 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 				&acrtc_attach->dm_irq_params.vrr_params.adjust);
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
+
+		/*
+		 * If vblank is being enabled in worker thread, wait for it to
+		 * enable interrupts before programming pflips, otherwise the
+		 * interrupt may not fire.
+		 */
+		drm_crtc_vblank_wait_deferred_enable(pcrtc);
+
 		mutex_lock(&dm->dc_lock);
 		update_planes_and_stream_adapter(dm->dc,
 					 acrtc_state->update_type,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
index 3df38f3cb7423..fd84977f55ecd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
@@ -454,6 +454,140 @@ static void amdgpu_dm_crtc_disable_vblank(struct drm_crtc *crtc)
 						 acrtc_state, false);
 }
 
+/*
+ * Deferred vblank enable/disable works differently: the drm vblank core manages
+ * the workqueue instead of amdgpu_dm, sandwiching the vblank_enable/disable()
+ * with the crtc pre/post callbacks. Therefore, they need to be sequenced
+ * differently from their non-deferred variants.
+ */
+
+static int amdgpu_dm_crtc_deferred_enable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+	int ret;
+
+	ret = amdgpu_dm_crtc_set_vblank(crtc, true);
+	if (!ret)
+		dm->active_vblank_irq_count++;
+
+	return ret;
+}
+
+static void amdgpu_dm_crtc_deferred_disable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+
+	amdgpu_dm_crtc_set_vblank(crtc, false);
+
+	if (dm->active_vblank_irq_count)
+		dm->active_vblank_irq_count--;
+}
+
+
+static void amdgpu_dm_crtc_pre_enable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+
+	mutex_lock(&dm->dc_lock);
+	dc_allow_idle_optimizations(dm->dc, false);
+}
+
+static void amdgpu_dm_crtc_post_enable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	struct vblank_control_work vblank_work = { 0 };
+
+	/* If crtc disabled, skip panel optimizations exit */
+	if (!crtc->enabled) {
+		mutex_unlock(&dm->dc_lock);
+		return;
+	}
+
+	vblank_work.dm = dm;
+	vblank_work.acrtc = acrtc;
+	vblank_work.enable = true;
+	if (acrtc_state->stream) {
+		dc_stream_retain(acrtc_state->stream);
+		vblank_work.stream = acrtc_state->stream;
+	}
+
+	/*
+	 * Control PSR based on vblank requirements from OS
+	 *
+	 * If panel supports PSR SU, there's no need to disable PSR when OS is
+	 * submitting fast atomic commits (we infer this by whether the OS
+	 * requests vblank events). Fast atomic commits will simply trigger a
+	 * full-frame-update (FFU); a specific case of selective-update (SU)
+	 * where the SU region is the full hactive*vactive region. See
+	 * fill_dc_dirty_rects().
+	 */
+	if (vblank_work.stream && vblank_work.stream->link && vblank_work.acrtc) {
+		amdgpu_dm_crtc_set_panel_sr_feature(
+			&vblank_work, true,
+			vblank_work.acrtc->dm_irq_params.allow_sr_entry);
+	}
+
+	if (vblank_work.stream)
+		dc_stream_release(vblank_work.stream);
+
+	mutex_unlock(&dm->dc_lock);
+}
+
+static void amdgpu_dm_crtc_pre_disable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+
+	mutex_lock(&dm->dc_lock);
+}
+
+static void amdgpu_dm_crtc_post_disable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	struct vblank_control_work vblank_work = { 0 };
+	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+
+	/* If a vblank_get got ahead of us (can happen when !disable_immediate),
+	 * skip panel optimizations */
+	if (atomic_read(&vblank->refcount) > 0) {
+		mutex_unlock(&dm->dc_lock);
+		return;
+	}
+
+	vblank_work.dm = dm;
+	vblank_work.acrtc = acrtc;
+	vblank_work.enable = false;
+	if (acrtc_state->stream) {
+		dc_stream_retain(acrtc_state->stream);
+		vblank_work.stream = acrtc_state->stream;
+	}
+
+	if (vblank_work.stream && vblank_work.stream->link && vblank_work.acrtc) {
+		amdgpu_dm_crtc_set_panel_sr_feature(
+			&vblank_work, false,
+			vblank_work.acrtc->dm_irq_params.allow_sr_entry);
+	}
+
+	if (vblank_work.stream)
+		dc_stream_release(vblank_work.stream);
+
+	if (dm->active_vblank_irq_count == 0) {
+		dc_post_update_surfaces_to_stream(dm->dc);
+		dc_allow_idle_optimizations(dm->dc, true);
+	}
+
+	mutex_unlock(&dm->dc_lock);
+}
+
 static void amdgpu_dm_crtc_destroy_state(struct drm_crtc *crtc,
 				  struct drm_crtc_state *state)
 {
@@ -623,6 +757,33 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 #endif
 };
 
+static const struct drm_crtc_funcs amdgpu_dm_crtc_deferred_vblank_funcs = {
+	.reset = amdgpu_dm_crtc_reset_state,
+	.destroy = amdgpu_dm_crtc_destroy,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = amdgpu_dm_crtc_duplicate_state,
+	.atomic_destroy_state = amdgpu_dm_crtc_destroy_state,
+	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
+	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
+	.get_crc_sources = amdgpu_dm_crtc_get_crc_sources,
+	.get_vblank_counter = amdgpu_get_vblank_counter_kms,
+	.enable_vblank = amdgpu_dm_crtc_deferred_enable_vblank,
+	.disable_vblank = amdgpu_dm_crtc_deferred_disable_vblank,
+	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
+#if defined(CONFIG_DEBUG_FS)
+	.late_register = amdgpu_dm_crtc_late_register,
+#endif
+#ifdef AMD_PRIVATE_COLOR
+	.atomic_set_property = amdgpu_dm_atomic_crtc_set_property,
+	.atomic_get_property = amdgpu_dm_atomic_crtc_get_property,
+#endif
+	.pre_enable_vblank = amdgpu_dm_crtc_pre_enable_vblank,
+	.post_enable_vblank = amdgpu_dm_crtc_post_enable_vblank,
+	.pre_disable_vblank = amdgpu_dm_crtc_pre_disable_vblank,
+	.post_disable_vblank = amdgpu_dm_crtc_post_disable_vblank,
+};
+
 static void amdgpu_dm_crtc_helper_disable(struct drm_crtc *crtc)
 {
 }
@@ -755,6 +916,7 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
 			       struct drm_plane *plane,
 			       uint32_t crtc_index)
 {
+	const struct drm_crtc_funcs *crtc_funcs;
 	struct amdgpu_crtc *acrtc = NULL;
 	struct drm_plane *cursor_plane;
 	bool has_degamma;
@@ -771,12 +933,25 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
 	if (!acrtc)
 		goto fail;
 
+	/*
+	 * Only enable deferred vblank enable/disable for ASICs with IPS
+	 * support
+	 */
+	if (dm->dc->caps.ips_support) {
+		crtc_funcs = &amdgpu_dm_crtc_deferred_vblank_funcs;
+		drm_dbg_driver(dm->ddev,
+			       "Initializing CRTC %d with deferred vBlank enable/disable\n",
+			       crtc_index);
+	} else {
+		crtc_funcs = &amdgpu_dm_crtc_funcs;
+	}
+
 	res = drm_crtc_init_with_planes(
 			dm->ddev,
 			&acrtc->base,
 			plane,
 			cursor_plane,
-			&amdgpu_dm_crtc_funcs, NULL);
+			crtc_funcs, NULL);
 
 	if (res)
 		goto fail;
-- 
2.52.0

[PATCH 5/5] drm/vblank: Add some debugging trace events

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

It's useful to trace vblank get/put and enable/disable (plus their
deferred variants) when debugging. The trace stack feature for events
can be especially useful.

Using trace-cmd, one can obtain a trace like so:

    trace-cmd record -e drm_vblank*
    # With deferred events and stack info:
    trace-cmd record -e drm_vblank* -e drm_deferred_vblank* -T

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_trace.h  | 112 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_vblank.c |  34 +++++++++--
 2 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h
index 11c6dd577e8ed..3372513a10eeb 100644
--- a/drivers/gpu/drm/drm_trace.h
+++ b/drivers/gpu/drm/drm_trace.h
@@ -66,6 +66,118 @@ TRACE_EVENT(drm_vblank_event_delivered,
 		      __entry->seq)
 );
 
+DECLARE_EVENT_CLASS(drm_vblank_get_put_template,
+		    TP_PROTO(int crtc, int refcount),
+		    TP_ARGS(crtc, refcount),
+		    TP_STRUCT__entry(
+			__field(int, crtc)
+			__field(int, refcount)
+		    ),
+		    TP_fast_assign(
+			__entry->crtc = crtc;
+			__entry->refcount = refcount;
+		    ),
+		    TP_printk(
+			"crtc=%d, refcount=%u",
+			__entry->crtc, __entry->refcount
+		    )
+);
+
+DEFINE_EVENT(drm_vblank_get_put_template, drm_vblank_get,
+	     TP_PROTO(int crtc, int refcount),
+	     TP_ARGS(crtc, refcount));
+
+/* put's refcount not sync'd using vbl_lock, use for debugging purposes only */
+DEFINE_EVENT(drm_vblank_get_put_template, drm_vblank_put,
+	     TP_PROTO(int crtc, int refcount),
+	     TP_ARGS(crtc, refcount));
+
+DECLARE_EVENT_CLASS(drm_vblank_on_off_template,
+		    TP_PROTO(int crtc, int refcount, bool enabled, bool inmodeset),
+		    TP_ARGS(crtc, refcount, enabled, inmodeset),
+		    TP_STRUCT__entry(
+			__field(int, crtc)
+			__field(int, refcount)
+			__field(bool, enabled)
+			__field(bool, inmodeset)
+		    ),
+		    TP_fast_assign(
+			__entry->crtc = crtc;
+			__entry->refcount = refcount;
+			__entry->enabled = enabled;
+			__entry->inmodeset = inmodeset;
+		    ),
+		    TP_printk(
+			"crtc=%d, refcount=%u, enabled=%s, inmodeset=%s",
+			__entry->crtc, __entry->refcount,
+			__entry->enabled ? "true" : "false",
+			__entry->inmodeset ? "true" : "false"
+		    )
+);
+
+DEFINE_EVENT(drm_vblank_on_off_template, drm_vblank_on,
+	     TP_PROTO(int crtc, int refcount, bool enabled, bool inmodeset),
+	     TP_ARGS(crtc, refcount, enabled, inmodeset));
+
+DEFINE_EVENT(drm_vblank_on_off_template, drm_vblank_off,
+	     TP_PROTO(int crtc, int refcount, bool enabled, bool inmodeset),
+	     TP_ARGS(crtc, refcount, enabled, inmodeset));
+
+DECLARE_EVENT_CLASS(drm_deferred_vblank_template,
+		    TP_PROTO(int crtc),
+		    TP_ARGS(crtc),
+		    TP_STRUCT__entry(
+			__field(int, crtc)
+		    ),
+		    TP_fast_assign(
+			__entry->crtc = crtc;
+		    ),
+		    TP_printk(
+			"crtc=%d",
+			__entry->crtc
+		    )
+);
+
+DEFINE_EVENT(drm_deferred_vblank_template, drm_deferred_vblank_enable_queued,
+	     TP_PROTO(int crtc),
+	     TP_ARGS(crtc));
+
+DEFINE_EVENT(drm_deferred_vblank_template, drm_deferred_vblank_enable,
+	     TP_PROTO(int crtc),
+	     TP_ARGS(crtc));
+
+TRACE_EVENT(drm_deferred_vblank_disable_queued,
+	    TP_PROTO(int crtc, int delay_ms),
+	    TP_ARGS(crtc, delay_ms),
+	    TP_STRUCT__entry(
+		__field(int, crtc)
+		__field(int, delay_ms)
+	    ),
+	    TP_fast_assign(
+		__entry->crtc = crtc;
+		__entry->delay_ms = delay_ms;
+	    ),
+	    TP_printk(
+		"crtc=%d, delay_ms=%d",
+		__entry->crtc,
+		__entry->delay_ms
+	    )
+);
+
+DEFINE_EVENT(drm_deferred_vblank_template, drm_deferred_vblank_disable,
+	     TP_PROTO(int crtc),
+	     TP_ARGS(crtc));
+
+DEFINE_EVENT(drm_deferred_vblank_template,
+	     drm_deferred_vblank_wait_enable_start,
+	     TP_PROTO(int crtc),
+	     TP_ARGS(crtc));
+
+DEFINE_EVENT(drm_deferred_vblank_template,
+	     drm_deferred_vblank_wait_enable_end,
+	     TP_PROTO(int crtc),
+	     TP_ARGS(crtc));
+
 #endif /* _DRM_TRACE_H_ */
 
 /* This part must be outside protection */
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index db17800f58e03..afa918c508bef 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1265,6 +1265,8 @@ static void drm_vblank_deferred_enable_worker(struct work_struct *work)
 	if (drm_WARN_ON(dev, !crtc))
 		return;
 
+	trace_drm_deferred_vblank_enable(crtc->index);
+
 	if (crtc->funcs->pre_enable_vblank)
 		crtc->funcs->pre_enable_vblank(crtc);
 
@@ -1294,6 +1296,8 @@ static void drm_vblank_deferred_disable_worker(struct work_struct *work)
 	if (drm_WARN_ON(dev, !crtc))
 		return;
 
+	trace_drm_deferred_vblank_disable(crtc->index);
+
 	if (crtc->funcs->pre_disable_vblank)
 		crtc->funcs->pre_disable_vblank(crtc);
 
@@ -1323,10 +1327,14 @@ void drm_crtc_vblank_wait_deferred_enable(struct drm_crtc *crtc)
 	if (!drm_crtc_needs_deferred_vblank(crtc))
 		return;
 
+	trace_drm_deferred_vblank_wait_enable_start(crtc->index);
+
 	if (!wait_for_completion_timeout(&vblank->enable_done,
 	    msecs_to_jiffies(1000)))
 		drm_err(crtc->dev, "CRTC-%d: Timed out waiting for deferred vblank enable\n",
 			 drm_crtc_index(crtc));
+
+	trace_drm_deferred_vblank_wait_enable_end(crtc->index);
 }
 EXPORT_SYMBOL(drm_crtc_vblank_wait_deferred_enable);
 
@@ -1347,11 +1355,15 @@ int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 		drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe));
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+
+	trace_drm_vblank_get(pipe, atomic_read(&vblank->refcount));
+
 	/* Going from 0->1 means we have to enable interrupts again */
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
 		if (needs_deferred_enable) {
 			/* Arm completion before queueing deferred enable */
 			reinit_completion(&vblank->enable_done);
+			trace_drm_deferred_vblank_enable_queued(pipe);
 			queue_work(dev->deferred_vblank_wq, &vblank->enable_work);
 		} else {
 			ret = drm_vblank_enable(dev, pipe);
@@ -1398,6 +1410,8 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 	needs_deferred_disable =
 		drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe));
 
+	trace_drm_vblank_put(pipe, atomic_read(&vblank->refcount));
+
 	/* Last user schedules interrupt disable */
 	if (!atomic_dec_and_test(&vblank->refcount))
 		return;
@@ -1405,18 +1419,21 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
 	if (!vblank_offdelay)
 		return;
 	else if (vblank_offdelay < 0) {
-		if (needs_deferred_disable)
+		if (needs_deferred_disable) {
+			trace_drm_deferred_vblank_disable_queued(pipe, 0);
 			mod_delayed_work(dev->deferred_vblank_wq,
 					 &vblank->disable_work,
 					 0);
-		else
+		} else
 			vblank_disable_fn(&vblank->disable_timer);
 	} else if (!vblank->config.disable_immediate) {
-		if (needs_deferred_disable)
+		if (needs_deferred_disable) {
+			trace_drm_deferred_vblank_disable_queued(
+				pipe, vblank_offdelay);
 			mod_delayed_work(dev->deferred_vblank_wq,
 					 &vblank->disable_work,
 					 msecs_to_jiffies(vblank_offdelay));
-		else
+		} else
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((vblank_offdelay * HZ) / 1000));
 	}
@@ -1508,6 +1525,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 	spin_lock(&dev->vbl_lock);
 	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
 		    pipe, vblank->enabled, vblank->inmodeset);
+	trace_drm_vblank_off(pipe, atomic_read(&vblank->refcount),
+			     vblank->enabled, vblank->inmodeset);
 
 	/* Avoid redundant vblank disables without previous
 	 * drm_crtc_vblank_on(). */
@@ -1649,6 +1668,8 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
 	spin_lock_irq(&dev->vbl_lock);
 	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
 		    pipe, vblank->enabled, vblank->inmodeset);
+	trace_drm_vblank_on(pipe, atomic_read(&vblank->refcount),
+			    vblank->enabled, vblank->inmodeset);
 
 	vblank->config = *config;
 
@@ -2123,11 +2144,12 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 
 	if (disable_irq) {
-		if (drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe)))
+		if (drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe))) {
+			trace_drm_deferred_vblank_disable_queued(pipe, 0);
 			mod_delayed_work(dev->deferred_vblank_wq,
 					 &vblank->disable_work,
 					 0);
-		else
+		} else
 			vblank_disable_fn(&vblank->disable_timer);
 	}
 
-- 
2.52.0

Re: [PATCH 2/5] drm/vblank: Introduce deferred vblank enable/disable

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on daeinki-drm-exynos/exynos-drm-next drm/drm-next drm-i915/for-linux-next-fixes drm-tip/drm-tip linus/master v7.0-rc1 next-20260224]
[cannot apply to drm-i915/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/sunpeng-li-amd-com/drm-vblank-Add-drm_crtc_vblank_is_off-helper/20260225-052945
base:   https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link:    https://lore.kernel.org/r/20260224212639.390768-3-sunpeng.li%40amd.com
patch subject: [PATCH 2/5] drm/vblank: Introduce deferred vblank enable/disable
config: microblaze-randconfig-r053-20260225 (https://download.01.org/0day-ci/archive/20260225/202602251257.hwgG2w64-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 8.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602251257.hwgG2w64-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/drm_vblank.c:1402:6-25: WARNING: atomic_dec_and_test variation before object free at line 1409.
   drivers/gpu/drm/drm_vblank.c:1402:6-25: WARNING: atomic_dec_and_test variation before object free at line 1416.

vim +1402 drivers/gpu/drm/drm_vblank.c

  1385	
  1386	void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
  1387	{
  1388		struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
  1389		int vblank_offdelay = vblank->config.offdelay_ms;
  1390		bool needs_deferred_disable;
  1391	
  1392		if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
  1393			return;
  1394	
  1395		if (drm_WARN_ON(dev, atomic_read(&vblank->refcount) == 0))
  1396			return;
  1397	
  1398		needs_deferred_disable =
  1399			drm_crtc_needs_deferred_vblank(drm_crtc_from_index(dev, pipe));
  1400	
  1401		/* Last user schedules interrupt disable */
> 1402		if (!atomic_dec_and_test(&vblank->refcount))
  1403			return;
  1404	
  1405		if (!vblank_offdelay)
  1406			return;
  1407		else if (vblank_offdelay < 0) {
  1408			if (needs_deferred_disable)
> 1409				mod_delayed_work(dev->deferred_vblank_wq,
  1410						 &vblank->disable_work,
  1411						 0);
  1412			else
  1413				vblank_disable_fn(&vblank->disable_timer);
  1414		} else if (!vblank->config.disable_immediate) {
  1415			if (needs_deferred_disable)
  1416				mod_delayed_work(dev->deferred_vblank_wq,
  1417						 &vblank->disable_work,
  1418						 msecs_to_jiffies(vblank_offdelay));
  1419			else
  1420				mod_timer(&vblank->disable_timer,
  1421					  jiffies + ((vblank_offdelay * HZ) / 1000));
  1422		}
  1423	}
  1424	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Claude review: drm/vblank: Deferred Enable and Disable

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vblank: Deferred Enable and Disable
Author: <sunpeng.li@amd.com>
Patches: 7
Reviewed: 2026-02-27T14:40:52.376367

---

This series introduces **deferred vblank enable/disable** to the DRM vblank core, motivated by AMD's IPS (Idle Power States) feature where the entire DCN block can be power-gated, requiring the `dc_lock` mutex (blocking) to disallow idle optimizations before accessing vblank HW registers. The approach is well-motivated and the cover letter does a good job explaining the tradeoffs vs. the previously attempted `vblank_prepare()` approach.

However, there are **several concerns** with the current implementation, primarily around race conditions between deferred work and `drm_crtc_vblank_off()`, unconditional resource allocation, and `crtc->state` access from worker context. The core idea is sound but needs another iteration to tighten the synchronization.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: Add drm_crtc_vblank_is_off() helper

[Claude Code Review Bot]

Patch Review

**Generally clean.** The refactoring correctly replaces the `drm_crtc_vblank_get()` + check return value pattern with a direct state query, removing the need for a balancing `drm_crtc_vblank_put()`.

The condition in `drm_crtc_vblank_is_off()`:
```c
guard(spinlock_irqsave)(&crtc->dev->vbl_lock);
return vblank->inmodeset && !vblank->enabled;
```
correctly captures the "vblank_off was called" state — `inmodeset` is set in `drm_crtc_vblank_off()` when it bumps the refcount, and `enabled` is cleared by `drm_vblank_disable_and_save()`. This is semantically what the original code was checking.

**Minor issues:**

1. The function is declared in `drm_internal.h` but uses `EXPORT_SYMBOL`. This is needed because `drm_atomic_helper.c` compiles into a separate module (`drm_kms_helper`), but it's a slightly awkward pattern — an exported symbol with no public header declaration. Consider whether a public header (e.g., `drm_vblank.h`) would be more appropriate if other drivers/helpers might eventually need this.

2. The kdoc says "Return False otherwise" with a capital 'F' — should be lowercase "false" for consistency.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: Introduce deferred vblank enable/disable

[Claude Code Review Bot]

Patch Review

This is the core patch and has the most substance. The design — wrapping `drm_vblank_enable()`/`vblank_disable_fn()` in workers with pre/post callbacks — is reasonable. However, several issues need attention:

**Issue 1: Unconditional workqueue allocation**

```c
dev->deferred_vblank_wq = alloc_workqueue("drm_vblank_deferred_wq",
                                          WQ_HIGHPRI | WQ_UNBOUND, 0);
if (!dev->deferred_vblank_wq)
    return -ENOMEM;
```

This is allocated for **every** DRM device in `drm_vblank_init()`, even though the vast majority of drivers will never use deferred vblank. Only devices implementing the `pre/post_enable/disable_vblank` callbacks need this workqueue. The allocation (and init of `enable_work`, `disable_work`, `enable_done` per-CRTC) should be deferred or conditional. One approach: allocate it lazily on first use, or check at CRTC init time if any CRTC needs it.

**Issue 2: Missing cancellation of pending deferred work in drm_crtc_vblank_off()**

The patched `drm_crtc_vblank_off()` calls `pre_disable_vblank` and `post_disable_vblank` directly, and calls `drm_vblank_disable_and_save()` under the spinlock. But it does **not** cancel any pending deferred `enable_work` or `disable_work`.

Consider this race:
1. `drm_vblank_get()` queues `enable_work` to the deferred workqueue
2. Before the worker runs, `drm_crtc_vblank_off()` is called (modeset)
3. `vblank_off` disables vblank and sets `inmodeset`
4. The queued `enable_work` now runs, takes `vbl_lock`, calls `drm_vblank_enable()` — which checks `!vblank->enabled`, sees it's false, and **re-enables vblank** after `vblank_off` has already shut everything down

This could lead to vblank interrupts firing on hardware that the driver expects to be quiesced. `drm_crtc_vblank_off()` should `cancel_work_sync(&vblank->enable_work)` and `cancel_delayed_work_sync(&vblank->disable_work)` (after dropping spinlocks, before or after the existing `drm_vblank_flush_worker()` call).

Similarly, `drm_crtc_vblank_on_config()` should cancel any pending deferred `disable_work` to avoid a stale disable racing with the new enable.

**Issue 3: Cosmetic extra parentheses**

```c
if ((!needs_deferred_enable && !vblank->enabled)) {
```

The outer parentheses are unnecessary.

**Issue 4: Comment style**

```c
/* Initialize in the completed state, the first deferred vblank
 * enable will reinitialize this. */
```

Kernel multi-line comment style puts `*/` on its own line:
```c
/*
 * Initialize in the completed state, the first deferred vblank
 * enable will reinitialize this.
 */
```

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amd/display: Refactor amdgpu_dm_crtc_set_vblank

[Claude Code Review Bot]

Patch Review

This is a reasonable cleanup separating the vblank workqueue management from the actual interrupt enable/disable logic.

**Observation:** The refactored `amdgpu_dm_crtc_set_vblank()` now uses `enable && crtc->enabled` as its guard:

```c
if (enable && crtc->enabled) {
    // do enable work
}
```

When `enable=true` but `crtc->enabled=false`, this silently returns 0 (success) without actually enabling anything. The caller `amdgpu_dm_crtc_enable_vblank()` checks `!crtc->enabled` and rejects early, so for the non-deferred path this is fine. But in patch 4, `amdgpu_dm_crtc_deferred_enable_vblank()` does **not** check `crtc->enabled` and calls `set_vblank(crtc, true)` directly. If the CRTC is disabled, `set_vblank` returns 0, and `active_vblank_irq_count` gets incremented without actually enabling the interrupt. The count will be balanced on disable, but the vblank core now believes the interrupt was successfully enabled when it wasn't.

**Also:** `amdgpu_dm_crtc_queue_vblank_work()` still uses `GFP_ATOMIC` for the allocation. Since the callers (`enable_vblank`/`disable_vblank` callbacks) can be called from atomic context in the non-deferred path this is needed. But for future clarity, a comment would help.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amd/display: Implement deferred vblanks on IPS platforms

[Claude Code Review Bot]

Patch Review

This is the driver-side implementation. The pre/post callbacks acquire/release `dc_lock` to bracket HW access. The design is sound in principle.

**Issue 1: `crtc->state` access from worker context**

In `amdgpu_dm_crtc_post_enable_vblank()` and `amdgpu_dm_crtc_post_disable_vblank()`:

```c
struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
```

These run from the deferred workqueue, meaning there's a window between when the work was queued (from `drm_vblank_get()`) and when it executes. During that window, an atomic commit could swap `crtc->state`. Accessing `crtc->state` without holding `drm_modeset_lock` from a worker is unsafe. The stream pointer could become stale or freed.

Consider either:
- Capturing the stream reference at queue time and passing it through to the worker, or
- Ensuring `dc_lock` serialization is sufficient to prevent state changes (it might be, since commits also take `dc_lock`, but this is fragile and should be documented)

**Issue 2: dc_lock held across drm_vblank_enable/disable internals**

`pre_enable_vblank` takes `dc_lock` and `post_enable_vblank` releases it. This means `dc_lock` is held while the DRM core calls `__enable_vblank` → `enable_vblank` callback → `amdgpu_dm_crtc_deferred_enable_vblank()`. That callback also does `amdgpu_dm_crtc_set_vblank()` which itself touches DC state. Since `dc_lock` is already held, this requires that all the code in the `set_vblank` path is safe for recursive `dc_lock` holding (mutex is not recursive by default). Verify that `dc_lock` isn't taken again inside `set_vblank` → interrupt enable path, or this will deadlock.

**Issue 3: Missing `crtc->enabled` check in deferred enable**

As noted in patch 3 review, `amdgpu_dm_crtc_deferred_enable_vblank()` doesn't check `crtc->enabled` before calling `set_vblank`. The `post_enable_vblank` callback does check `crtc->enabled` and bails early, but `active_vblank_irq_count` has already been incremented by that point.

**Issue 4: Duplicate vtable**

`amdgpu_dm_crtc_deferred_vblank_funcs` duplicates the entire `amdgpu_dm_crtc_funcs` structure with only 6 fields differing (enable/disable_vblank + 4 pre/post callbacks). If future changes are made to `amdgpu_dm_crtc_funcs`, the deferred variant must be kept in sync manually. Consider documenting this coupling prominently, or using a helper to populate shared fields.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: Add some debugging trace events

[Claude Code Review Bot]

Patch Review

Straightforward tracing additions. Useful for debugging the new deferred path.

**Minor issues:**

1. Format string mismatch in `drm_vblank_get_put_template`:
```c
TP_STRUCT__entry(
    __field(int, refcount)
),
TP_printk(
    "crtc=%d, refcount=%u",    // %u for signed int
```
`refcount` is declared as `int` but printed with `%u`. Use `%d` for consistency.

2. The `drm_vblank_put` trace point reads `atomic_read(&vblank->refcount)` **before** `atomic_dec_and_test()`:
```c
trace_drm_vblank_put(pipe, atomic_read(&vblank->refcount));

/* Last user schedules interrupt disable */
if (!atomic_dec_and_test(&vblank->refcount))
    return;
```
The comment "put's refcount not sync'd using vbl_lock, use for debugging purposes only" is appropriate, but the trace shows the refcount *before* the put, not after. This is fine for debugging but could confuse readers expecting the post-decrement value.

3. The `trace_drm_vblank_get` placement is before the `atomic_add_return`, so it also shows the pre-increment refcount, which is at least consistent with the put trace.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: Introduce deferred vblank enable/disable

[Claude Code Review Bot]

Patch Review

This is the core patch and has the most issues:

**1. Workqueue allocated unconditionally (wasteful)**

```c
dev->deferred_vblank_wq = alloc_workqueue("drm_vblank_deferred_wq",
                                          WQ_HIGHPRI | WQ_UNBOUND,
                                          0);
```

This allocates a workqueue for *every* DRM device regardless of whether any CRTC uses deferred vblank. This wastes resources for the vast majority of drivers. Consider lazily allocating it (e.g., in `drm_crtc_init_with_planes` when the funcs have deferred callbacks) or at least guarding it behind a check.

**2. Race between deferred enable and drm_crtc_vblank_off()**

`drm_vblank_get()` queues the enable worker, then releases `vbl_lock`. If `drm_crtc_vblank_off()` runs before the worker executes, the worker will call `drm_vblank_enable()` → `crtc->funcs->enable_vblank()` on hardware that's being torn down. The worker does:

```c
spin_lock_irqsave(&dev->vbl_lock, irqflags);
ret = drm_vblank_enable(dev, vblank->pipe);
```

But `drm_crtc_vblank_off()` sets `vblank->inmodeset` under `vbl_lock`, which would make `drm_vblank_enable()` fail with `-EINVAL` (because the refcount was already decremented by `vblank_off`). However, the `pre_enable_vblank()` callback runs *before* this lock is taken, so the driver's blocking pre-work could still execute against torn-down hardware. Consider cancelling pending enable work in `drm_crtc_vblank_off()` before proceeding, or checking `vblank->inmodeset` inside the worker before calling the pre-hook.

**3. Race between deferred disable and deferred enable**

If a deferred disable is pending (delayed work) and a new `drm_vblank_get()` triggers a deferred enable, the pending disable isn't cancelled. The `queue_work()` for enable is non-delayed, so it may execute first, but there's no explicit cancellation of the pending `disable_work`. After enable completes, the stale disable could fire. The regular (non-deferred) path handles this via the `disable_timer` being re-armed, but the deferred path should cancel `disable_work` when queueing `enable_work`.

**4. drm_wait_vblank_ioctl ordering bug**

```c
ret = drm_vblank_get(dev, pipe);
drm_crtc_vblank_wait_deferred_enable(drm_crtc_from_index(dev, pipe));

if (ret) {
```

The `drm_crtc_vblank_wait_deferred_enable()` is called *before* checking `ret`. If `drm_vblank_get()` returned an error, this waits on a potentially stale/completed completion. It should be inside the success path:

```c
ret = drm_vblank_get(dev, pipe);
if (ret) {
    ...error handling...
}
drm_crtc_vblank_wait_deferred_enable(drm_crtc_from_index(dev, pipe));
```

**5. Missing `cancel_work_sync` / `cancel_delayed_work_sync` in cleanup paths**

In `drm_dev_init_release()`, only `flush_workqueue` + `destroy_workqueue` is done. But `drm_vblank_init_release()` (per-CRTC cleanup via `timer_delete_sync`) doesn't cancel the per-CRTC `enable_work` and `disable_work`. These should be cancelled in `drm_vblank_init_release()`:

```c
cancel_work_sync(&vblank->enable_work);
cancel_delayed_work_sync(&vblank->disable_work);
```

**6. `try_wait_for_completion` used outside lock for racy check**

```c
bool deferred_enable_pending =
    needs_deferred_enable &&
    !try_wait_for_completion(&vblank->enable_done);
```

`try_wait_for_completion()` actually *consumes* the completion if it's done. Inside `drm_vblank_get()` under `vbl_lock`, this consumption could cause a subsequent `wait_for_completion_timeout` (from `drm_crtc_vblank_wait_deferred_enable`) to block even though enable already completed. Using `completion_done()` instead would be non-destructive.

**7. `WQ_UNBOUND` with `max_active=0`**

The workqueue is created with `WQ_UNBOUND` and `max_active=0` (unlimited). The commit message says "single-threaded worker" but the workqueue is not single-threaded. Enable and disable workers for different CRTCs (or even the same CRTC) could run concurrently. If single-threaded semantics are intended, use `alloc_ordered_workqueue()`.

---
Generated by Claude Code Patch Reviewer