[PATCH 1/5] drm/client: Make the FBIO_WAITFORVSYNC vblank wait interruptible

[PATCH 1/5] drm/client: Make the FBIO_WAITFORVSYNC vblank wait interruptible

[Prike Liang]

FBIO_WAITFORVSYNC calls drm_client_modeset_wait_for_vblank(), which
waits with drm_crtc_wait_one_vblank(). That wait is uninterruptible and
only gives up after one second. If no vblank interrupt arrives, for
example while the driver resets the display hardware, the caller sleeps
in D state for the whole second and can't be interrupted, not even by
SIGKILL.

DRM_IOCTL_WAIT_VBLANK waits interruptibly, and so do fbdev drivers that
implement FBIO_WAITFORVSYNC themselves, such as atyfb and matroxfb.

Add drm_crtc_wait_one_vblank_interruptible(), a DRM-internal variant of
drm_crtc_wait_one_vblank() that a signal can interrupt, and use it in
drm_client_modeset_wait_for_vblank(). Return -ERESTARTSYS when a signal
interrupts the wait, so that the ioctl returns to userspace or is
restarted. Timeouts and disabled CRTCs are still not reported.

The fbdev damage worker also calls drm_client_modeset_wait_for_vblank(),
but it runs in a kernel thread, which doesn't receive signals, so its
wait doesn't change. Drivers keep using drm_crtc_wait_one_vblank().

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Assisted-by: Copilot:claude-opus-5.5
---
 drivers/gpu/drm/drm_client_modeset.c | 10 +++--
 drivers/gpu/drm/drm_internal.h       |  1 +
 drivers/gpu/drm/drm_vblank.c         | 60 ++++++++++++++++++++--------
 3 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 0080a8e9556a..b475b8dbdb17 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -1301,10 +1301,11 @@ EXPORT_SYMBOL(drm_client_modeset_dpms);
  *
  * Block the caller until the given CRTC has seen a VBLANK. Do nothing
  * if the CRTC is disabled. If there's another DRM master present, fail
- * with -EBUSY.
+ * with -EBUSY. A signal interrupts the wait.
  *
  * Returns:
- * 0 on success, or negative error code otherwise.
+ * 0 on success, -ERESTARTSYS if a signal interrupted the wait, or
+ * negative error code otherwise.
  */
 int drm_client_modeset_wait_for_vblank(struct drm_client_dev *client, unsigned int crtc_index)
 {
@@ -1328,13 +1329,14 @@ int drm_client_modeset_wait_for_vblank(struct drm_client_dev *client, unsigned i
 	 */
 	ret = drm_crtc_vblank_get(crtc);
 	if (!ret) {
-		drm_crtc_wait_one_vblank(crtc);
+		ret = drm_crtc_wait_one_vblank_interruptible(crtc);
 		drm_crtc_vblank_put(crtc);
 	}
 
 	drm_master_internal_release(dev);
 
-	return 0;
+	/* Only report interruption by a signal, not timeouts. */
+	return ret == -ERESTARTSYS ? ret : 0;
 }
 EXPORT_SYMBOL(drm_client_modeset_wait_for_vblank);
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f893b1e3a596..2be21c20eb78 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -115,6 +115,7 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe);
 int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
 void drm_vblank_put(struct drm_device *dev, unsigned int pipe);
 u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe);
+int drm_crtc_wait_one_vblank_interruptible(struct drm_crtc *crtc);
 
 /* drm_vblank_work.c */
 static inline void drm_vblank_flush_worker(struct drm_vblank_crtc *vblank)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e42..c3c9afc08ed2 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1297,21 +1297,12 @@ void drm_crtc_vblank_put(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_put);
 
-/**
- * drm_crtc_wait_one_vblank - wait for one vblank
- * @crtc: DRM crtc
- *
- * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
- * It is a failure to call this when the vblank irq for @crtc is disabled, e.g.
- * due to lack of driver support or because the crtc is off.
- *
- * Returns: 0 on success, negative error on failures.
- */
-int drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
+static int __drm_crtc_wait_one_vblank(struct drm_crtc *crtc, bool interruptible)
 {
 	struct drm_device *dev = crtc->dev;
 	int pipe = drm_crtc_index(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+	long timeout = msecs_to_jiffies(1000);
 	int ret;
 	u64 last;
 
@@ -1322,18 +1313,55 @@ int drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
 
 	last = drm_vblank_count(dev, pipe);
 
-	ret = wait_event_timeout(vblank->queue,
-				 last != drm_vblank_count(dev, pipe),
-				 msecs_to_jiffies(1000));
+	if (interruptible)
+		timeout = wait_event_interruptible_timeout(vblank->queue,
+							   last != drm_vblank_count(dev, pipe),
+							   timeout);
+	else
+		timeout = wait_event_timeout(vblank->queue,
+					     last != drm_vblank_count(dev, pipe),
+					     timeout);
 
-	drm_WARN(dev, ret == 0, "vblank wait timed out on crtc %i\n", pipe);
+	drm_WARN(dev, timeout == 0, "vblank wait timed out on crtc %i\n", pipe);
 
 	drm_vblank_put(dev, pipe);
 
-	return ret ? 0 : -ETIMEDOUT;
+	if (timeout < 0)
+		return timeout;
+
+	return timeout ? 0 : -ETIMEDOUT;
+}
+
+/**
+ * drm_crtc_wait_one_vblank - wait for one vblank
+ * @crtc: DRM crtc
+ *
+ * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
+ * It is a failure to call this when the vblank irq for @crtc is disabled, e.g.
+ * due to lack of driver support or because the crtc is off.
+ *
+ * Returns: 0 on success, negative error on failures.
+ */
+int drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
+{
+	return __drm_crtc_wait_one_vblank(crtc, false);
 }
 EXPORT_SYMBOL(drm_crtc_wait_one_vblank);
 
+/**
+ * drm_crtc_wait_one_vblank_interruptible - wait for one vblank, interruptibly
+ * @crtc: DRM crtc
+ *
+ * Like drm_crtc_wait_one_vblank(), but a signal interrupts the wait.
+ *
+ * Returns: 0 on success, -ERESTARTSYS if a signal interrupted the wait,
+ * negative error on other failures.
+ */
+int drm_crtc_wait_one_vblank_interruptible(struct drm_crtc *crtc)
+{
+	return __drm_crtc_wait_one_vblank(crtc, true);
+}
+
 /**
  * drm_crtc_vblank_off - disable vblank events on a CRTC
  * @crtc: CRTC in question
-- 
2.34.1

[PATCH 2/5] drm/vblank: Stop drm_crtc_wait_one_vblank() when vblank is turned off

[Prike Liang]

drm_crtc_vblank_off() turns off the vblank interrupt even while someone
holds a vblank reference and waits for a vblank. It then wakes up the
vblank wait queue. DRM_IOCTL_WAIT_VBLANK stops waiting at this point,
because its wait condition also checks whether vblank is still enabled.

drm_crtc_wait_one_vblank() and drm_crtc_wait_one_vblank_interruptible()
only check whether the vblank count has changed. Once vblank is off, the
count doesn't change, so the woken waiter sleeps again until the wait
times out after one second and warns "vblank wait timed out". This
happens, for example, when the fbdev damage worker or FBIO_WAITFORVSYNC
waits for a vblank while the driver turns the CRTC off for suspend or a
GPU reset.

Make __drm_crtc_wait_one_vblank(), which implements both functions, also
stop waiting when vblank is turned off, and return -EINVAL if no vblank
passed, the same error that drm_vblank_get() returns when vblank is off.
The wait condition is checked before going to sleep, so this also covers
vblank being turned off between drm_vblank_get() and the wait. Waits
that see a vblank, time out or are interrupted behave as before.

Fixes: e8450f51a4b3 ("drm/irq: Implement a generic vblank_wait function")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Assisted-by: Copilot:claude-opus-5.5
---
 drivers/gpu/drm/drm_vblank.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index c3c9afc08ed2..5e247663978a 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1315,21 +1315,29 @@ static int __drm_crtc_wait_one_vblank(struct drm_crtc *crtc, bool interruptible)
 
 	if (interruptible)
 		timeout = wait_event_interruptible_timeout(vblank->queue,
-							   last != drm_vblank_count(dev, pipe),
+							   last != drm_vblank_count(dev, pipe) ||
+							   !READ_ONCE(vblank->enabled),
 							   timeout);
 	else
 		timeout = wait_event_timeout(vblank->queue,
-					     last != drm_vblank_count(dev, pipe),
+					     last != drm_vblank_count(dev, pipe) ||
+					     !READ_ONCE(vblank->enabled),
 					     timeout);
 
 	drm_WARN(dev, timeout == 0, "vblank wait timed out on crtc %i\n", pipe);
 
-	drm_vblank_put(dev, pipe);
-
 	if (timeout < 0)
-		return timeout;
+		ret = timeout;
+	else if (!timeout)
+		ret = -ETIMEDOUT;
+	else if (last == drm_vblank_count(dev, pipe))
+		ret = -EINVAL; /* drm_crtc_vblank_off() ended the wait */
+	else
+		ret = 0;
 
-	return timeout ? 0 : -ETIMEDOUT;
+	drm_vblank_put(dev, pipe);
+
+	return ret;
 }
 
 /**
@@ -1338,7 +1346,8 @@ static int __drm_crtc_wait_one_vblank(struct drm_crtc *crtc, bool interruptible)
  *
  * This waits for one vblank to pass on @crtc, using the irq driver interfaces.
  * It is a failure to call this when the vblank irq for @crtc is disabled, e.g.
- * due to lack of driver support or because the crtc is off.
+ * due to lack of driver support or because the crtc is off. The wait also fails
+ * if drm_crtc_vblank_off() disables the vblank irq before a vblank passes.
  *
  * Returns: 0 on success, negative error on failures.
  */
