[igt-dev] [PATCH i-g-t v1] tests/kms_cursor_crc: Wait extra vblank

[Jessica Zhang]

On 9/15/2022 12:58 PM, Jessica Zhang wrote:
> Hi Juha-Pekka
> 
> On 9/15/2022 11:02 AM, Juha-Pekka Heikkila wrote:
>> Hi Jessica,
>>
>> On 15.9.2022 1.58, Jessica Zhang wrote:
>>> Wait an extra vblank for legacy cursor ioctl to finish.
>>>
>>> Extra vblank wait is needed for both HW and SW test as the legacy cursor
>>> ioctl is called in both cases.
>>>
>>> Based on Rob's patch [1] and, similarly, fixes flaky results on MSM.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>
>>> [1] https://patchwork.freedesktop.org/series/105999/
>>> ---
>>>   tests/kms_cursor_crc.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
>>> index 53f18f4f1add..272dcb7fa0a4 100644
>>> --- a/tests/kms_cursor_crc.c
>>> +++ b/tests/kms_cursor_crc.c
>>> @@ -202,8 +202,8 @@ static void do_single_test(data_t *data, int x, 
>>> int y, bool hw_test,
>>>           igt_display_commit(display);
>>>           /* Extra vblank wait is because nonblocking cursor ioctl */
>>> -        igt_wait_for_vblank(data->drm_fd,
>>> -                display->pipes[data->pipe].crtc_offset);
>>> +        igt_wait_for_vblank_count(data->drm_fd,
>>> +                display->pipes[data->pipe].crtc_offset, 2);
>>
>> It will take more than one frame on your target device for non 
>> blocking cursor commit to settle? This cannot be right, even if this 
>> somehow was the case you are breaking this test for everyone else.
> 
> As stated in Rob's original patch [1]:
> 
> "if the cursor update comes too close to vblank,
> it could end up being scheduled for the *following* vblank to avoid
> blocking"
> 
> In the case that the cursor commit is scheduled for the next vblank 
> (let's call it vblank n + 1), the CRC value will be updated in the 
> *following* that vblank (so, vblank n + 2) because the CRC value in MSM 
> updates in the following vblank after a commit [2].

*"... in the vblank *following* that vblank ..."

> 
> Basically, since we're using CRC to validate, we should wait 2 vblanks 
> to get the CRC value that matches the frame where the cursor is 
> committed due to the possibility that the cursor update might come in 
> the next vblank.
> 
> [1] https://patchwork.freedesktop.org/patch/492589/?series=105999&rev=1
> 
> [2] 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L591
> 
>>
>>>           igt_pipe_crc_get_current(data->drm_fd, pipe_crc, hwcrc);
>>> @@ -243,8 +243,8 @@ static void do_single_test(data_t *data, int x, 
>>> int y, bool hw_test,
>>>           igt_plane_set_fb(data->primary, &data->primary_fb[swbufidx]);
>>>           igt_display_commit(display);
>>> -        igt_wait_for_vblank(data->drm_fd,
>>> -                display->pipes[data->pipe].crtc_offset);
>>> +        igt_wait_for_vblank_count(data->drm_fd,
>>> +                display->pipes[data->pipe].crtc_offset, 2);
>>
>> This change make even less sense to me than above. There's nothing 
>> cursor plane related on this part of test. You are saying flipping 
>> normal framebuffers on non cursor plane take also more than one frame 
>> on your target device? It sound like vblank counting is somehow broken 
>> on your target device if these changes together fix something.
> 
> 2 cursor mode ioctls are actually called during the igt_display_commit 
> here. This is because before a SW test is run, disable_cursor is called 
> ([1], for example). This will just set the FB to NULL and position to 
> (0, 0) on IGT side and the following commit (which is the 
> igt_display_commit called here) will then call drmModeSetCursor and 
> drmModeMoveCursor [2] to disable the cursor plane + reset position on 
> driver side.
> 
> So, like with the HW test, we should wait for 2 vblanks to get the 
> corresponding CRC value.
> 
> Thanks,
> 
> Jessica Zhang
> 
> [1] 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_cursor_crc.c#L561
> 
> [2] 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_kms.c#L3239
> 
>>
>> /Juha-Pekka
>>
>>>           igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>>>           igt_assert_crc_equal(&crc, hwcrc);
>>