[igt-dev] [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC

[Srinivas, Vidya]

Hello Juha-Pekka

Instead of wait for vblank, this also works
igt_pipe_crc_start-> igt_pipe_crc_get_current for small fb after commit -> then igt_pipe_crc_get_current For big fb -> compare -> igt_pipe_crc_stop

Would this change be okay? Kindly suggest. igt_pipe_crc_collect_crc is not working. It gives CRC mismatch for few subtests like subtest y-tiled-32bpp-rotate-0

Have submitted the change here https://patchwork.freedesktop.org/patch/437657/?series=90389&rev=6

Thank you so much.

Regards
Vidya

-----Original Message-----
From: Srinivas, Vidya 
Sent: Tuesday, June 8, 2021 1:19 PM
To: juhapekka.heikkila at gmail.com; Modem, Bhanuprakash <Bhanuprakash.Modem at intel.com>; intel-gfx at lists.freedesktop.org; igt-dev at lists.freedesktop.org
Cc: Lin, Charlton <Charlton.Lin at intel.com>
Subject: RE: [igt-dev] [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC

Hello Juha-Pekka and Bhanu

Thank you for the review comments. Apologies Juha-Pekka, I will incorporate your review comments and try out.

Regards
Vidya


-----Original Message-----
From: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
Sent: Tuesday, June 8, 2021 1:04 PM
To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; Srinivas, Vidya <vidya.srinivas at intel.com>; intel-gfx at lists.freedesktop.org; igt-dev at lists.freedesktop.org
Cc: Lin, Charlton <charlton.lin at intel.com>
Subject: Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC

On 8.6.2021 10.01, Modem, Bhanuprakash wrote:
>> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf 
>> Of Vidya Srinivas
>> Sent: Friday, May 28, 2021 9:57 AM
>> To: intel-gfx at lists.freedesktop.org; igt-dev at lists.freedesktop.org
>> Cc: markyacoub at chromium.org; Lin, Charlton <charlton.lin at intel.com>
>> Subject: [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for 
>> vblank before collecting CRC
>>
>> Without wait for vblank, CRC mismatch is seen between big and small 
>> CRC on few Gen11 systems.
>>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
>> ---
>>   tests/kms_big_fb.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_big_fb.c b/tests/kms_big_fb.c index
>> b35727a09bd0..f90363c3beb2 100644
>> --- a/tests/kms_big_fb.c
>> +++ b/tests/kms_big_fb.c
>> @@ -254,6 +254,7 @@ static void unset_lut(data_t *data)
>>   static bool test_plane(data_t *data)
>>   {
>>   	igt_plane_t *plane = data->plane;
>> +	igt_display_t *display = &data->display;
> 
> For code readability purpose, I think we need to update to use this 
> variable wherever we are using "&data->display" in this function.
> s/"&data->display"/"display"/
> 
> With above change, this patch LGTM
> Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> 

I still don't see benefit in this patch. For now all this look like is doing is slow down the test and if this actually helps there's a real bug somewhere which is not here. My earlier review comments were not addressed hence repeat here, see below.


>>   	struct igt_fb *small_fb = &data->small_fb;
>>   	struct igt_fb *big_fb = &data->big_fb;
>>   	int w = data->big_fb_width - small_fb->width; @@ -337,16 +338,17 
>> @@ static bool test_plane(data_t *data)
>>   		igt_display_commit2(&data->display, data->display.is_atomic ?
>>   				    COMMIT_ATOMIC : COMMIT_UNIVERSAL);
>>
>> -
>> +		igt_wait_for_vblank(data->drm_fd,
>> +display->pipes[data->pipe].crtc_offset);

Above this line there's flip to different fb. Below this line crc calculation is restarted, get one crc and stop crc. There's several vblanks already spent here, if now adding one more somehow helps it sound like there's problems in crc calculation on some platform or kernel is saying too early framebuffer is ready to be shown. Am I missing something here?

/Juha-Pekka

>>   		igt_pipe_crc_collect_crc(data->pipe_crc, &small_crc);
>>
>>   		igt_plane_set_fb(plane, big_fb);
>>   		igt_fb_set_position(big_fb, plane, x, y);
>>   		igt_fb_set_size(big_fb, plane, small_fb->width, 
>> small_fb->height);
>> +

spurious empty line need to be removed.

>>   		igt_plane_set_size(plane, data->width, data->height);
>>   		igt_display_commit2(&data->display, data->display.is_atomic ?
>>   				    COMMIT_ATOMIC : COMMIT_UNIVERSAL);
>> -
>> +		igt_wait_for_vblank(data->drm_fd, 
>> +display->pipes[data->pipe].crtc_offset);
>>   		igt_pipe_crc_collect_crc(data->pipe_crc, &big_crc);
>>
>>   		igt_plane_set_fb(plane, NULL);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>