-- 
2.34.1

Re: [PATCH 2/5] drm/vblank: Stop drm_crtc_wait_one_vblank() when vblank is turned off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/vblank: missing Signed-off-by trailer in commit message
--

commit 7617b69ad08b75966bdfac2d98682c551d654593
Author: Prike Liang <Prike.Liang@amd.com>

> drm/vblank: Stop drm_crtc_wait_one_vblank() when vblank is turned off
>
> drm_crtc_vblank_off() turns off the vblank interrupt even while someone
> holds a vblank reference and waits for a vblank. It then wakes up the
> vblank wait queue. DRM_IOCTL_WAIT_VBLANK stops waiting at this point,
> because its wait condition also checks whether vblank is still enabled.
>
> drm_crtc_wait_one_vblank() and drm_crtc_wait_one_vblank_interruptible()
> only check whether the vblank count has changed. Once vblank is off, the
> count doesn't change, so the woken waiter sleeps again until the wait
> times out after one second and warns "vblank wait timed out". This
> happens, for example, when the fbdev damage worker or FBIO_WAITFORVSYNC
> waits for a vblank while the driver turns the CRTC off for suspend or a
> GPU reset.
>
> Make __drm_crtc_wait_one_vblank(), which implements both functions, also
> stop waiting when vblank is turned off, and return -EINVAL if no vblank
> passed, the same error that drm_vblank_get() returns when vblank is off.
> The wait condition is checked before going to sleep, so this also covers
> vblank being turned off between drm_vblank_get() and the wait. Waits
> that see a vblank, time out or are interrupted behave as before.
>
> Fixes: e8450f51a4b3 ("drm/irq: Implement a generic vblank_wait function")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
This commit message is missing a Signed-off-by: trailer from the patch
author.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=2

[PATCH 3/5] drm/amd/display: send the flip event right away if vblank is off

[Prike Liang]

dm_arm_vblank_event_pre_programming() takes a vblank reference for a
page flip or cursor update that carries an event, and the handler that
later sends the event drops that reference again. The return value of
drm_crtc_vblank_get() is ignored, though. If vblank is off, the event
is armed anyway, and the handler that eventually sends it drops a
reference that was never taken, which unbalances the vblank refcount.

That can't happen today: DM only turns vblank off for a CRTC that it
disables, and a disabled CRTC has no active planes. The next patch also
turns vblank off while a GPU reset has the streams stopped, though.
Page flips that don't modify the DC state skip
amdgpu_dm_commit_streams() and only take dc_lock after this point, so
they still get here while the reset holds it.

Send the event right away when no vblank reference can be taken, like
drm_crtc_vblank_off() does for queued vblank events. No vblank interrupt
could deliver it in that case anyway.

Add a KUnit test for it.

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Assisted-by: Copilot:claude-opus-5.5
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++++--
 .../display/amdgpu_dm/tests/amdgpu_dm_test.c  | 29 +++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

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 33c73ca9cff1..d3882ffcb5d4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3825,6 +3825,11 @@ EXPORT_IF_KUNIT(dm_arm_vblank_event);
  * Grab a reference on the vblank counter if a page flip or cursor update is to
  * be programmed. Do this before programming so the HW is not in any
  * idle-optimized state (such as PSR).
+ *
+ * If vblank is off, no vblank interrupt can deliver the event, and the
+ * handlers that send it would drop a reference that was never taken. Send
+ * the event right away instead, like drm_crtc_vblank_off() does for queued
+ * vblank events.
  */
 STATIC_IFN_KUNIT void dm_arm_vblank_event_pre_programming(struct amdgpu_crtc *acrtc,
 							  struct dm_crtc_state *acrtc_state,
@@ -3836,8 +3841,14 @@ STATIC_IFN_KUNIT void dm_arm_vblank_event_pre_programming(struct amdgpu_crtc *ac
 	if (!acrtc->base.state->event || acrtc_state->active_planes == 0)
 		return;
 
-	if (pflip_update || cursor_update)
-		drm_crtc_vblank_get(&acrtc->base);
+	if (!pflip_update && !cursor_update)
+		return;
+
+	if (drm_crtc_vblank_get(&acrtc->base)) {
+		drm_crtc_send_vblank_event(&acrtc->base,
+					   acrtc->base.state->event);
+		acrtc->base.state->event = NULL;
+	}
 }
 EXPORT_IF_KUNIT(dm_arm_vblank_event_pre_programming);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
index b9272b28f53c..ebec1260c510 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
@@ -1812,6 +1812,34 @@ static void dm_test_arm_vblank_pre_programming_update(struct kunit *test)
 	drm_crtc_vblank_put(&ctx->acrtc->base);
 }
 
