[PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[Alex Deucher]

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.
If we add planes or unblank the OTG later then we'll still want the
synchronization.

Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
Cc: nicholas.kazlauskas@amd.com
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 04c3d9f7e323..6279520f7873 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -1040,14 +1040,6 @@ static void program_timing_sync(
 				status->timing_sync_info.master = false;
 
 		}
-		/* remove any other pipes with plane as they have already been synced */
-		for (j = j + 1; j < group_size; j++) {
-			if (pipe_set[j]->plane_state) {
-				group_size--;
-				pipe_set[j] = pipe_set[group_size];
-				j--;
-			}
-		}
 
 		if (group_size > 1) {
 			dc->hwss.enable_timing_synchronization(
-- 
2.25.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[Kazlauskas, Nicholas]

On 2020-05-28 10:06 a.m., Alex Deucher wrote:
> 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.
> If we add planes or unblank the OTG later then we'll still want the
> synchronization.
> 
> Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
> Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
> Cc: nicholas.kazlauskas@amd.com
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > ---
>   drivers/gpu/drm/amd/display/dc/core/dc.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 04c3d9f7e323..6279520f7873 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -1040,14 +1040,6 @@ static void program_timing_sync(
>   				status->timing_sync_info.master = false;
>   
>   		}
> -		/* remove any other pipes with plane as they have already been synced */
> -		for (j = j + 1; j < group_size; j++) {
> -			if (pipe_set[j]->plane_state) {
> -				group_size--;
> -				pipe_set[j] = pipe_set[group_size];
> -				j--;
> -			}
> -		}


Looking at this again, I think I may understand the issue this was 
trying to work around.

If we try to force timing synchronization on displays that are currently 
active then this is going to force reset the vertical position, 
resulting in screen corruption.

So what this logic was attempting to do was ensure that timing 
synchronization only happens when committing two streams at a time 
without any image on the screen.

Maybe it'd be best to just blank these streams out first, but for now, 
let's actually go back to fixing this by applying the actual dpg/tg 
check that Wenjing suggests, something like:

    if (pool->opps[i]->funcs->dpg_is_blanked)
                 s.blank_enabled = 
pool->opps[i]->funcs->dpg_is_blanked(pool->opps[i]);
            else
                 s.blank_enabled = tg->funcs->is_blanked(tg);



The reason why we have this issue in the first place is because 
amdgpu_dm usually commits a dc_state with the planes already in it 
instead of committing them later, so plane_state not being NULL is 
typically true.

Regards,
Nicholas Kazlauskas

>   
>   		if (group_size > 1) {
>   			dc->hwss.enable_timing_synchronization(
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[Alex Deucher]

On Fri, May 29, 2020 at 9:56 AM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2020-05-28 10:06 a.m., Alex Deucher wrote:
> > 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.
> > If we add planes or unblank the OTG later then we'll still want the
> > synchronization.
> >
> > Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
> > Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
> > Cc: nicholas.kazlauskas@amd.com
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > ---
> >   drivers/gpu/drm/amd/display/dc/core/dc.c | 8 --------
> >   1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > index 04c3d9f7e323..6279520f7873 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > @@ -1040,14 +1040,6 @@ static void program_timing_sync(
> >                               status->timing_sync_info.master = false;
> >
> >               }
> > -             /* remove any other pipes with plane as they have already been synced */
> > -             for (j = j + 1; j < group_size; j++) {
> > -                     if (pipe_set[j]->plane_state) {
> > -                             group_size--;
> > -                             pipe_set[j] = pipe_set[group_size];
> > -                             j--;
> > -                     }
> > -             }
>
>
> Looking at this again, I think I may understand the issue this was
> trying to work around.
>
> If we try to force timing synchronization on displays that are currently
> active then this is going to force reset the vertical position,
> resulting in screen corruption.
>
> So what this logic was attempting to do was ensure that timing
> synchronization only happens when committing two streams at a time
> without any image on the screen.
>
> Maybe it'd be best to just blank these streams out first, but for now,
> let's actually go back to fixing this by applying the actual dpg/tg
> check that Wenjing suggests, something like:
>
>     if (pool->opps[i]->funcs->dpg_is_blanked)
>                  s.blank_enabled =
> pool->opps[i]->funcs->dpg_is_blanked(pool->opps[i]);
>             else
>                  s.blank_enabled = tg->funcs->is_blanked(tg);
>

Hmm, it's not clear to me where this code needs to go.  Can you point
me in the right direction or provide a quick patch?

Thanks,

Alex

>
>
> The reason why we have this issue in the first place is because
> amdgpu_dm usually commits a dc_state with the planes already in it
> instead of committing them later, so plane_state not being NULL is
> typically true.
>
> Regards,
> Nicholas Kazlauskas
>
> >
> >               if (group_size > 1) {
> >                       dc->hwss.enable_timing_synchronization(
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/display: drop the reduction loop when setting the sync groups

[Kazlauskas, Nicholas]

On 2020-05-29 2:04 p.m., Alex Deucher wrote:
> On Fri, May 29, 2020 at 9:56 AM Kazlauskas, Nicholas
> <nicholas.kazlauskas@amd.com> wrote:
>>
>> On 2020-05-28 10:06 a.m., Alex Deucher wrote:
>>> 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.
>>> If we add planes or unblank the OTG later then we'll still want the
>>> synchronization.
>>>
>>> Bug: https://gitlab.freedesktop.org/drm/amd/issues/781
>>> Fixes: 5fc0cbfad45648 ("drm/amd/display: determine if a pipe is synced by plane state")
>>> Cc: nicholas.kazlauskas@amd.com
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > ---
>>>    drivers/gpu/drm/amd/display/dc/core/dc.c | 8 --------
>>>    1 file changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
>>> index 04c3d9f7e323..6279520f7873 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
>>> @@ -1040,14 +1040,6 @@ static void program_timing_sync(
>>>                                status->timing_sync_info.master = false;
>>>
>>>                }
>>> -             /* remove any other pipes with plane as they have already been synced */
>>> -             for (j = j + 1; j < group_size; j++) {
>>> -                     if (pipe_set[j]->plane_state) {
>>> -                             group_size--;
>>> -                             pipe_set[j] = pipe_set[group_size];
>>> -                             j--;
>>> -                     }
>>> -             }
>>
>>
>> Looking at this again, I think I may understand the issue this was
>> trying to work around.
>>
>> If we try to force timing synchronization on displays that are currently
>> active then this is going to force reset the vertical position,
>> resulting in screen corruption.
>>
>> So what this logic was attempting to do was ensure that timing
>> synchronization only happens when committing two streams at a time
>> without any image on the screen.
>>
>> Maybe it'd be best to just blank these streams out first, but for now,
>> let's actually go back to fixing this by applying the actual dpg/tg
>> check that Wenjing suggests, something like:
>>
>>      if (pool->opps[i]->funcs->dpg_is_blanked)
>>                   s.blank_enabled =
>> pool->opps[i]->funcs->dpg_is_blanked(pool->opps[i]);
>>              else
>>                   s.blank_enabled = tg->funcs->is_blanked(tg);
>>
> 
> Hmm, it's not clear to me where this code needs to go.  Can you point
> me in the right direction or provide a quick patch?
> 
> Thanks,
> 
> Alex

The old code used to check !tg->funcs->is_blanked(tg) ie. to drop the 
pipe from the group if it's currently active.

The issue was that on newer ASIC it's now the DPG that's the indicator 
from the hardware side, so we should replace the !plane_state check with 
a check first for !dpg_is_blanked and then !is_blanked if the DPG 
doesn't exist.

Regards,
Nicholas Kazlauskas

> 
>>
>>
>> The reason why we have this issue in the first place is because
>> amdgpu_dm usually commits a dc_state with the planes already in it
>> instead of committing them later, so plane_state not being NULL is
>> typically true.
>>
>> Regards,
>> Nicholas Kazlauskas
>>
>>>
>>>                if (group_size > 1) {
>>>                        dc->hwss.enable_timing_synchronization(
>>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx