From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6F30611A17D for ; Mon, 27 Jun 2022 07:32:08 +0000 (UTC) Message-ID: Date: Mon, 27 Jun 2022 13:01:50 +0530 Content-Language: en-US To: Nidhi Gupta , References: <20220627070710.13641-1-nidhi1.gupta@intel.com> <20220627070710.13641-3-nidhi1.gupta@intel.com> From: "Modem, Bhanuprakash" In-Reply-To: <20220627070710.13641-3-nidhi1.gupta@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_invalid_mode: Test Cleanup List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Mon-27-06-2022 12:37 pm, Nidhi Gupta wrote: > Sanitize the system state before starting the subtest. > > Signed-off-by: Nidhi Gupta > --- > tests/kms_invalid_mode.c | 38 +++++++++++--------------------------- > 1 file changed, 11 insertions(+), 27 deletions(-) > > diff --git a/tests/kms_invalid_mode.c b/tests/kms_invalid_mode.c > index b79fc78a..0d76adc1 100644 > --- a/tests/kms_invalid_mode.c > +++ b/tests/kms_invalid_mode.c > @@ -35,7 +35,6 @@ struct _data { > enum pipe pipe; > igt_display_t display; > igt_output_t *output; > - drmModeResPtr res; > int max_dotclock; > bool (*adjust_mode)(data_t *data, drmModeModeInfoPtr mode); > }; > @@ -182,35 +181,23 @@ static void > test_output(data_t *data) > { > igt_output_t *output = data->output; > - drmModeModeInfo mode; > - struct igt_fb fb; > - int ret; > - uint32_t crtc_id; > + drmModeConnector *connector = output->config.connector; > > + igt_display_reset(&data->display); > + igt_output_set_pipe(output, data->pipe); > /* > * FIXME test every mode we have to be more > - * sure everything is really getting rejected? > + e sure everything is really getting rejected? Please drop this comment, since we are trying all connector modes. > */ > - mode = *igt_output_get_mode(output); > - igt_require(data->adjust_mode(data, &mode)); > + igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc); > > - igt_create_fb(data->drm_fd, > - max_t(uint16_t, mode.hdisplay, 64), > - max_t(uint16_t, mode.vdisplay, 64), > - DRM_FORMAT_XRGB8888, > - DRM_FORMAT_MOD_LINEAR, > - &fb); > - > - kmstest_unset_all_crtcs(data->drm_fd, data->res); > - > - crtc_id = data->display.pipes[data->pipe].crtc_id; > - > - ret = drmModeSetCrtc(data->drm_fd, crtc_id, > - fb.fb_id, 0, 0, > - &output->id, 1, &mode); > - igt_assert_lt(ret, 0); > + for_each_connector_mode(output) { What is the impact of CI execution time? If it takes more time, maybe we can limit the execution to few (maybe 5) modes. > + igt_output_override_mode(output, &connector->modes[j__]); > + igt_require(data->adjust_mode(data, &connector->modes[j__])); For readability, please swap above two statements. > + igt_display_commit2(&data->display, &data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > + } > > - igt_remove_fb(data->drm_fd, &fb); Why did you drop this? > + igt_output_override_mode(output, NULL); Please unset the crtc. igt_output_set_pipe(output, NULL); Apart from these minor changes, overall this patch looks good to me. - Bhanu > } > > static int i915_max_dotclock(data_t *data) > @@ -290,8 +277,6 @@ igt_main > kmstest_set_vt_graphics_mode(); > > igt_display_require(&data.display, data.drm_fd); > - data.res = drmModeGetResources(data.drm_fd); > - igt_assert(data.res); > > data.max_dotclock = i915_max_dotclock(&data); > igt_info("Max dotclock: %d kHz\n", data.max_dotclock); > @@ -314,6 +299,5 @@ igt_main > igt_fixture { > igt_display_fini(&data.display); > igt_reset_connectors(); > - drmModeFreeResources(data.res); > } > }