+/**
+ * dm_test_arm_vblank_pre_programming_vblank_off - Test the event is sent with vblank off
+ * @test: The KUnit test context
+ *
+ * No vblank interrupt can deliver the event, and the handlers that send it
+ * must not drop a vblank reference that was never taken.
+ */
+static void dm_test_arm_vblank_pre_programming_vblank_off(struct kunit *test)
+{
+	struct dm_test_vblank_ctx *ctx = dm_test_vblank_ctx_alloc(test);
+	struct drm_vblank_crtc *vblank;
+
+	KUNIT_ASSERT_EQ(test, drm_vblank_init(&ctx->adev->ddev, 1), 0);
+	vblank = drm_crtc_vblank_crtc(&ctx->acrtc->base);
+	drm_crtc_vblank_off(&ctx->acrtc->base);
+
+	/* Sending an event that has no file frees it. */
+	ctx->acrtc_state->base.event = kzalloc_obj(*ctx->event, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx->acrtc_state->base.event);
+
+	dm_test_arm_vblank_pre_programming(ctx, true, false);
+
+	KUNIT_EXPECT_NULL(test, ctx->acrtc_state->base.event);
+	/* Only drm_crtc_vblank_off() holds a reference. */
+	KUNIT_EXPECT_EQ(test, atomic_read(&vblank->refcount), 1);
+	kfree(ctx->acrtc_state->base.event);
+}
+
 /**
  * dm_test_arm_vblank_event_no_event - Test a commit without an event is a no-op
  * @test: The KUnit test context
@@ -5298,6 +5326,7 @@ static struct kunit_case amdgpu_dm_tests[] = {
 	KUNIT_CASE(dm_test_arm_vblank_pre_programming_no_event),
 	KUNIT_CASE(dm_test_arm_vblank_pre_programming_no_planes),
 	KUNIT_CASE(dm_test_arm_vblank_pre_programming_update),
+	KUNIT_CASE(dm_test_arm_vblank_pre_programming_vblank_off),
 	/* dm_update_pflip_irq_state */
 	KUNIT_CASE(dm_test_update_pflip_irq_state_dcn),
 	KUNIT_CASE(dm_test_update_pflip_irq_state_dce),
-- 
2.34.1

Re: [PATCH 3/5] drm/amd/display: send the flip event right away if vblank is off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amd/display: missing Signed-off-by trailer in commit
  message
--

commit 26c603fc4a2cad18ac38e5e382079d6b467933c0
Author: Prike Liang <Prike.Liang@amd.com>

> drm/amd/display: send the flip event right away if vblank is off
>
> dm_arm_vblank_event_pre_programming() takes a vblank reference for a
> page flip or cursor update that carries an event, and the handler that
> later sends the event drops that reference again. The return value of
> drm_crtc_vblank_get() is ignored, though. If vblank is off, the event
> is armed anyway, and the handler that eventually sends it drops a
> reference that was never taken, which unbalances the vblank refcount.
>
> That can't happen today: DM only turns vblank off for a CRTC that it
> disables, and a disabled CRTC has no active planes. The next patch also
> turns vblank off while a GPU reset has the streams stopped, though.
> Page flips that don't modify the DC state skip
> amdgpu_dm_commit_streams() and only take dc_lock after this point, so
> they still get here while the reset holds it.
>
> Send the event right away when no vblank reference can be taken, like
> drm_crtc_vblank_off() does for queued vblank events. No vblank interrupt
> could deliver it in that case anyway.
>
> Add a KUnit test for it.
>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
This commit message is missing a Signed-off-by trailer from the patch
author.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=3

[PATCH 4/5] drm/amd/display: turn vblank off in DRM across GPU reset

[Prike Liang]

For a GPU reset, dm_suspend() stops all streams with
amdgpu_dm_commit_zero_streams() and toggles the OTG interrupts
directly, and dm_resume() restores the cached DC state the same way.
Neither goes through manage_dm_interrupts(), so DRM is never told that
vblank stops: drm_crtc_vblank_get() keeps succeeding while no vblank
interrupt can arrive until the streams are back.

Since commit ebc3351cc4a9 ("drm/amdgpu: remove suspend/resume DRM
clients during GPU recovery"), the fbdev client keeps running during
recovery, and its damage worker rate-limits display updates with
drm_client_modeset_wait_for_vblank(). Every display update during the
reset then waits for a vblank that never comes, until the wait times
out and warns:

  amdgpu 0000:03:00.0: [drm] vblank wait timed out on crtc 0
  WARNING: drivers/gpu/drm/drm_vblank.c:1320 at drm_crtc_wait_one_vblank+0x194/0x200
  Workqueue: events drm_fb_helper_damage_work
  Call Trace:
   drm_client_modeset_wait_for_vblank+0x63/0x80
   drm_fb_helper_damage_work+0x89/0x1a0

FBIO_WAITFORVSYNC runs into the same timeout, and DRM_IOCTL_WAIT_VBLANK
sleeps until its own 3 second timeout.

Do what a regular CRTC disable does: call drm_crtc_vblank_off() for the
CRTCs of the cached DC state before their streams are stopped, and
drm_crtc_vblank_on_config() with their existing configuration once the
streams and interrupts are restored. drm_crtc_vblank_get() then fails
for the duration of the reset, so drm_client_modeset_wait_for_vblank()
doesn't wait at all, and DRM_IOCTL_WAIT_VBLANK waiters are woken up.
Page flips that get to amdgpu_dm_commit_planes() meanwhile have their
event sent right away, see the previous patch.

drm_crtc_vblank_off() also clears the vblank timestamping constants,
which a modeset sets again before it turns vblank back on. Recompute
them from the CRTC state after the reset as well; otherwise the vblank
timestamp query in drm_crtc_vblank_on_config() warns about an
uninitialized mode.

A modeset that waits for dc_lock during the reset may be turning one of
these CRTCs off. Leave vblank off for a CRTC that is no longer enabled;
amdgpu_dm_crtc_set_vblank() would refuse to enable vblank on it anyway.

Neither call can block on the reset: drm_crtc_vblank_off() only takes
spinlocks and flushes the DRM vblank worker, which DM doesn't use, and
amdgpu_dm_crtc_set_vblank() doesn't queue vblank_control_work during a
reset. Unlike suspending the DRM clients, this doesn't make recovery
wait for anything that may itself depend on the reset.

Add KUnit tests for the new helper.

Fixes: ebc3351cc4a9 ("drm/amdgpu: remove suspend/resume DRM clients during GPU recovery")
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Assisted-by: Copilot:claude-opus-5.5
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  52 ++++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 .../display/amdgpu_dm/tests/amdgpu_dm_test.c  | 112 ++++++++++++++++++
 3 files changed, 165 insertions(+)

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 d3882ffcb5d4..007968e892c8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1496,6 +1496,56 @@ STATIC_IFN_KUNIT void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
 }
 EXPORT_IF_KUNIT(dm_gpureset_toggle_interrupts);
 
