[PATCH] drm/amd/display: Fix writeback completion timing

[PATCH] drm/amd/display: Fix writeback completion timing

[Alex Hung]

[WHY]
The out fence was signalled on the first vblank after arming, before the
DMA finished copying, and the old code worked around this with an
mdelay() in the IRQ handler.

[HOW]
Hold a vblank reference while writeback is pending and signal the out
fence on the second vblank instead of using mdelay(). Add
amdgpu_dm_crtc_complete_writeback() to finish and clean up writeback
from both the IRQ and teardown paths.

This can be verified by running IGT's kms_writeback 20 times without
timeout errors.

Assisted-by: Copilot:Claude-Opus-4.8
Signed-off-by: Alex Hung <alex.hung@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  1 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 33 ++++++++++---------
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 8069fc41cc7f..7c784277396a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -509,6 +509,7 @@ struct amdgpu_crtc {
 	struct drm_pending_vblank_event *event;

 	bool wb_pending;
+	bool wb_frame_done;
 	bool wb_enabled;
 	struct drm_writeback_connector *wb_conn;
 };
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 d67dcaa3fa8f..0f5453649200 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4521,6 +4521,7 @@ bool amdgpu_dm_crtc_complete_writeback(struct amdgpu_crtc *acrtc)
 	spin_lock_irqsave(&acrtc->wb_conn->job_lock, flags);
 	pending = acrtc->wb_pending;
 	acrtc->wb_pending = false;
+	acrtc->wb_frame_done = false;
 	spin_unlock_irqrestore(&acrtc->wb_conn->job_lock, flags);

 	if (!pending)
@@ -4988,6 +4989,7 @@ static void dm_set_writeback(struct amdgpu_display_manager *dm,
 	 * cannot run its matching vblank_put before this get.
 	 */
 	WARN_ON(drm_crtc_vblank_get(&acrtc->base));
+	acrtc->wb_frame_done = false;
 	acrtc->wb_pending = true;
 }

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index c5467f34c51f..4de7fb264cb2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -1974,23 +1974,26 @@ static void dm_crtc_high_irq(void *interrupt_params)
 		return;

 	if (acrtc->wb_conn && acrtc->wb_pending) {
-		struct dc_stream_state *stream = acrtc->dm_irq_params.stream;
-		unsigned int v_total, refresh_hz;
-
-		v_total = stream->adjust.v_total_max ?
-			  stream->adjust.v_total_max : stream->timing.v_total;
-		refresh_hz = div_u64((uint64_t) stream->timing.pix_clk_100hz *
-			     100LL, (v_total * stream->timing.h_total));
-		mdelay(1000 / refresh_hz);
-
-		/*
-		 * Completion (signalling the out fence and releasing the vblank
-		 * reference taken in dm_set_writeback()) is handled by the shared
-		 * helper, which is also used by the teardown path.
-		 */
-		if (amdgpu_dm_crtc_complete_writeback(acrtc))
+		if (acrtc->wb_frame_done) {
+			/*
+			 * Second vblank: the DMA for the captured frame has
+			 * had a full frame period to flush to memory. Signal
+			 * the out fence now.
+			 */
+			amdgpu_dm_crtc_complete_writeback(acrtc);
+		} else {
+			/*
+			 * First vblank after arming: the frame has been
+			 * scanned out and the DMA is finishing. Disable
+			 * writeback immediately to prevent the hardware from
+			 * starting a new capture that would overwrite the
+			 * buffer. Signal completion on the next vblank to
+			 * ensure the DMA is fully flushed to memory.
+			 */
 			dc_stream_fc_disable_writeback(adev->dm.dc,
 						       acrtc->dm_irq_params.stream, 0);
+			acrtc->wb_frame_done = true;
+		}
 	}

 	vrr_active = amdgpu_dm_crtc_vrr_active_irq(acrtc);
--
2.43.0

Re: [PATCH] drm/amd/display: Fix writeback completion timing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lockless state machine corruption on overlapping writeback jobs stalls fences and leaks vblank references.

