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

[Matthew Schwartz <mattschwartz@xxxxxxxxxxxxxx>]

On Fri, 4 Oct 2024, Melissa Wen wrote:

> On 03/10/2024 20:33, Rodrigo Siqueira wrote:
> >  From: Josip Pavic <Josip.Pavic@xxxxxxx>
> >
> >  [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@xxxxxxx>
> >  Signed-off-by: Josip Pavic <Josip.Pavic@xxxxxxx>
> >  Signed-off-by: Rodrigo Siqueira <rodrigo.siqueira@xxxxxxx>
> >  ---
> >    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)