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

[Matthew Schwartz]

On Fri, 4 Oct 2024, Melissa Wen wrote:

>
>
>
> On 03/10/2024 20:33, Rodrigo Siqueira wrote:
>>  From: Josip Pavic <Josip.Pavic at 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
Hello,

I wanted to confirm that this patch also fixes artifacts when utilizing 
windowed MPO ODM on DCN32, as I originally reported here:

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

I bisected this as a regression in kernel 6.9, where I noticed my 7900XTX 
started to display almost identical artifacting to the issue on DCN301 
that Melissa linked. As this commit seems to resolve the regression, a 
submission to stable would be greatly appreciated.

Cheers,

Matthew
>>
>>  Reviewed-by: Aric Cyr <aric.cyr at amd.com>
>>  Signed-off-by: Josip Pavic <Josip.Pavic at amd.com>
>>  Signed-off-by: Rodrigo Siqueira <rodrigo.siqueira at 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)
>
>
>