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

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

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

[sunpeng.li]

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

See cover letter from v1 for motivation and details:
https://lore.kernel.org/dri-devel/20260224212639.390768-1-sunpeng.li@amd.com/

v2 contains fixes to some failing igt@kms_vblank tests, namely
* any test that calls wait_for_vblank()
* ts_continuation_dpms/modeset tests

Fixes are all in patch 2/5, the other patches remain unchanged.

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                  | 239 +++++++++++++++-
 include/drm/drm_crtc.h                        |  34 +++
 include/drm/drm_device.h                      |   6 +
 include/drm/drm_vblank.h                      |  20 ++
 11 files changed, 654 insertions(+), 49 deletions(-)

-- 
2.53.0

[PATCH v2 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.53.0

[PATCH v2 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.

v2: Fix failing igt@kms_vblank tests:
* Fix wait_vblank_ioctl() skipping on deferred enable (because it skips
  on !vblank->enabled) by calling drm_crtc_vblank_wait_deferred_enable()
  (fixes all subtests that call wait_for_vblank()).
* Fix drm_vblank_get() not returning -EINVAL on deferred enable when
  drm_crtc_vblank_off() was called (fixes ts-continuation-dpms/modeset).

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_drv.c    |   5 +
 drivers/gpu/drm/drm_vblank.c | 200 +++++++++++++++++++++++++++++++++--
 include/drm/drm_crtc.h       |  34 ++++++
 include/drm/drm_device.h     |   6 ++
 include/drm/drm_vblank.h     |  20 ++++
 5 files changed, 254 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..4b73ccde35425 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,17 @@ 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 +618,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 +1254,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 +1344,30 @@ 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 deferred enable is not pending, and vblank is disabled,
+		 * while there's an active requestor (refcount > 1), then crtc
+		 * is turned off. Decrement refcount, and error.
+		 */
+		bool deferred_enable_pending =
+			needs_deferred_enable &&
+			!try_wait_for_completion(&vblank->enable_done);
+
+		if ((!deferred_enable_pending && !vblank->enabled)) {
 			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
 		}
@@ -1255,6 +1397,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 +1405,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 +1506,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 +1555,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 +1653,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 +1677,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);
 
@@ -1797,6 +1967,8 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	}
 
 	ret = drm_vblank_get(dev, pipe);
+	drm_crtc_vblank_wait_deferred_enable(drm_crtc_from_index(dev, pipe));
+
 	if (ret) {
 		drm_dbg_core(dev,
 			     "crtc %d failed to acquire vblank counter, %d\n",
@@ -1962,8 +2134,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 312fc1e745d22..dc45400c18094 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -904,6 +904,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.53.0

[PATCH v2 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    | 87 +++++++++++++------
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.h    |  4 -
 2 files changed, 59 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 304437c2284d8..e6727d5098863 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,78 @@ 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.53.0

[PATCH v2 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    | 178 +++++++++++++++++-
 2 files changed, 185 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 fc5e0bf121d22..76cd50036db92 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10208,6 +10208,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 e6727d5098863..6ea99ad9c4b36 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
@@ -453,6 +453,141 @@ 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)
 {
@@ -622,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)
 {
 }
@@ -754,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;
@@ -770,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.53.0

[PATCH v2 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 4b73ccde35425..3a010ee1a6596 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1266,6 +1266,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);
 
@@ -1295,6 +1297,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);
 
@@ -1324,10 +1328,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);
 
@@ -1348,11 +1356,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);
@@ -1408,6 +1420,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;
@@ -1415,18 +1429,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));
 	}
@@ -1518,6 +1535,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(). */
@@ -1659,6 +1678,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;
 
@@ -2135,11 +2156,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.53.0

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

[Mario Limonciello]

On 3/23/26 15:27, sunpeng.li@amd.com wrote:
> 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.
> 
I have two trivial nits below, you can include my tag with next version 
or just fix them when committing if everything else turns out reviewed OK.

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> 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.

nit1: you should be consistent with case of true/False
nit2: vblank not vblanks

> + */
> +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);

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-03-25T07:22:01.297418

---

This series introduces deferred vblank enable/disable for DRM, motivated by AMD's IPS (Idle Power States) feature where the DCN block can be power-gated and re-enabling requires acquiring a mutex (blocking operation) that cannot be done from atomic context. The approach defers `drm_vblank_enable()` and `vblank_disable_fn()` to a workqueue, with pre/post hooks for drivers to do blocking work.

