[igt-dev] [PATCH i-g-t] tests/kms_plane_cursor: Change API for collecting the CRC

[Aurabindo Pillai]

On 2022-10-18 13:18, Juha-Pekka Heikkila wrote:
> On 18.10.2022 19.09, Aurabindo Pillai wrote:
>>
>>
>> On 2022-10-15 16:30, Juha-Pekka Heikkila wrote:
>>> On 14.10.2022 21.49, Aurabindo Pillai wrote:
>>>> On AMD hw, igt_pipe_crc_get_current() does not work correctly, but
>>>> igt_pipe_crc_collect_crc() does. The latter is more robust and reports
>>>> the expected crc. This is due to a delay of 2 frames in the hardware 
>>>> for
>>>> crc capture.
>>>>
>>>> The following diff is an alternate fix instead of changing the API:
>>>>
>>>> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
>>>> index fa1e3b69..dfcb2638 100644
>>>> --- a/lib/igt_debugfs.c
>>>> +++ b/lib/igt_debugfs.c
>>>> @@ -1061,6 +1061,10 @@ igt_pipe_crc_get_current(int drm_fd, 
>>>> igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
>>>>   {
>>>>          unsigned vblank = kmstest_get_vblank(drm_fd, 
>>>> pipe_crc->pipe, 0) + 1;
>>>>
>>>> +       /* AMD hw has an additional frame delay */
>>>> +       if (is_amdgpu_device(drm_fd))
>>>> +               vblank += 1;
>>>> +
>>>>          return igt_pipe_crc_get_for_frame(drm_fd, pipe_crc, vblank, 
>>>> crc);
>>>>   }
>>>>
>>>> Since the above solution adds device specific code into core IGT lib,
>>>> changing the API is a preferable solution.
>>>
>>> This sound like you are fixing driver bug in test suite. If pipe crc 
>>> doesn't match it mean rendered picture is wrong. If later crcs match 
>>> you're probably showing picture too early, I have no amd hw at hand 
>>> so I have no real idea what's going wrong.
>>
>> Looks like patchwork got confused. The hunk above is not part of the 
>> change and was just meant to show the underlying problem. Its either 
>> the above one, or the one below that fixes the issue.
>>
>> Since flipping and crc enablement happen asynchronously, more often 
>> than not -  we return an 'uncooked' crc on first frame probably 
>> because hw isn't ready yet. For this reason, we skip the first two CRC 
>> values. Its not a driver bug.
> 
> If crc enablement is where problem is coming from would correct place 
> for amd related special case then be at igt_pipe_crc_start() ?
> 
> If you look at that function you see also Intel hw need special handling 
> there with fifo underruns.

Here is the case that fails:

igt_pipe_crc_start()
igt_pipe_crc_get_current(ref_crc)
igt_pipe_crc_get_current(test_crc)
igt_pipe_crc_stop()

Once I change the API like in this patch, this sequence becomes:

igt_pipe_crc_start()
igt_pipe_crc_get_current(ref_crc)
igt_pipe_crc_stop()

igt_pipe_crc_start()
igt_pipe_crc_get_current(test_crc)
igt_pipe_crc_stop()

Since igt_pipe_crc_start() already calls an igt_pipe_crc_stop(), the 
openat syscall within igt_pipe_crc_start() is what helps in a pass, 
since inside the driver, we skip the first two CRCs when it's opened.

The subsequent calls to igt_pipe_crc_get_current() should technically be 
correct, without having to require additional frame delays. However, 
cursor updates aren't synchronized with our atomic code path with the 
same frame. It's possible that they get deferred to the next.

I think a proper fix would be to make the cursor programming an atomic 
update for AMDGPU, but that's not a trivial change. Someone else was 
working on this, though. Until that solution is ready, I'd like a device 
specific call to igt_pipe_crc_collect_crc() so that this test passes on 
AMD hw.