+/**
+ * dm_gpureset_toggle_vblank() - Turn DRM vblank off or on around a GPU reset
+ * @adev: amdgpu device
+ * @state: DC state whose streams the reset stops and restores
+ * @enable: false before the streams are stopped, true once they are restored
+ *
+ * The GPU reset path stops and restores the streams through DC directly and
+ * bypasses manage_dm_interrupts(). Turn vblank off in DRM like a CRTC
+ * disable does, so that drm_crtc_vblank_get() fails while no vblank
+ * interrupt can arrive, instead of letting vblank waiters such as the fbdev
+ * damage worker sleep until they time out.
+ */
+STATIC_IFN_KUNIT void dm_gpureset_toggle_vblank(struct amdgpu_device *adev,
+						struct dc_state *state, bool enable)
+{
+	struct drm_vblank_crtc_config config;
+	struct amdgpu_crtc *acrtc;
+	int i;
+
+	for (i = 0; i < state->stream_count; i++) {
+		int otg_inst = state->stream_status[i].primary_otg_inst;
+
+		acrtc = amdgpu_dm_get_crtc_by_otg_inst(adev, otg_inst);
+		if (!acrtc)
+			continue;
+
+		if (!enable) {
+			drm_crtc_vblank_off(&acrtc->base);
+			continue;
+		}
+
+		/*
+		 * A modeset waiting for dc_lock may be turning the CRTC off,
+		 * and amdgpu_dm_crtc_set_vblank() refuses to enable vblank on
+		 * it then. Leave its vblank off.
+		 */
+		if (!acrtc->base.enabled)
+			continue;
+
+		/* drm_crtc_vblank_off() cleared the timestamping constants. */
+		drm_calc_timestamping_constants(&acrtc->base,
+						&acrtc->base.state->adjusted_mode);
+
+		/* Keep the off delay that manage_dm_interrupts() configured. */
+		config = drm_crtc_vblank_crtc(&acrtc->base)->config;
+		drm_crtc_vblank_on_config(&acrtc->base, &config);
+	}
+}
+EXPORT_IF_KUNIT(dm_gpureset_toggle_vblank);
+
 DEFINE_FREE(state_release, struct dc_state *, dc_state_release(_T))
 
 STATIC_IFN_KUNIT enum dc_status amdgpu_dm_commit_zero_streams(struct dc *dc)
@@ -1630,6 +1680,7 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)
 			return -ENOMEM;
 		}
 
+		dm_gpureset_toggle_vblank(adev, dm->cached_dc_state, false);
 		dm_gpureset_toggle_interrupts(adev, dm->cached_dc_state, false);
 
 		res = amdgpu_dm_commit_zero_streams(dm->dc);
@@ -1969,6 +2020,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
 		dm_gpureset_commit_state(dm->cached_dc_state, dm);
 
 		dm_gpureset_toggle_interrupts(adev, dm->cached_dc_state, true);
+		dm_gpureset_toggle_vblank(adev, dm->cached_dc_state, true);
 
 		dc_state_release(dm->cached_dc_state);
 		dm->cached_dc_state = NULL;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f8503ebf28f4..303e9a57e7b8 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -1351,6 +1351,7 @@ void mmhub_read_system_context(struct amdgpu_device *adev,
 			       struct dc_phy_addr_space_config *pa_config);
 int amdgpu_dm_init_power_module(struct amdgpu_display_manager *dm);
 void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev, struct dc_state *state, bool enable);
+void dm_gpureset_toggle_vblank(struct amdgpu_device *adev, struct dc_state *state, bool enable);
 enum dc_status amdgpu_dm_commit_zero_streams(struct dc *dc);
 int dm_cache_state(struct amdgpu_device *adev);
 void dm_destroy_cached_state(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
index ebec1260c510..ed2e65bad674 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
@@ -5232,6 +5232,113 @@ static void dm_test_gpureset_toggle_interrupts_dcn(struct kunit *test)
 	dm_gpureset_toggle_interrupts(adev, state, true);
 }
 
