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

[sashiko-bot@kernel.org]

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