[PATCH] drm/amdgpu/display: fix logic inversion in program_timing_sync()

[Alex Deucher]

Can we apply this for now until we can get further analysis on the
actual root cause?

Alex

On Mon, Apr 6, 2020 at 10:44 AM Alex Deucher <alexdeucher at gmail.com> wrote:
>
> Ping again?
>
> Alex
>
> On Thu, Feb 20, 2020 at 8:27 AM Alex Deucher <alexdeucher at gmail.com> wrote:
> >
> > On Tue, Feb 4, 2020 at 9:06 AM Kazlauskas, Nicholas
> > <nicholas.kazlauskas at amd.com> wrote:
> > >
> > > Comments inline.
> > >
> > > On 2020-02-03 4:07 p.m., Alex Deucher wrote:
> > > > Ping?
> > > >
> > > > On Fri, Jan 10, 2020 at 3:11 PM Alex Deucher <alexdeucher at gmail.com> wrote:
> > > >>
> > > >> It looks like we should be reducing the group size when we don't
> > > >> have a plane rather than when we do.
> > > >>
> > > >> Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
> > > >> Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
> > > >> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > > >> ---
> > > >>   drivers/gpu/drm/amd/display/dc/core/dc.c | 4 ++--
> > > >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > >> index 3d89904003f0..01b27726d9c5 100644
> > > >> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > >> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > >> @@ -1003,9 +1003,9 @@ static void program_timing_sync(
> > > >>                                  status->timing_sync_info.master = false;
> > > >>
> > > >>                  }
> > > >> -               /* remove any other pipes with plane as they have already been synced */
> > > >> +               /* remove any other pipes without plane as they have already been synced */
> > >
> > > This took a while to wrap my head around but I think I understand what
> > > this was originally trying to do.
> > >
> > > The original logic seems to have been checking for blanked streams and
> > > trying to remove anything that was blanked from the group to try and
> > > avoid having to enable timing synchronization.
> > >
> > > However, 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.
> > >
> > > The problem is really this iteration below:
> > >
> > > >>                  for (j = j + 1; j < group_size; j++) {
> > >
> > > There could still be pipes in here (depending on the ordering) that have
> > > planes and could be synchronized with the master OTG. I think starting
> > > at j + 1 is a mistake for this logic as well.
> > >
> > > I wonder if we can just drop this loop altogether. If we add planes or
> > > unblank the OTG later then we'll still want the synchronization.
> > >
> > > Dymtro, Wenjing - feel free to correct my understanding if I'm mistaken
> > > about this.
> >
> > Ping?  Any thoughts on this?  It would be nice to get this fixed.
> >
> > Alex
> >
> >
> > >
> > > Regards,
> > > Nicholas Kazlauskas
> > >
> > > >> -                       if (pipe_set[j]->plane_state) {
> > > >> +                       if (!pipe_set[j]->plane_state) {
> > > >>                                  group_size--;
> > > >>                                  pipe_set[j] = pipe_set[group_size];
> > > >>                                  j--;
> > > >> --
> > > >> 2.24.1
> > > >>
> > > > _______________________________________________
> > > > amd-gfx mailing list
> > > > amd-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > > >
> > >