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

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

[2564278112]

From: Wang Jiang <jiangwang@kylinos.cn>

dc_commit_streams() can fail and leave the streams half committed, but
amdgpu_dm_commit_streams() and the reset recovery path in dm_resume()
only emitted a WARN_ON() and continued.  The planes are then programmed
against a stream list that the hardware never took, which turns a
recoverable commit failure into a crash.

Log the failure and stop programming further stream/plane state in
amdgpu_dm_commit_streams().  In dm_resume(), keep the WARN_ON_ONCE() for
the stack trace, but continue the reset recovery as before.

Signed-off-by: Wang Jiang <jiangwang@kylinos.cn>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 +++++++++++++++++--
 1 file changed, 18 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 c4c8904fea42..d88bd89da840 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1916,7 +1916,12 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
 		commit_params.streams = dc_state->streams;
 		commit_params.stream_count = dc_state->stream_count;
 		dc_exit_ips_for_hw_access(dm->dc);
-		WARN_ON(!dc_commit_streams(dm->dc, &commit_params));
+		if (dc_commit_streams(dm->dc, &commit_params) != DC_OK) {
+			drm_err(adev_to_drm(adev),
+				"DC failed to commit %u stream(s) during reset recovery\n",
+				(unsigned int)commit_params.stream_count);
+			WARN_ON_ONCE(1);
+		}
 
 		dm_gpureset_commit_state(dm->cached_dc_state, dm);
 
@@ -4619,7 +4624,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;
+	}
 
 	bool frl_stream_found = false;
 
-- 
2.25.1

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

[Harry Wentland]

On 10/8/26 03:17, 2564278112@qq.com wrote:
> From: Wang Jiang <jiangwang@kylinos.cn>
>
> dc_commit_streams() can fail and leave the streams half committed, but
> amdgpu_dm_commit_streams() and the reset recovery path in dm_resume()
> only emitted a WARN_ON() and continued.  The planes are then programmed
> against a stream list that the hardware never took, which turns a
> recoverable commit failure into a crash.
>
> Log the failure and stop programming further stream/plane state in
> amdgpu_dm_commit_streams().  In dm_resume(), keep the WARN_ON_ONCE() for
> the stack trace, but continue the reset recovery as before.
>
> Signed-off-by: Wang Jiang <jiangwang@kylinos.cn>

Please look at and respond to the sashiko comments. They highlight
some of the problems with an early return without additional cleanup.

Harry

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 +++++++++++++++++--
>  1 file changed, 18 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 c4c8904fea42..d88bd89da840 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1916,7 +1916,12 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
>  		commit_params.streams = dc_state->streams;
>  		commit_params.stream_count = dc_state->stream_count;
>  		dc_exit_ips_for_hw_access(dm->dc);
> -		WARN_ON(!dc_commit_streams(dm->dc, &commit_params));
> +		if (dc_commit_streams(dm->dc, &commit_params) != DC_OK) {
> +			drm_err(adev_to_drm(adev),
> +				"DC failed to commit %u stream(s) during reset recovery\n",
> +				(unsigned int)commit_params.stream_count);
> +			WARN_ON_ONCE(1);
> +		}
>  
>  		dm_gpureset_commit_state(dm->cached_dc_state, dm);
>  
> @@ -4619,7 +4624,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;
> +	}
>  
>  	bool frl_stream_found = false;