Re: [PATCH 13/26] drm/amd/display: Clear update flags after update has been applied

[Melissa Wen <mwen@igalia.com>]

On 03/10/2024 20:33, Rodrigo Siqueira wrote:
> From: Josip Pavic <Josip.Pavic@amd.com>
>
> [Why]
> Since the surface/stream update flags aren't cleared after applying
> updates, those same updates may be applied again in a future call to
> update surfaces/streams for surfaces/streams that aren't actually part
> of that update (i.e. applying an update for one surface/stream can
> trigger unintended programming on a different surface/stream).
>
> For example, when an update results in a call to
> program_front_end_for_ctx, that function may call program_pipe on all
> pipes. If there are surface update flags that were never cleared on the
> surface some pipe is attached to, then the same update will be
> programmed again.
>
> [How]
> Clear the surface and stream update flags after applying the updates.
Hi,

Just to let you know: this patch fixes artifacts when transitioning from 
2 to 3 planes with dynamic pipe split policy on DCN301, as reported here:

https://gitlab.freedesktop.org/drm/amd/-/issues/3441

The problem was first seen in kernel 6.5, when multiple features were 
enabled (plane color mgmt and zpos properties) and minimal transition 
state was reworked.

Should it be sent to stable too?

Thanks,

Melissa
>
> Reviewed-by: Aric Cyr <aric.cyr@amd.com>
> Signed-off-by: Josip Pavic <Josip.Pavic@amd.com>
> Signed-off-by: Rodrigo Siqueira <rodrigo.siqueira@amd.com>
> ---
>   drivers/gpu/drm/amd/display/dc/core/dc.c | 45 ++++++++++++++++++------
>   1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 981d9a327daf..7b239cbfbb4a 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -5129,11 +5129,26 @@ static bool update_planes_and_stream_v3(struct dc *dc,
>   	return true;
>   }
>   
> +static void clear_update_flags(struct dc_surface_update *srf_updates,
> +	int surface_count, struct dc_stream_state *stream)
> +{
> +	int i;
> +
> +	if (stream)
> +		stream->update_flags.raw = 0;
> +
> +	for (i = 0; i < surface_count; i++)
> +		if (srf_updates[i].surface)
> +			srf_updates[i].surface->update_flags.raw = 0;
> +}
> +
>   bool dc_update_planes_and_stream(struct dc *dc,
>   		struct dc_surface_update *srf_updates, int surface_count,
>   		struct dc_stream_state *stream,
>   		struct dc_stream_update *stream_update)
>   {
> +	bool ret = false;
> +
>   	dc_exit_ips_for_hw_access(dc);
>   	/*
>   	 * update planes and stream version 3 separates FULL and FAST updates
> @@ -5150,10 +5165,16 @@ bool dc_update_planes_and_stream(struct dc *dc,
>   	 * features as they are now transparent to the new sequence.
>   	 */
>   	if (dc->ctx->dce_version >= DCN_VERSION_4_01)
> -		return update_planes_and_stream_v3(dc, srf_updates,
> +		ret = update_planes_and_stream_v3(dc, srf_updates,
>   				surface_count, stream, stream_update);
> -	return update_planes_and_stream_v2(dc, srf_updates,
> +	else
> +		ret = update_planes_and_stream_v2(dc, srf_updates,
>   			surface_count, stream, stream_update);
> +
> +	if (ret)
> +		clear_update_flags(srf_updates, surface_count, stream);
> +
> +	return ret;
>   }
>   
>   void dc_commit_updates_for_stream(struct dc *dc,
> @@ -5163,6 +5184,8 @@ void dc_commit_updates_for_stream(struct dc *dc,
>   		struct dc_stream_update *stream_update,
>   		struct dc_state *state)
>   {
> +	bool ret = false;
> +
>   	dc_exit_ips_for_hw_access(dc);
>   	/* TODO: Since change commit sequence can have a huge impact,
>   	 * we decided to only enable it for DCN3x. However, as soon as
> @@ -5170,17 +5193,17 @@ void dc_commit_updates_for_stream(struct dc *dc,
>   	 * the new sequence for all ASICs.
>   	 */
>   	if (dc->ctx->dce_version >= DCN_VERSION_4_01) {
> -		update_planes_and_stream_v3(dc, srf_updates, surface_count,
> +		ret = update_planes_and_stream_v3(dc, srf_updates, surface_count,
>   				stream, stream_update);
> -		return;
> -	}
> -	if (dc->ctx->dce_version >= DCN_VERSION_3_2) {
> -		update_planes_and_stream_v2(dc, srf_updates, surface_count,
> +	} else if (dc->ctx->dce_version >= DCN_VERSION_3_2) {
> +		ret = update_planes_and_stream_v2(dc, srf_updates, surface_count,
>   				stream, stream_update);
> -		return;
> -	}
> -	update_planes_and_stream_v1(dc, srf_updates, surface_count, stream,
> -			stream_update, state);
> +	} else
> +		ret = update_planes_and_stream_v1(dc, srf_updates, surface_count, stream,
> +				stream_update, state);
> +
> +	if (ret)
> +		clear_update_flags(srf_updates, surface_count, stream);
>   }
>   
>   uint8_t dc_get_current_stream_count(struct dc *dc)