+/* Tests for dm_gpureset_toggle_vblank() */
+
+/*
+ * One stream on the OTG of the CRTC from dm_test_irq_mgmt_ctx_alloc(), which
+ * is enabled with a 1080p60 mode and has its vblank set up like on a regular
+ * CRTC enable: timestamping constants first, then manage_dm_interrupts().
+ */
+static struct dc_state *dm_test_gpureset_vblank_state(struct kunit *test,
+						      struct dm_test_irq_mgmt_ctx *ctx)
+{
+	struct drm_display_mode *mode = &ctx->acrtc_state->base.adjusted_mode;
+	struct dc_state *state = dm_kunit_alloc_dc_state(test);
+
+	mode->crtc_clock = 148500;
+	mode->crtc_htotal = 2200;
+	mode->crtc_vtotal = 1125;
+	ctx->acrtc->base.state = &ctx->acrtc_state->base;
+	ctx->acrtc->base.enabled = true;
+	drm_calc_timestamping_constants(&ctx->acrtc->base, mode);
+	ctx->adev->ip_versions[DCE_HWIP][0] = IP_VERSION(3, 1, 2);
+	manage_dm_interrupts(ctx->adev, ctx->acrtc, ctx->acrtc_state);
+
+	state->stream_count = 1;
+	state->stream_status[0].primary_otg_inst = ctx->acrtc->otg_inst;
+
+	return state;
+}
+
+/**
+ * dm_test_gpureset_toggle_vblank_off - Test vblank is off while streams are down
+ * @test: The KUnit test context
+ *
+ * drm_crtc_vblank_get() has to fail, or vblank waiters such as the fbdev
+ * damage worker sleep until they time out.
+ */
+static void dm_test_gpureset_toggle_vblank_off(struct kunit *test)
+{
+	struct dm_test_irq_mgmt_ctx *ctx = dm_test_irq_mgmt_ctx_alloc(test);
+	struct dc_state *state = dm_test_gpureset_vblank_state(test, ctx);
+	struct drm_crtc *crtc = &ctx->acrtc->base;
+
+	KUNIT_ASSERT_EQ(test, drm_crtc_vblank_get(crtc), 0);
+	drm_crtc_vblank_put(crtc);
+
+	dm_gpureset_toggle_vblank(ctx->adev, state, false);
+
+	KUNIT_EXPECT_FALSE(test, ctx->adev->ddev.vblank[0].enabled);
+	KUNIT_EXPECT_EQ(test, drm_crtc_vblank_get(crtc), -EINVAL);
+}
+
+/**
+ * dm_test_gpureset_toggle_vblank_on - Test vblank comes back with its config and mode
+ * @test: The KUnit test context
+ */
+static void dm_test_gpureset_toggle_vblank_on(struct kunit *test)
+{
+	struct dm_test_irq_mgmt_ctx *ctx = dm_test_irq_mgmt_ctx_alloc(test);
+	struct dc_state *state = dm_test_gpureset_vblank_state(test, ctx);
+	struct drm_crtc *crtc = &ctx->acrtc->base;
+
+	dm_gpureset_toggle_vblank(ctx->adev, state, false);
+	dm_gpureset_toggle_vblank(ctx->adev, state, true);
+
+	/* manage_dm_interrupts() derived this off delay from the timing. */
+	KUNIT_EXPECT_EQ(test, ctx->adev->ddev.vblank[0].config.offdelay_ms, 34);
+	/* Vblank timestamps need the mode that drm_crtc_vblank_off() cleared. */
+	KUNIT_EXPECT_EQ(test, ctx->adev->ddev.vblank[0].hwmode.crtc_clock, 148500);
+	KUNIT_ASSERT_EQ(test, drm_crtc_vblank_get(crtc), 0);
+	drm_crtc_vblank_put(crtc);
+}
+
+/**
+ * dm_test_gpureset_toggle_vblank_no_crtc - Test streams without a CRTC are skipped
+ * @test: The KUnit test context
+ */
+static void dm_test_gpureset_toggle_vblank_no_crtc(struct kunit *test)
+{
+	struct dm_test_irq_mgmt_ctx *ctx = dm_test_irq_mgmt_ctx_alloc(test);
+	struct dc_state *state = dm_test_gpureset_vblank_state(test, ctx);
+	struct drm_crtc *crtc = &ctx->acrtc->base;
+
+	state->stream_status[0].primary_otg_inst = ctx->acrtc->otg_inst + 1;
+
+	dm_gpureset_toggle_vblank(ctx->adev, state, false);
+
+	KUNIT_ASSERT_EQ(test, drm_crtc_vblank_get(crtc), 0);
+	drm_crtc_vblank_put(crtc);
+}
+
+/**
+ * dm_test_gpureset_toggle_vblank_crtc_disabled - Test a disabled CRTC stays off
+ * @test: The KUnit test context
+ *
+ * A modeset that waits for the reset to finish may be turning the CRTC off.
+ */
+static void dm_test_gpureset_toggle_vblank_crtc_disabled(struct kunit *test)
+{
+	struct dm_test_irq_mgmt_ctx *ctx = dm_test_irq_mgmt_ctx_alloc(test);
+	struct dc_state *state = dm_test_gpureset_vblank_state(test, ctx);
+
+	dm_gpureset_toggle_vblank(ctx->adev, state, false);
+	ctx->acrtc->base.enabled = false;
+	dm_gpureset_toggle_vblank(ctx->adev, state, true);
+
+	KUNIT_EXPECT_EQ(test, drm_crtc_vblank_get(&ctx->acrtc->base), -EINVAL);
+}
+
 static struct kunit_case amdgpu_dm_tests[] = {
 	/* Simple DM callbacks */
 	KUNIT_CASE(dm_test_wait_for_idle),
@@ -5481,6 +5588,11 @@ static struct kunit_case amdgpu_dm_tests[] = {
 	KUNIT_CASE(dm_test_initialize_plane_overlay),
 	/* dm_gpureset_toggle_interrupts */
 	KUNIT_CASE(dm_test_gpureset_toggle_interrupts_dcn),
+	/* dm_gpureset_toggle_vblank */
+	KUNIT_CASE(dm_test_gpureset_toggle_vblank_off),
+	KUNIT_CASE(dm_test_gpureset_toggle_vblank_on),
+	KUNIT_CASE(dm_test_gpureset_toggle_vblank_no_crtc),
+	KUNIT_CASE(dm_test_gpureset_toggle_vblank_crtc_disabled),
 	{}
 };
 
-- 
2.34.1

Re: [PATCH 4/5] drm/amd/display: turn vblank off in DRM across GPU reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] drm/amd/display: missing Signed-off-by trailer in commit
  message
- [High] drm/amd/display: dm_gpureset_toggle_vblank() leaks
  active_vblank_irq_count
- [High] drm/amd/display: stale vblank->last in
  dm_gpureset_toggle_vblank() on resume
- [Medium] drm/amd/display: racy crtc state access in
  dm_gpureset_toggle_vblank()
- [Medium] drm/amd/display: vblank left disabled on dm_suspend()
  zero-streams error
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] drm/amd/display: dc_lock deadlock in handle_hpd_rx_irq() during
  GPU reset [1]
--

commit d669822b620fb7fc244d14e47f0566cf3ab18695
Author: Prike Liang <Prike.Liang@amd.com>

