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 F31FD10E131 for ; Wed, 17 May 2023 09:01:45 +0000 (UTC) Message-ID: <6e4e2fa2-1e61-53c1-5e45-38d6eb39c39f@intel.com> Date: Wed, 17 May 2023 14:31:30 +0530 To: Arun R Murthy , References: <20230516062206.1064604-1-arun.r.murthy@intel.com> <20230516062206.1064604-4-arun.r.murthy@intel.com> Content-Language: en-US From: Karthik B S In-Reply-To: <20230516062206.1064604-4-arun.r.murthy@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 4/4] tests/kms_async_flips: Test all modifiers List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 5/16/2023 11:52 AM, Arun R Murthy wrote: > Run the basic async flip test for all modifiers reported > by the plane (for the XRGB8888 format). The driver may not > support async flip with all modifiers so we allow this to fail. > > TODO: probably shuould add an even more basic subtest that > makes sure the driver accepts async flips with a least > one format/modifier combo, if it advertises async flip > support > > Signed-off-by: Ville Syrjälä > Signed-off-by: Arun R Murthy > --- > tests/kms_async_flips.c | 53 +++++++++++++++++++++++++++++++++-------- > 1 file changed, 43 insertions(+), 10 deletions(-) > > diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c > index f54bd7ca..3b8cb988 100644 > --- a/tests/kms_async_flips.c > +++ b/tests/kms_async_flips.c > @@ -53,6 +53,8 @@ typedef struct { > igt_output_t *output; > unsigned long flip_timestamp_us; > double flip_interval; > + uint64_t modifier; > + igt_plane_t *plane; > igt_pipe_crc_t *pipe_crc; > igt_crc_t ref_crc; > int flip_count; > @@ -61,6 +63,7 @@ typedef struct { > bool extended; > enum pipe pipe; > bool alternate_sync_async; > + bool allow_fail; > } data_t; > > static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec, > @@ -133,7 +136,7 @@ static void make_fb(data_t *data, struct igt_fb *fb, > rec_width = width / (ARRAY_SIZE(data->bufs) * 2); > > igt_create_color_fb(data->drm_fd, width, height, DRM_FORMAT_XRGB8888, > - default_modifier(data), 0.0, 0.0, 0.5, fb); > + data->modifier, 0.0, 0.0, 0.5, fb); > > cr = igt_get_cairo_ctx(data->drm_fd, fb); > igt_paint_color_rand(cr, rec_width * 2 + rec_width * index, 0, rec_width, height); > @@ -159,24 +162,26 @@ static void test_init(data_t *data) > data->refresh_rate = mode->vrefresh; > > igt_output_set_pipe(data->output, data->pipe); > + > + data->plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY); > } > > static void test_init_fbs(data_t *data) > { > int i; > uint32_t width, height; > - igt_plane_t *plane; > static uint32_t prev_output_id; > + static uint64_t prev_modifier; > drmModeModeInfo *mode; > > mode = igt_output_get_mode(data->output); > width = mode->hdisplay; > height = mode->vdisplay; > > - plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY); > - > - if (prev_output_id != data->output->id) { > + if (prev_output_id != data->output->id || > + prev_modifier != data->modifier) { > prev_output_id = data->output->id; > + prev_modifier = data->modifier; > > if (data->bufs[0].fb_id) { > for (i = 0; i < ARRAY_SIZE(data->bufs); i++) > @@ -187,8 +192,8 @@ static void test_init_fbs(data_t *data) > make_fb(data, &data->bufs[i], width, height, i); > } > > - igt_plane_set_fb(plane, &data->bufs[0]); > - igt_plane_set_size(plane, width, height); > + igt_plane_set_fb(data->plane, &data->bufs[0]); > + igt_plane_set_size(data->plane, width, height); > > igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > } > @@ -243,8 +248,10 @@ static void test_async_flip(data_t *data) > ret = drmModePageFlip(data->drm_fd, data->crtc_id, > data->bufs[frame % 4].fb_id, > flags, data); > - > - igt_assert(ret == 0); > + if (frame == 1 && data->allow_fail) > + igt_skip_on(ret == -EINVAL); > + else > + igt_assert(ret == 0); > > wait_flip_event(data); > > @@ -556,6 +563,8 @@ static void run_test(data_t *data, void (*test)(data_t *)) > continue; > > test_init(data); > + data->allow_fail = false; > + data->modifier = default_modifier(data); > igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(data->pipe), data->output->name) { > test_init_fbs(data); > test(data); > @@ -566,6 +575,30 @@ static void run_test(data_t *data, void (*test)(data_t *)) > } > } > > +static void run_test_with_modifiers(data_t *data, void (*test)(data_t *)) > +{ > + for_each_pipe_with_valid_output(&data->display, data->pipe, data->output) { Hi, Don't we want to run this with all pipes at least once like the other subtests? Any particular reason for this? > + test_init(data); > + > + for (int i = 0; i < data->plane->format_mod_count; i++) { > + if (data->plane->formats[i] != DRM_FORMAT_XRGB8888) > + continue; > + > + data->allow_fail = true; > + data->modifier = data->plane->modifiers[i]; > + > + igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(data->pipe), Please add output name print here as well, like in patch 2. > + igt_fb_modifier_name(data->modifier)) { > + test_init_fbs(data); > + test(data); > + } > + } > + > + if (!data->extended) > + break; Should this be removed similar to patch 2, so that all outputs can be verified even on normal CI run? Thanks, Karthik.B.S > + } > +} > + > static int opt_handler(int opt, int opt_index, void *_data) > { > data_t *data = _data; > @@ -612,7 +645,7 @@ igt_main_args("e", NULL, help_str, opt_handler, &data) > igt_describe("Wait for page flip events in between successive asynchronous flips"); > igt_subtest_with_dynamic("async-flip-with-page-flip-events") { > data.alternate_sync_async = false; > - run_test(&data, test_async_flip); > + run_test_with_modifiers(&data, test_async_flip); > } > > igt_describe("Alternate between sync and async flips");