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.heikk...@gmail.com>
Sent: Tuesday, June 8, 2021 1:04 PM
To: Modem, Bhanuprakash <bhanuprakash.mo...@intel.com>; Srinivas, Vidya
<vidya.srini...@intel.com>; intel-gfx@lists.freedesktop.org;
igt-...@lists.freedesktop.org
Cc: Lin, Charlton <charlton....@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-boun...@lists.freedesktop.org> On Behalf
>> Of Vidya Srinivas
>> Sent: Friday, May 28, 2021 9:57 AM
>> To: intel-gfx@lists.freedesktop.org; igt-...@lists.freedesktop.org
>> Cc: markyac...@chromium.org; Lin, Charlton <charlton....@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.srini...@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.mo...@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> igt-dev mailing list
> igt-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
