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

[Alex Deucher]

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
> > >
> >