Pre-existing issues:
- [High] Unconditional memory leak of dc_writeback_info structure in dm_set_writeback.
--

--- Patch [1]: [PATCH] drm/amd/display: Fix writeback completion timing ---
commit d7178e82119f1d066c5082ca676728119bef3a13
Author: Alex Hung <alex.hung@amd.com>

drm/amd/display: Fix writeback completion timing

Hold a vblank reference while writeback is pending and signal the out
fence on the second vblank instead of using mdelay(). Add
amdgpu_dm_crtc_complete_writeback() to finish and clean up writeback
from both the IRQ and teardown paths.

> 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 b97ceabe61736..90ee250e6e360 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -4988,6 +4989,7 @@ static void dm_set_writeback(struct amdgpu_display_manager *dm,
>  	 * cannot run its matching vblank_put before this get.
>  	 */
>  	WARN_ON(drm_crtc_vblank_get(&acrtc->base));
> +	acrtc->wb_frame_done = false;
>  	acrtc->wb_pending = true;

[Severity: High]
Does this lockless state update cause issues if userspace submits another
writeback job immediately?

Since page flip events can signal on the first vblank, userspace could
submit a new atomic commit before the second vblank occurs. If a new
job is submitted in this window, dm_set_writeback() will overwrite
acrtc->wb_frame_done = false and acrtc->wb_pending = true without
holding a lock to protect the active job's state.

[Severity: High]
This is a pre-existing issue, but does this function leak the memory
allocated for wb_info?

Earlier in dm_set_writeback(), wb_info is allocated:

	wb_info = kzalloc(sizeof(*wb_info), GFP_KERNEL);

Then it is passed to dc_stream_add_writeback():

	dc_stream_add_writeback(dm->dc, crtc_state->stream, wb_info);

Since dc_stream_add_writeback() copies the structure by value into the
stream's array and does not take ownership of the pointer, it seems the
originally allocated wb_info pointer is never freed before returning.
Could this lead to continuous kernel memory leakage when userspace streams
writeback?

>  }
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> index c5467f34c51f1..4de7fb264cb26 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> @@ -1974,23 +1974,26 @@ static void dm_crtc_high_irq(void *interrupt_params)
>  		return;
>  
>  	if (acrtc->wb_conn && acrtc->wb_pending) {
> -		struct dc_stream_state *stream = acrtc->dm_irq_params.stream;
> -		unsigned int v_total, refresh_hz;
> -
> -		v_total = stream->adjust.v_total_max ?
> -			  stream->adjust.v_total_max : stream->timing.v_total;
> -		refresh_hz = div_u64((uint64_t) stream->timing.pix_clk_100hz *
> -			     100LL, (v_total * stream->timing.h_total));
> -		mdelay(1000 / refresh_hz);
> -
> -		/*
> -		 * Completion (signalling the out fence and releasing the vblank
> -		 * reference taken in dm_set_writeback()) is handled by the shared
> -		 * helper, which is also used by the teardown path.
> -		 */
> -		if (amdgpu_dm_crtc_complete_writeback(acrtc))
> +		if (acrtc->wb_frame_done) {

[Severity: High]
If the state was overwritten by a new commit as mentioned above, will this
read wb_frame_done == false on the second vblank?

If so, it seems it would take the else branch, set wb_frame_done = true
again, and completely skip calling amdgpu_dm_crtc_complete_writeback()
for the original job, potentially stalling the out-fence permanently and
leaking a vblank reference.

> +			/*
> +			 * Second vblank: the DMA for the captured frame has
> +			 * had a full frame period to flush to memory. Signal
> +			 * the out fence now.
> +			 */
> +			amdgpu_dm_crtc_complete_writeback(acrtc);
> +		} else {
> +			/*
> +			 * First vblank after arming: the frame has been
> +			 * scanned out and the DMA is finishing. Disable
> +			 * writeback immediately to prevent the hardware from
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711033159.3096477-1-alex.hung@amd.com?part=1