The design is reasonable for the stated problem, but there are several concurrency concerns in patch 2 that need attention — particularly around races between deferred enable, deferred disable, and `drm_crtc_vblank_off()`. The series also unconditionally allocates a workqueue for all DRM devices even when no driver uses deferred vblank.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

Clean, straightforward patch. The replacement in `drm_atomic_helper_commit_crtc_disable` removes the side-effectful `drm_crtc_vblank_get()`/`drm_crtc_vblank_put()` pair that was only being used to check if vblank_off had been called. Good preparation for patch 2.

Minor nit: the function is exported (`EXPORT_SYMBOL`) but placed in `drm_internal.h`. That combination is unusual — it means out-of-tree modules could call it but in-tree headers don't expose it. Since it's only used internally, maybe just don't export it, or use `EXPORT_SYMBOL_GPL` if future driver use is anticipated.

The condition `vblank->inmodeset && !vblank->enabled` correctly captures the "vblank_off was called" state, which sets `inmodeset` and disables vblank.

---
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

Claude review: drm/amd/display: Refactor amdgpu_dm_crtc_set_vblank

[Claude Code Review Bot]

Patch Review

Clean refactoring. The split is logical: `amdgpu_dm_crtc_set_vblank()` does the HW interrupt work, `amdgpu_dm_crtc_queue_vblank_work()` handles the PSR/panel optimization work, and the enable/disable wrappers tie them together.

One minor issue: in `amdgpu_dm_crtc_set_vblank()`, the condition changed from `if (enable)` to `if (enable && crtc->enabled)`, but the `crtc->enabled` check was moved to `amdgpu_dm_crtc_enable_vblank()` which returns `-EINVAL` early. So in the non-deferred path, `amdgpu_dm_crtc_set_vblank()` should never be called with `enable=true` and `!crtc->enabled`. But patch 4 calls `amdgpu_dm_crtc_set_vblank(crtc, true)` from `amdgpu_dm_crtc_deferred_enable_vblank()` *without* the `crtc->enabled` guard. That means the `enable && crtc->enabled` check inside `set_vblank` becomes the only guard for the deferred path, which is fine — but it means a deferred enable on an unconfigured CRTC silently succeeds (returns 0) without actually enabling anything. That seems fragile; it might be better to return an error.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**1. `active_vblank_irq_count` is not protected**

```c
dm->active_vblank_irq_count++;
```

This counter is incremented/decremented in the deferred callbacks, which run in workqueue context. Without any lock protecting it, concurrent enable/disable on different CRTCs could race. The deferred workqueue isn't ordered (see patch 2 review), so these can run in parallel.

**2. `dc_lock` held across pre→enable→post boundary is asymmetric**

`pre_enable_vblank` acquires `dc_lock`, and `post_enable_vblank` releases it. This means the DRM vblank core's `drm_vblank_enable()` (called between pre and post) runs with `dc_lock` held. This works for the IPS use case, but it means the `enable_vblank` driver callback also runs with `dc_lock` held — which is the intent, but it couples the DRM core's locking guarantees to driver-internal lock ordering. If any code path under `drm_vblank_enable()` ever tries to acquire a lock that `dc_lock` nests inside, it would deadlock.

**3. Accessing `crtc->state` without proper locking**

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

In `post_enable_vblank` and `post_disable_vblank`, `crtc->state` is accessed from workqueue context. The `crtc->state` pointer can change during atomic commits. This needs either atomic state locking or the driver needs to ensure the state won't change while the worker runs (e.g., by holding a reference or ensuring commit serialization). This is a pre-existing pattern in the AMDGPU driver's vblank control worker, but deferring it to the DRM core's workqueue may change timing assumptions.

**4. `vblank_control_work` on the stack**

```c
struct vblank_control_work vblank_work = { 0 };
```

Using a stack-allocated `vblank_control_work` and calling `amdgpu_dm_crtc_set_panel_sr_feature()` with it seems fine as long as that function doesn't store a pointer to it. Worth verifying.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

Reasonable debugging addition. A few observations:

- The `drm_vblank_put` trace comment notes `refcount not sync'd using vbl_lock` — good callout. The trace shows the refcount *before* the decrement, which is slightly misleading for a "put" event. Consider tracing after the dec, or documenting that it shows the pre-decrement value.

- The trace events in `drm_vblank_get` are placed *after* acquiring `vbl_lock` but *before* the `atomic_add_return`, so they show pre-increment refcount, which is correct for showing the state at entry.

- Style nit: some `if/else` blocks now have braces on one arm but not the other (e.g., lines around `vblank_offdelay < 0`). The kernel style guide says if one branch has braces, both should.

---
Generated by Claude Code Patch Reviewer