> drm/amd/display: turn vblank off in DRM across GPU reset
>
> For a GPU reset, dm_suspend() stops all streams with
> amdgpu_dm_commit_zero_streams() and toggles the OTG interrupts
> directly, and dm_resume() restores the cached DC state the same way.
> Neither goes through manage_dm_interrupts(), so DRM is never told that
> vblank stops: drm_crtc_vblank_get() keeps succeeding while no vblank
> interrupt can arrive until the streams are back.
>
> Since commit ebc3351cc4a9 ("drm/amdgpu: remove suspend/resume DRM
> clients during GPU recovery"), the fbdev client keeps running during
> recovery, and its damage worker rate-limits display updates with
> drm_client_modeset_wait_for_vblank(). Every display update during the
> reset then waits for a vblank that never comes, until the wait times
> out and warns:
>
>   amdgpu 0000:03:00.0: [drm] vblank wait timed out on crtc 0
>   WARNING: drivers/gpu/drm/drm_vblank.c:1320 at drm_crtc_wait_one_vblank+0x194/0x200
>   Workqueue: events drm_fb_helper_damage_work
>   Call Trace:
>    drm_client_modeset_wait_for_vblank+0x63/0x80
>    drm_fb_helper_damage_work+0x89/0x1a0
>
> FBIO_WAITFORVSYNC runs into the same timeout, and DRM_IOCTL_WAIT_VBLANK
> sleeps until its own 3 second timeout.
>
> Do what a regular CRTC disable does: call drm_crtc_vblank_off() for the
> CRTCs of the cached DC state before their streams are stopped, and
> drm_crtc_vblank_on_config() with their existing configuration once the
> streams and interrupts are restored. drm_crtc_vblank_get() then fails
> for the duration of the reset, so drm_client_modeset_wait_for_vblank()
> doesn't wait at all, and DRM_IOCTL_WAIT_VBLANK waiters are woken up.
> Page flips that get to amdgpu_dm_commit_planes() meanwhile have their
> event sent right away, see the previous patch.
>
> drm_crtc_vblank_off() also clears the vblank timestamping constants,
> which a modeset sets again before it turns vblank back on. Recompute
> them from the CRTC state after the reset as well; otherwise the vblank
> timestamp query in drm_crtc_vblank_on_config() warns about an
> uninitialized mode.
>
> A modeset that waits for dc_lock during the reset may be turning one of
> these CRTCs off. Leave vblank off for a CRTC that is no longer enabled;
> amdgpu_dm_crtc_set_vblank() would refuse to enable vblank on it anyway.
>
> Neither call can block on the reset: drm_crtc_vblank_off() only takes
> spinlocks and flushes the DRM vblank worker, which DM doesn't use, and
> amdgpu_dm_crtc_set_vblank() doesn't queue vblank_control_work during a
> reset. Unlike suspending the DRM clients, this doesn't make recovery
> wait for anything that may itself depend on the reset.
>
> Add KUnit tests for the new helper.
>
> Fixes: ebc3351cc4a9 ("drm/amdgpu: remove suspend/resume DRM clients during GPU recovery")
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
The commit message is missing a Signed-off-by: trailer from the author.

> 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 22d5ac351f87c..2f60ad0746764 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1494,6 +1494,56 @@ STATIC_IFN_KUNIT void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
>  }
>  EXPORT_IF_KUNIT(dm_gpureset_toggle_interrupts);
>  
> +/**
> + * dm_gpureset_toggle_vblank() - Turn DRM vblank off or on around a GPU reset
> + * @adev: amdgpu device
> + * @state: DC state whose streams the reset stops and restores
> + * @enable: false before the streams are stopped, true once they are restored
> + *
> + * The GPU reset path stops and restores the streams through DC directly and
> + * bypasses manage_dm_interrupts(). Turn vblank off in DRM like a CRTC
> + * disable does, so that drm_crtc_vblank_get() fails while no vblank
> + * interrupt can arrive, instead of letting vblank waiters such as the fbdev
> + * damage worker sleep until they time out.
> + */
> +STATIC_IFN_KUNIT void dm_gpureset_toggle_vblank(struct amdgpu_device *adev,
> +						struct dc_state *state, bool enable)
> +{
> +	struct drm_vblank_crtc_config config;
> +	struct amdgpu_crtc *acrtc;
> +	int i;
> +
> +	for (i = 0; i < state->stream_count; i++) {
> +		int otg_inst = state->stream_status[i].primary_otg_inst;
> +
> +		acrtc = amdgpu_dm_get_crtc_by_otg_inst(adev, otg_inst);
> +		if (!acrtc)
> +			continue;
> +
> +		if (!enable) {
> +			drm_crtc_vblank_off(&acrtc->base);
> +			continue;
> +		}

[Severity: High]
Does turning vblank off here leak dm->active_vblank_irq_count when vblank
was enabled before the reset?

When vblank was enabled prior to the reset, amdgpu_dm_crtc_set_vblank()
queued vblank_control_work, which incremented dm->active_vblank_irq_count
and sent DM_ISM_EVENT_EXIT_IDLE_REQUESTED to acrtc->ism.

During the reset, drm_crtc_vblank_off() calls
drm_vblank_disable_and_save() -> amdgpu_dm_crtc_set_vblank(crtc, false)
and sets vblank->enabled = false, but amdgpu_dm_crtc_set_vblank() returns
early because amdgpu_in_reset(adev) is true:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c:amdgpu_dm_crtc_set_vblank() {
    ...
	if (amdgpu_in_reset(adev))
		return 0;
    ...
}

drm_crtc_vblank_off() then wakes vblank->queue so waiters drop their
vblank references.  On resume, drm_crtc_vblank_on_config() sees
vblank->refcount == 0 and config.offdelay_ms > 0, so it leaves
vblank->enabled as false without re-enabling vblank.  No matching disable
work ever runs, leaving dm->active_vblank_irq_count permanently elevated
and preventing dm_ism_commit_idle_optimization_state() and
amdgpu_dm_commit_streams() from re-enabling display idle optimizations
(dc_allow_idle_optimizations(dm->dc, true)).

> +
> +		/*
> +		 * A modeset waiting for dc_lock may be turning the CRTC off,
> +		 * and amdgpu_dm_crtc_set_vblank() refuses to enable vblank on
> +		 * it then. Leave its vblank off.
> +		 */
> +		if (!acrtc->base.enabled)
> +			continue;
> +
> +		/* drm_crtc_vblank_off() cleared the timestamping constants. */
> +		drm_calc_timestamping_constants(&acrtc->base,
> +						&acrtc->base.state->adjusted_mode);
> +
> +		/* Keep the off delay that manage_dm_interrupts() configured. */
> +		config = drm_crtc_vblank_crtc(&acrtc->base)->config;
> +		drm_crtc_vblank_on_config(&acrtc->base, &config);
> +	}
> +}
> +EXPORT_IF_KUNIT(dm_gpureset_toggle_vblank);

[Severity: High]
Does drm_crtc_vblank_on_config() leave vblank->last set to the pre-reset
frame count while the OTG hardware counter has restarted from 0?

During GPU recovery, the reset thread holds down_write(&adev->reset_domain->sem)
across both dm_suspend() and dm_resume().  When dm_gpureset_toggle_vblank()
calls drm_crtc_vblank_off() and drm_crtc_vblank_on_config() ->
drm_reset_vblank_timestamp(), both query amdgpu_get_vblank_counter_kms() in
task context:

drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c:amdgpu_get_vblank_counter_kms() {
    ...
	if (!down_read_trylock(&adev->reset_domain->sem))
		return drm_crtc_vblank_count(crtc);
    ...
}

Because down_read_trylock() fails while reset_domain->sem is write-locked,
drm_update_vblank_count() on suspend stores the truncated 64-bit software
counter into the 24-bit hardware baseline vblank->last.  Even with commit
"drm/amdgpu: keep reading the vblank counter in IRQ context during reset"
later in this series returning vblank->last in task context,
drm_reset_vblank_timestamp() in dm_resume() still cannot read the post-reset
OTG hardware counter (which dc_commit_streams() just reset to 0) and stores
the old pre-reset vblank->last back into vblank->last.  When the first
post-reset vblank IRQ or drm_vblank_enable() reads the hardware counter
near 0, drm_update_vblank_count() computes (cur_vblank - vblank->last) &
max_vblank_count (0x00ffffff), jumping the software vblank counter forward
by up to ~16.7 million frames.

[Severity: Medium]
Can dm_gpureset_toggle_vblank() race with a concurrent atomic commit that
is deactivating or updating the CRTC?

CPU0 (atomic commit)              CPU1 (GPU reset)
amdgpu_dm_atomic_commit_tail()
  amdgpu_dm_commit_streams()
    manage_dm_interrupts(..., NULL)
      drm_crtc_vblank_off()
    ...
    mutex_lock(&dm->dc_lock)
      <blocks>
                                  dm_resume()
                                    ...
                                    dm_gpureset_toggle_vblank(..., true)
                                      if (!acrtc->base.enabled)
                                      drm_crtc_vblank_on_config()
                                    mutex_unlock(&dm->dc_lock)
    remove_stream(adev, acrtc, ...)
    dc_commit_streams()

On a DPMS-off commit (old_crtc_state->active && !new_crtc_state->active
with new_crtc_state->enable == true), acrtc->base.enabled remains true.
amdgpu_dm_commit_streams() calls manage_dm_interrupts(adev, acrtc, NULL) to
turn vblank off before acquiring dm->dc_lock and calling remove_stream() to
clear acrtc->otg_inst.  If GPU reset runs while amdgpu_dm_commit_streams()
is waiting on dm->dc_lock, dm_gpureset_toggle_vblank() sees
acrtc->base.enabled == true and turns vblank back on, after which
amdgpu_dm_commit_streams() removes the stream and finishes with vblank left
enabled on an inactive CRTC.

In addition, for a full CRTC disable (new_crtc_state->enable == false),
drm_atomic_helper_swap_state() swaps acrtc->base.state to the disabled state
(with adjusted_mode.crtc_clock == 0) before
drm_atomic_helper_update_legacy_modeset_state() clears acrtc->base.enabled,
so dm_gpureset_toggle_vblank() can pass a zeroed mode to
drm_calc_timestamping_constants() and trigger drm_WARN_ON_ONCE() in
drm_crtc_vblank_on_config().  And because dm_gpureset_toggle_vblank() does
not hold crtc->mutex, a concurrent fast atomic commit that does not take
dm->dc_lock can swap and free acrtc->base.state while
acrtc->base.state->adjusted_mode is being read.

