Re: [PATCH] drm/amd/display: Don't ignore dc_commit_streams() failures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amd: incomplete fix, plane programming still executes after commit failure
- [Medium] drm/amd/display: hdmi_frl_status_polling_work leaks on commit failure
--

commit 5df85d959754dc46c8ae54787ba1fc6639162940
Author: Wang Jiang <jiangwang@kylinos.cn>

drm/amd/display: Don't ignore dc_commit_streams() failures

This patch modifies the AMD DRM driver to stop ignoring failures from
dc_commit_streams(). It logs the failure and adds an early return in
amdgpu_dm_commit_streams() to avoid programming plane state against a
half-committed stream list. It also logs failures in the reset recovery
path in dm_resume().

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

[ ... ]

> @@ -4735,7 +4740,18 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
>  	dm_enable_per_frame_crtc_master_sync(dc_state);
>  	mutex_lock(&dm->dc_lock);
>  	dc_exit_ips_for_hw_access(dm->dc);
> -	WARN_ON(!dc_commit_streams(dm->dc, &params));
> +	if (dc_commit_streams(dm->dc, &params) != DC_OK) {
> +		mutex_unlock(&dm->dc_lock);
> +		/*
> +		 * DC refused the stream list, so the hardware and the DC state
> +		 * no longer agree. Bail out instead of programming more
> +		 * stream/plane state for a half committed stream list (this
> +		 * used to be a silently ignored WARN_ON()).
> +		 */
> +		drm_err(dev, "DC failed to commit %u stream(s)\n",
> +			(unsigned int)params.stream_count);
> +		return;

[Severity: High]
Will this early return successfully stop the plane programming?

Since amdgpu_dm_commit_streams() returns void, it cannot propagate this
failure state back to its caller. Looking at the caller
amdgpu_dm_atomic_commit_tail():

amdgpu_dm_atomic_commit_tail() {
        ...
        if (dm_state && dm_state->context) {
                dc_state = dm_state->context;
                amdgpu_dm_commit_streams(state, dc_state);
        }
        ...
        if (dm_new_crtc_state->stream)
                amdgpu_dm_commit_planes(state, dev, dm, crtc, wait_for_vblank);
}

Because the tail continues unconditionally, won't it still program the
planes against the rejected stream list, resulting in the exact crash
this patch intended to prevent?

[Severity: Medium]
Does this early return cause the HDMI FRL status polling work to leak?

By returning early, we bypass the subsequent block that manages the FRL
polling work:

        if (frl_stream_found) {
                queue_delayed_work(...)
        } else {
                if (cancel_delayed_work_sync(&dm->hdmi_frl_status_polling_work))
                        ...
        }

Looking at hdmi_frl_status_polling_work(), the handler unconditionally
re-arms itself by calling queue_delayed_work() at the end of its execution.
If we fail the stream commit and return early, doesn't it leave the delayed
work to run and wake up the CPU indefinitely on a recoverable failure path?

> +	}
>  
>  	bool frl_stream_found = false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_34284E790157CA20C27C9260A7A32C08C10A@qq.com?part=1