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

[Juha-Pekka Heikkila]

On 19.10.2022 21.38, Aurabindo Pillai wrote:
> 
> 
> 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.

This sound similar issue as guys writing msm driver are having. Their 
argument was on kms_cursor_crc legacy cursor api is not supposing to be 
frame exact but they still wanted crc test to pass. You can see 
discussion here
https://lists.freedesktop.org/archives/igt-dev/2022-September/045784.html
..or maybe in bit more convenient form same discussion in patchwork 
https://patchwork.freedesktop.org/patch/503140/?series=108579&rev=2

 From kms_cursor_crc if you look at data.vblank_wait_count you see what 
was done with msm on that test.

 From above sequence you listed would it work if you just add vblank 
wait before getting test_crc when running on amd? You'd get away with 
minimal change and I think it would look less messy than what I see you 
suggested on v3..I'd put my r-b for that amd specific case but imho I 
don't think you are then really testing what you want to test as I see 
users complain if some panel self refresh features cause cursor run just 
one frame late.

Anyway, if you have someone writing driver side fix for this wouldn't 
this type adjusting of test harm that work?

/Juha-Pekka