Would getting the mode from state->streams[i] and checking
acrtc->base.state->active (or reconciling with the ongoing commit) avoid
reading acrtc->base.state unlocked and re-enabling vblank on a deactivating
CRTC?

> @@ -1628,6 +1678,7 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)
>  			return -ENOMEM;
>  		}
>  
> +		dm_gpureset_toggle_vblank(adev, dm->cached_dc_state, false);
>  		dm_gpureset_toggle_interrupts(adev, dm->cached_dc_state, false);
>  
>  		res = amdgpu_dm_commit_zero_streams(dm->dc);

[Severity: Medium]
If amdgpu_dm_commit_zero_streams() fails just below this, does the error
path leave vblank and OTG interrupts disabled?

On error, dm_suspend() frees dm->cached_dc_state and returns -EINVAL, so
amdgpu_ip_block_suspend() leaves ip_block->status.hw as true and
amdgpu_device_ip_resume_phase3() skips dm_resume(), leaving DRM vblank and
display interrupts permanently off for the active CRTCs.

[Severity: High]
This problem wasn't introduced by this patch, but can dm_suspend() deadlock
on dm->dc_lock with a concurrent HPD RX IRQ worker during GPU reset
[1]?

CPU0                              kworker
----                              -------
mutex_lock(&dm->dc_lock)
                                  handle_hpd_rx_irq()
flush_work(&handler->work)
                                  mutex_lock(&adev->dm.dc_lock)

 *** DEADLOCK ***

CPU0: dm_suspend() -> amdgpu_dm_irq_suspend() -> flush_low_context_interrupt()
kworker: dm_irq_work_func() -> handle_hpd_rx_irq()

In the amdgpu_in_reset(adev) branch, dm_suspend() acquires dm->dc_lock
before calling amdgpu_dm_irq_suspend(), which flushes the low-context HPD
RX work queue while handle_hpd_rx_irq() takes adev->dm.dc_lock around
dc_link_detect().

[1] https://sashiko.dev/#/bug/linux-f34e9965-c69d-47c0-99f8-2d9e9bfb8858

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=4

[PATCH 5/5] drm/amdgpu: keep reading the vblank counter in IRQ context during reset

[Prike Liang]

amdgpu_get_vblank_counter_kms() returns the cached vblank->last instead
of reading the hardware counter whenever it can't take
reset_domain->sem for reading. The vblank interrupt reads the counter
through the same callback via drm_crtc_handle_vblank(), so once GPU
recovery holds the semaphore for writing, every vblank interrupt sees
diff == 0 and the DRM vblank count stops advancing. The CRTCs keep
scanning out until dm_suspend() turns vblank off and stops the streams,
though, which only happens after the rest of the recovery preparation.

Vblank waits in that window don't see a vblank anymore. They only end
when dm_suspend() turns vblank off, or after a second with the
"vblank wait timed out" warning if that takes longer. The fbdev damage
worker is likely to be waiting already, because recovery prints
"GPU reset begin!" right before it takes the semaphore.

Vblank interrupts only arrive while a CRTC is scanning out, so the
interrupt path can read the hardware counter.
amdgpu_device_skip_hw_access() likewise only checks the reset semaphore
in task context. Only back off in task context, so the interrupt path
keeps the count moving until DM turns vblank off.

Fixes: f2bc71d16e73 ("drm/amdgpu: serialize vblank counter reads against GPU reset")
Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
Cc: James Lin <pinglei.lin@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Assisted-by: Copilot:claude-opus-5.5
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 14a72a406d37..06e539c4c3ea 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1692,6 +1692,7 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	unsigned int pipe = crtc->index;
 	struct amdgpu_device *adev = drm_to_adev(dev);
+	bool locked = false;
 	int vpos, hpos, stat;
 	u32 count;
 
@@ -1699,9 +1700,26 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
 		DRM_ERROR("Invalid crtc %u\n", pipe);
 		return -EINVAL;
 	}
-	if (!down_read_trylock(&adev->reset_domain->sem))
-		return drm_crtc_vblank_count(crtc);
 
+	/*
+	 * A GPU reset is in progress and the display hardware cannot be read
+	 * safely. Return the hardware counter value DRM already cached rather
+	 * than the software vblank count: the core feeds this callback's
+	 * result back into vblank->last, and a software count lives in a
+	 * different numeric domain, which later trips
+	 * drm_WARN_ON_ONCE(cur_vblank != vblank->last) in
+	 * drm_update_vblank_count().
+	 *
+	 * Only task context has to back off. Vblank interrupts only arrive
+	 * while a CRTC is scanning out, so the interrupt path can read the
+	 * hardware, and it has to keep counting: vblank waits that already
+	 * sleep when a reset takes the semaphore never complete otherwise.
+	 */
+	if (in_task()) {
+		if (!down_read_trylock(&adev->reset_domain->sem))
+			return drm_crtc_vblank_crtc(crtc)->last;
+		locked = true;
+	}
 
 	/* The hw increments its frame counter at start of vsync, not at start
 	 * of vblank, as is required by DRM core vblank counter handling.
@@ -1747,7 +1765,8 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
 		DRM_DEBUG_VBL("NULL mode info! Returned count may be wrong.\n");
 	}
 
-	up_read(&adev->reset_domain->sem);
+	if (locked)
+		up_read(&adev->reset_domain->sem);
 	return count;
 }
 
-- 
2.34.1

Re: [PATCH 5/5] drm/amdgpu: keep reading the vblank counter in IRQ context during reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] drm/amdgpu: missing Signed-off-by trailer in commit message
- [High] drm/amdgpu: self-exclusion of reset task in
  amdgpu_get_vblank_counter_kms()
- [Medium] drm/amdgpu: vblank->last corruption in
  amdgpu_get_vblank_counter_kms()
- [Medium] drm/amdgpu: in_task() in amdgpu_get_vblank_counter_kms() true
  in threaded IRQ
- [Low] drm/amdgpu: commit message omits vblank->last fallback change
--

commit 65f179edc9e1f1c857a1c9cda00e5004a70269a6
Author: Prike Liang <Prike.Liang@amd.com>

> drm/amdgpu: keep reading the vblank counter in IRQ context during reset
>
> amdgpu_get_vblank_counter_kms() returns the cached vblank->last instead
> of reading the hardware counter whenever it can't take
> reset_domain->sem for reading. The vblank interrupt reads the counter
> through the same callback via drm_crtc_handle_vblank(), so once GPU
> recovery holds the semaphore for writing, every vblank interrupt sees
> diff == 0 and the DRM vblank count stops advancing. The CRTCs keep
> scanning out until dm_suspend() turns vblank off and stops the streams,
> though, which only happens after the rest of the recovery preparation.
>
> Vblank waits in that window don't see a vblank anymore. They only end
> when dm_suspend() turns vblank off, or after a second with the
> "vblank wait timed out" warning if that takes longer. The fbdev damage
> worker is likely to be waiting already, because recovery prints
> "GPU reset begin!" right before it takes the semaphore.
>
> Vblank interrupts only arrive while a CRTC is scanning out, so the
> interrupt path can read the hardware counter.
> amdgpu_device_skip_hw_access() likewise only checks the reset semaphore
> in task context. Only back off in task context, so the interrupt path
> keeps the count moving until DM turns vblank off.
>
> Fixes: f2bc71d16e73 ("drm/amdgpu: serialize vblank counter reads against GPU reset")
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Cc: James Lin <pinglei.lin@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
The commit message is missing a Signed-off-by: trailer.

[Severity: Low]
Prior to this patch, f2bc71d16e73 ("drm/amdgpu: serialize vblank counter
reads against GPU reset") returned drm_crtc_vblank_count(crtc) rather than
vblank->last when the trylock failed.  Should the commit message describe
changing the fallback return value from drm_crtc_vblank_count(crtc) to
drm_crtc_vblank_crtc(crtc)->last instead of stating that it already
returned vblank->last?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 8e7e522a89e42..07d178031abb5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1691,6 +1691,7 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = crtc->index;
>  	struct amdgpu_device *adev = drm_to_adev(dev);
> +	bool locked = false;
>  	int vpos, hpos, stat;
>  	u32 count;
>  
> @@ -1698,9 +1699,26 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
>  		DRM_ERROR("Invalid crtc %u\n", pipe);
>  		return -EINVAL;
>  	}
> -	if (!down_read_trylock(&adev->reset_domain->sem))
> -		return drm_crtc_vblank_count(crtc);
>  
> +	/*
> +	 * A GPU reset is in progress and the display hardware cannot be read
> +	 * safely. Return the hardware counter value DRM already cached rather
> +	 * than the software vblank count: the core feeds this callback's
> +	 * result back into vblank->last, and a software count lives in a
> +	 * different numeric domain, which later trips
> +	 * drm_WARN_ON_ONCE(cur_vblank != vblank->last) in
> +	 * drm_update_vblank_count().
> +	 *
> +	 * Only task context has to back off. Vblank interrupts only arrive
> +	 * while a CRTC is scanning out, so the interrupt path can read the
> +	 * hardware, and it has to keep counting: vblank waits that already
> +	 * sleep when a reset takes the semaphore never complete otherwise.
> +	 */
> +	if (in_task()) {
> +		if (!down_read_trylock(&adev->reset_domain->sem))
> +			return drm_crtc_vblank_crtc(crtc)->last;
> +		locked = true;
> +	}

