[PATCH] drm/amdgpu/display: fix logic inversion in program_timing_sync()
Kazlauskas, Nicholas
nicholas.kazlauskas at amd.com
Tue Feb 4 14:06:53 UTC 2020
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.
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
>
More information about the amd-gfx
mailing list