From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> To: "Srinivas, Vidya" <vidya.srinivas@intel.com>, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org> Cc: "Lin, Charlton" <charlton.lin@intel.com> Subject: Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC Date: Tue, 8 Jun 2021 14:50:45 +0300 [thread overview] Message-ID: <dc3df242-57b9-13f9-c5ab-42acadc39c86@gmail.com> (raw) In-Reply-To: <BY5PR11MB437298B9B622322E6663495E89379@BY5PR11MB4372.namprd11.prod.outlook.com> On 8.6.2021 12.19, Srinivas, Vidya wrote: > 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 This change is ok. It kind of implies there maybe is some problem on your platform with starting of crc calculation but if this is only place where it will show I'm ok with that since crc will not affect normal users in any way. I noticed your new patch, lets see how all ci machines behave on that before doing anything else. /Juha-Pekka > > 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@gmail.com; Modem, Bhanuprakash <Bhanuprakash.Modem@intel.com>; intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org > Cc: Lin, Charlton <Charlton.Lin@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@gmail.com> > Sent: Tuesday, June 8, 2021 1:04 PM > To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org > Cc: Lin, Charlton <charlton.lin@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@lists.freedesktop.org> On Behalf >>> Of Vidya Srinivas >>> Sent: Friday, May 28, 2021 9:57 AM >>> To: intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org >>> Cc: markyacoub@chromium.org; Lin, Charlton <charlton.lin@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@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@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-dev@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
WARNING: multiple messages have this Message-ID (diff)
From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> To: "Srinivas, Vidya" <vidya.srinivas@intel.com>, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org> Cc: "Lin, Charlton" <charlton.lin@intel.com> Subject: Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC Date: Tue, 8 Jun 2021 14:50:45 +0300 [thread overview] Message-ID: <dc3df242-57b9-13f9-c5ab-42acadc39c86@gmail.com> (raw) In-Reply-To: <BY5PR11MB437298B9B622322E6663495E89379@BY5PR11MB4372.namprd11.prod.outlook.com> On 8.6.2021 12.19, Srinivas, Vidya wrote: > 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 This change is ok. It kind of implies there maybe is some problem on your platform with starting of crc calculation but if this is only place where it will show I'm ok with that since crc will not affect normal users in any way. I noticed your new patch, lets see how all ci machines behave on that before doing anything else. /Juha-Pekka > > 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@gmail.com; Modem, Bhanuprakash <Bhanuprakash.Modem@intel.com>; intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org > Cc: Lin, Charlton <Charlton.Lin@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@gmail.com> > Sent: Tuesday, June 8, 2021 1:04 PM > To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org > Cc: Lin, Charlton <charlton.lin@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@lists.freedesktop.org> On Behalf >>> Of Vidya Srinivas >>> Sent: Friday, May 28, 2021 9:57 AM >>> To: intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org >>> Cc: markyacoub@chromium.org; Lin, Charlton <charlton.lin@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@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@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-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/igt-dev >> > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
next prev parent reply other threads:[~2021-06-08 11:50 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-28 4:27 [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC Vidya Srinivas 2021-05-28 4:27 ` [igt-dev] " Vidya Srinivas 2021-05-28 13:25 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_big_fb: Wait for vblank before collecting CRC (rev5) Patchwork 2021-05-28 22:58 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 2021-06-04 18:50 ` [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank before collecting CRC Mark Yacoub 2021-06-04 18:50 ` [igt-dev] " Mark Yacoub 2021-06-05 5:46 ` [Intel-gfx] " Srinivas, Vidya 2021-06-08 7:01 ` Modem, Bhanuprakash 2021-06-08 7:01 ` [igt-dev] " Modem, Bhanuprakash 2021-06-08 7:34 ` [Intel-gfx] [igt-dev] " Juha-Pekka Heikkila 2021-06-08 7:48 ` Srinivas, Vidya 2021-06-08 9:19 ` Srinivas, Vidya 2021-06-08 9:19 ` [igt-dev] [Intel-gfx] " Srinivas, Vidya 2021-06-08 11:50 ` Juha-Pekka Heikkila [this message] 2021-06-08 11:50 ` Juha-Pekka Heikkila 2021-06-08 11:54 ` [Intel-gfx] [igt-dev] " Srinivas, Vidya 2021-06-10 8:38 ` Srinivas, Vidya 2021-06-10 8:38 ` [igt-dev] [Intel-gfx] " Srinivas, Vidya 2021-06-10 8:51 ` [Intel-gfx] [igt-dev] " Petri Latvala 2021-06-10 8:51 ` [igt-dev] [Intel-gfx] " Petri Latvala 2021-06-10 8:52 ` [Intel-gfx] [igt-dev] " Srinivas, Vidya 2021-06-08 9:05 ` [igt-dev] [PATCH i-g-t] tests/kms_big_fb: Use igt_pipe_crc_get_current instead of igt_pipe_crc_collect_crc Vidya Srinivas 2021-06-10 8:52 ` Juha-Pekka Heikkila 2021-06-10 8:53 ` Srinivas, Vidya 2021-06-15 8:28 ` Srinivas, Vidya 2021-06-15 8:50 ` Petri Latvala 2021-06-15 8:50 ` Srinivas, Vidya 2021-06-08 10:54 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_big_fb: Wait for vblank before collecting CRC (rev6) Patchwork 2021-06-08 15:28 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 2021-06-10 5:01 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_big_fb: Wait for vblank before collecting CRC (rev7) Patchwork 2021-06-10 6:27 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=dc3df242-57b9-13f9-c5ab-42acadc39c86@gmail.com \ --to=juhapekka.heikkila@gmail.com \ --cc=bhanuprakash.modem@intel.com \ --cc=charlton.lin@intel.com \ --cc=igt-dev@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=vidya.srinivas@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.