[Severity: High]
Does down_read_trylock(&adev->reset_domain->sem) also exclude the GPU
recovery task itself during dm_resume()?

amdgpu_device_gpu_recover() holds reset_domain->sem for writing across
amdgpu_device_asic_reset().  After the ASIC reset zeroes the 24-bit
hardware frame counter, dm_resume() runs in task context while still
holding the write lock and calls dm_gpureset_toggle_vblank() ->
drm_crtc_vblank_on_config() -> drm_reset_vblank_timestamp() ->
amdgpu_get_vblank_counter_kms().  Because down_read_trylock() fails even
for the write-lock owner, drm_reset_vblank_timestamp() stores the stale
pre-reset vblank->last instead of the zeroed post-reset hardware counter.
On the first post-reset vblank interrupt or query,
drm_update_vblank_count() computes (cur_vblank - vblank->last) &
max_vblank_count with cur_vblank < vblank->last, wrapping around
0x00ffffff and jumping the DRM vblank counter forward by ~16.7 million
frames.

[Severity: Medium]
Can returning vblank->last in task context corrupt vblank->last when
vblank is enabled during the reset preparation window?

If vblank was disabled before reset and a task (such as
drm_fb_helper_damage_work) calls drm_vblank_get() -> drm_vblank_enable()
after reset_domain->sem is write-locked but before dm_suspend() runs,
amdgpu_dm_crtc_set_vblank() calls drm_crtc_vblank_restore() when IPS and
self-refresh are enabled with disable_immediate:

drivers/gpu/drm/drm_vblank.c:drm_vblank_restore() {
    ...
	get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
					 &t_vblank, false);

	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
	if (framedur_ns)
		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
    ...
	vblank->last = (cur_vblank - diff) & max_vblank_count;
}

Because amdgpu_get_vblank_counter_kms() returns the stale vblank->last as
cur_vblank while drm_get_last_vbltimestamp() ->
amdgpu_crtc_get_scanout_position() still reads the display registers
without holding reset_domain->sem (triggering the lockdep assert in
amdgpu_device_skip_hw_access()), drm_vblank_restore() sets vblank->last to
(vblank->last - diff) & max_vblank_count.  The next vblank interrupt then
reads the real hardware counter in hardirq context and compares it against
(vblank->last - diff), double-counting diff (or wrapping around
max_vblank_count if IPS/PSR reset the hardware counter while vblank was
off).

[Severity: Medium]
Does checking in_task() still back off in the vblank interrupt handler
when forced IRQ threading (CONFIG_PREEMPT_RT or threadirqs) is enabled?

amdgpu_irq_init() requests the IRQ without IRQF_NO_THREAD, so under forced
IRQ threading amdgpu_irq_handler() runs in irq_forced_thread_fn() under
local_bh_disable().  That sets SOFTIRQ_DISABLE_OFFSET rather than
HARDIRQ_MASK or SOFTIRQ_OFFSET, so in_task() returns true and the threaded
vblank interrupt handler still takes the trylock backoff path and returns
vblank->last during reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=5

Re: [PATCH 1/5] drm/client: Make the FBIO_WAITFORVSYNC vblank wait interruptible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/client: missing Signed-off-by trailer in commit message
--

commit bc9640f0750976b2feac736dbfd1a54661008656
Author: Prike Liang <Prike.Liang@amd.com>

> drm/client: Make the FBIO_WAITFORVSYNC vblank wait interruptible
>
> FBIO_WAITFORVSYNC calls drm_client_modeset_wait_for_vblank(), which
> waits with drm_crtc_wait_one_vblank(). That wait is uninterruptible and
> only gives up after one second. If no vblank interrupt arrives, for
> example while the driver resets the display hardware, the caller sleeps
> in D state for the whole second and can't be interrupted, not even by
> SIGKILL.
>
> DRM_IOCTL_WAIT_VBLANK waits interruptibly, and so do fbdev drivers that
> implement FBIO_WAITFORVSYNC themselves, such as atyfb and matroxfb.
>
> Add drm_crtc_wait_one_vblank_interruptible(), a DRM-internal variant of
> drm_crtc_wait_one_vblank() that a signal can interrupt, and use it in
> drm_client_modeset_wait_for_vblank(). Return -ERESTARTSYS when a signal
> interrupts the wait, so that the ioctl returns to userspace or is
> restarted. Timeouts and disabled CRTCs are still not reported.
>
> The fbdev damage worker also calls drm_client_modeset_wait_for_vblank(),
> but it runs in a kernel thread, which doesn't receive signals, so its
> wait doesn't change. Drivers keep using drm_crtc_wait_one_vblank().
>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
The commit message is missing a Signed-off-by trailer, which is required
under the Developer's Certificate of Origin before the patch can be
merged.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=1