[PATCH] drm/amd/display: Copy crc_skip_count when duplicating CRTC state

[Rodrigo Siqueira Jordao]

On 2022-01-18 11:49 a.m., Kazlauskas, Nicholas wrote:
> On 1/18/2022 11:40 AM, Rodrigo Siqueira wrote:
>> From: Leo Li <sunpeng.li at amd.com>
>>
>> [Why]
>> crc_skip_count is used to track how many frames to skip to allow the OTG
>> CRC engine to "warm up" before it outputs correct CRC values.
>> Experimentally, this seems to be 2 frames.
>>
>> When duplicating CRTC states, this value was not copied to the
>> duplicated state. Therefore, when this state is committed, we will
>> needlessly wait 2 frames before outputing CRC values. Even if the CRC
>> engine is already warmed up. >
>> [How]
>> Copy the crc_skip_count as part of dm_crtc_duplicate_state.
> 
> This likely introduces regressions.
> 
> Here's an example case where it can take two frames even after the CRTC 
> is enabled:
> 
> 1. VUPDATE is before line 0, in the front porch, counter=0
> 2. Flip arrives before VUPDATE is signaled, but does not finish 
> programming until after VUPDATE point, counter=0.
> 3. Vblank counter increments, counter=1.
> 4. Flip programming finishes, counter=1.
> 5. OS delay happens, cursor programming is delayed, counter=1.
> 6. Cursor programming starts, counter=1.
> 7. VUPDATE fires, updating frame but missing cursor, counter=1.
> 8. Cursor programming finishes, counter=2.
> 9. Cursor programming pending for counter=2.
> 
> This is a little contrived, but I've seen something similar happen 
> during IGT testing before.

Hi Nick,

I'm wondering if we have a test that can reproduce the issue that you 
described. For this specific patch, I ran it through our pre-submission 
CI, and everything worked as expected, except the subtest 
pipe-a-primary-size-* in the kms_plane_cursor. However, I submitted the 
below IGT patch that fixed the issue:

  https://patchwork.freedesktop.org/patch/469919/?series=98994&rev=1
  (Note that the IGT fix is not related to the scenario you described.)

In summary, I do not see regressions in other tests, whereas this patch 
fixes multiple CRC mismatches when running IGT (e.g., kms_plane and 
plane_scaling).

Do you recommend running some specific test to reproduce the issue? 
Maybe some particular set of steps that enable me to see the issue?

Ps.: Some IGT tests already compensate for the two extra frames (see 
link [1]), this behavior also happens on other drivers such as i915, and 
we still fail in those tests because we are skipping four frames without 
this patch.
1. 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_plane.c#L612

Thanks
Siqueira

> This is because cursor update happens independent of the rest of plane 
> programming and is tied to a separate lock. That lock part can't change 
> due to potential for stuttering, but the first part could be fixed.
> 
> Regards,
> Nicholas Kazlauskas
> 
>>
>> Cc: Mark Yacoub <markyacoub at chromium.org>
>> Cc: Hayden Goodfellow <hayden.goodfellow at amd.com>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Cc: Nicholas Choi <Nicholas.Choi at amd.com>
>>
>> Signed-off-by: Leo Li <sunpeng.li at amd.com>
>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 87299e62fe12..5482b0925396 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6568,6 +6568,7 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>       state->freesync_config = cur->freesync_config;
>>       state->cm_has_degamma = cur->cm_has_degamma;
>>       state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb;
>> +    state->crc_skip_count = cur->crc_skip_count;
>>       state->force_dpms_off = cur->force_dpms_off;
>>       /* TODO Duplicate dc_stream after objects are stream object is 
>> flattened */
>