From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id A1DB06ED16 for ; Fri, 30 Oct 2020 09:07:24 +0000 (UTC) From: "Kahola, Mika" Date: Fri, 30 Oct 2020 09:07:22 +0000 Message-ID: <26cb550102b54cd1951fba7654bd0e3c@intel.com> References: <20201026130811.11136-1-juhapekka.heikkila@gmail.com> <9fd4258d-82a0-5bc6-8bf7-87896c53d379@gmail.com> In-Reply-To: <9fd4258d-82a0-5bc6-8bf7-87896c53d379@gmail.com> Content-Language: en-US MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (-clamping) tests List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "juhapekka.heikkila@gmail.com" , "igt-dev@lists.freedesktop.org" List-ID: > -----Original Message----- > From: Juha-Pekka Heikkila > Sent: Thursday, October 29, 2020 8:23 PM > To: Kahola, Mika ; igt-dev@lists.freedesktop.org > Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format (- > clamping) tests > > On 29.10.2020 14.43, Kahola, Mika wrote: > >> -----Original Message----- > >> From: igt-dev On Behalf Of > >> Juha- Pekka Heikkila > >> Sent: Monday, October 26, 2020 3:08 PM > >> To: igt-dev@lists.freedesktop.org > >> Subject: [igt-dev] [PATCH i-g-t] tests/kms_plane: fix pixel format > >> (-clamping) tests > >> > >> cdclk caused modesets mid test will mess up crc sequence where two > >> consecutive frames will get same crc. If there come modeset restart > >> round for given set as modeset will anyway happen only on first color. > >> > >> Signed-off-by: Juha-Pekka Heikkila > >> --- > >> tests/kms_plane.c | 28 ++++++++++++++++++++++------ > >> 1 file changed, 22 insertions(+), 6 deletions(-) > >> > >> diff --git a/tests/kms_plane.c b/tests/kms_plane.c index > >> e42c71cd9..4ce859fba 100644 > >> --- a/tests/kms_plane.c > >> +++ b/tests/kms_plane.c > >> @@ -560,9 +560,11 @@ static void capture_format_crcs(data_t *data, > >> enum pipe pipe, > >> struct drm_event_vblank ev; > >> int i; > >> > >> +restart_round: > >> for (i = 0; i < data->num_colors; i++) { > >> const color_t *c = &data->colors[i]; > >> struct igt_fb old_fb = *fb; > >> + int ret; > >> > >> prepare_format_color(data, pipe, plane, format, modifier, > >> width, height, encoding, range, c, fb); @@ > >> -593,10 +595,22 @@ static void capture_format_crcs(data_t *data, enum > >> pipe pipe, > >> * to be prepared in parallel while the current fb > >> * awaits to be latched. > >> */ > >> - igt_display_commit_atomic(&data->display, > >> - > >> DRM_MODE_ATOMIC_ALLOW_MODESET | > >> - > >> DRM_MODE_ATOMIC_NONBLOCK | > >> - > >> DRM_MODE_PAGE_FLIP_EVENT, NULL); > >> + ret = igt_display_try_commit_atomic(&data->display, > >> + > >> DRM_MODE_ATOMIC_NONBLOCK | > >> + > >> DRM_MODE_PAGE_FLIP_EVENT, NULL); > >> + if (ret) { > >> + /* > >> + * there was needed modeset for pixel > >> format. > >> + * modeset here happen only on first color of > >> + * given set so restart round as modeset will > >> + * mess up crc frame sequence. > >> + */ > >> + igt_display_commit_atomic(&data->display, > >> + > >> DRM_MODE_ATOMIC_ALLOW_MODESET, > >> + NULL); > >> + igt_remove_fb(data->drm_fd, &old_fb); > >> + goto restart_round; > >> + } > > > > I tried to figure out the patch and it seems that we need to do modeset > only once, right? However, there is a theoretical chance (probability maybe > next to nothing) that we keep looping on restart_found. Therefore, I was > thinking that we should limit the restarts to 1 or to some finite number? > > You mean there would come more than one modeset? I think many tests will > start to fail at that moment. Here fb which required modeset is left on screen > when going for restart, there shouldn't be new modeset. Then if modeset > itself fail that modesetting commit is not try commit so it will assert on > failure. I don't think there is need for additional counter unless cdclk > calculations fail but then everything is failing everywhere anyway I think. That's what I was thinking that there might be a minor likelihood that more than one modeset would occur. If that happens, something else must have gone wrong. Anyway, the patch looks ok. Reviewed-by: Mika Kahola > > > > > Cheers, > > Mika > >> } else { > >> /* > >> * Last moment to grab the previous crc @@ -815,8 > >> +829,10 @@ static bool test_format_plane(data_t *data, enum pipe > >> +pipe, > >> > >> igt_plane_set_fb(plane, &test_fb); > >> > >> - ret = igt_display_try_commit_atomic(&data->display, > >> DRM_MODE_ATOMIC_TEST_ONLY, NULL); > >> - > >> + ret = igt_display_try_commit_atomic(&data->display, > >> + > >> DRM_MODE_ATOMIC_TEST_ONLY | > >> + > >> DRM_MODE_ATOMIC_ALLOW_MODESET, > >> + NULL); > >> if (!ret) { > >> width = test_fb.width; > >> height = test_fb.height; > >> -- > >> 2.28.0 > >> > >> _______________________________________________ > >> 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