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

[Alex Deucher]

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