[PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[Alex Deucher]

On Fri, May 29, 2020 at 9:56 AM Kazlauskas, Nicholas
<nicholas.kazlauskas at amd.com> wrote:
>
> On 2020-05-28 10:06 a.m., Alex Deucher wrote:
> > The logic for blanked is not the same as having a plane_state. Technically
> > you can drive an OTG without anything connected in the front end and it'll
> > just draw out the back color which is distinct from having the OTG be blanked.
> > If we add planes or unblank the OTG later then we'll still want the
> > synchronization.
> >
> > Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
> > Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
> > Cc: nicholas.kazlauskas at amd.com
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com> > ---
> >   drivers/gpu/drm/amd/display/dc/core/dc.c | 8 --------
> >   1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > index 04c3d9f7e323..6279520f7873 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > @@ -1040,14 +1040,6 @@ static void program_timing_sync(
> >                               status->timing_sync_info.master = false;
> >
> >               }
> > -             /* remove any other pipes with plane as they have already been synced */
> > -             for (j = j + 1; j < group_size; j++) {
> > -                     if (pipe_set[j]->plane_state) {
> > -                             group_size--;
> > -                             pipe_set[j] = pipe_set[group_size];
> > -                             j--;
> > -                     }
> > -             }
>
>
> Looking at this again, I think I may understand the issue this was
> trying to work around.
>
> If we try to force timing synchronization on displays that are currently
> active then this is going to force reset the vertical position,
> resulting in screen corruption.
>
> So what this logic was attempting to do was ensure that timing
> synchronization only happens when committing two streams at a time
> without any image on the screen.
>
> Maybe it'd be best to just blank these streams out first, but for now,
> let's actually go back to fixing this by applying the actual dpg/tg
> check that Wenjing suggests, something like:
>
>     if (pool->opps[i]->funcs->dpg_is_blanked)
>                  s.blank_enabled =
> pool->opps[i]->funcs->dpg_is_blanked(pool->opps[i]);
>             else
>                  s.blank_enabled = tg->funcs->is_blanked(tg);
>

Hmm, it's not clear to me where this code needs to go.  Can you point
me in the right direction or provide a quick patch?

Thanks,

Alex

>
>
> The reason why we have this issue in the first place is because
> amdgpu_dm usually commits a dc_state with the planes already in it
> instead of committing them later, so plane_state not being NULL is
> typically true.
>
> Regards,
> Nicholas Kazlauskas
>
> >
> >               if (group_size > 1) {
> >                       dc->hwss.enable_timing_synchronization(
> >
>