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

[Kazlauskas, Nicholas]

I still think we should just drop the reduction loop.

The problem with checking plane_state at all is that this logic will 
always be broken - plane_state isn't a good indicator of whether the 
stream is blanked or not since we can leave an OTG running with no 
planes at all while unblanked.

Regards,
Nicholas Kazlauskas

On 2020-05-27 4:51 p.m., Alex Deucher wrote:
> 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
>>>>>
>>>>