Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_invalid_mode: Test Cleanup

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Mon-27-06-2022 12:37 pm, Nidhi Gupta wrote:
> Sanitize the system state before starting the subtest.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   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);
>   	}
>   }