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

[Aurabindo Pillai]

On 2022-10-20 07:27, Juha-Pekka Heikkila wrote:
> 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

Sorry, I think my fingers skipped a crucial part in my response. I meant 
to say that cursor updates aren't synchronized with our atomic code path 
with the same frame.

But thanks for linking the other discussion. Skipping a vblank does 
help. I'll send a v4