Re: [igt-dev] [PATCH i-g-t v4] tests/i915/kms_dsc: IGT cleanup

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

Hi Swati,

Overall, this patch looks good to me. I have dropped few minor comments 
to be addressed inline.

On Thu-19-05-2022 11:02 am, Swati Sharma wrote:
> Remove redundant code and before starting the subtest,
> clean up the states to default by igt_display_reset().
> Few minor fixes in indentation. Added subtests description.
> 
> v2: -minor mistake in subtest name
>      -commit was missing after reset, added
> v3: -fixed styling error
>      -replaced drm calls with igt wrappers
> v4: -added igt_display_require_output() in igt_fixture
>      -modularized code, added func() for checks
>      -added func() to get highest mode
> 
> Signed-off-by: Patnana Venkata Sai <venkata.sai.patnana@intel.com>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>   lib/igt_kms.c        |   1 -
>   tests/i915/kms_dsc.c | 263 +++++++++++++++++++++++--------------------
>   2 files changed, 139 insertions(+), 125 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 7838ff28..50a965ad 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -5304,7 +5304,6 @@ bool igt_is_dsc_supported(int drmfd, drmModeConnector *connector)
>    */
>   bool igt_is_fec_supported(int drmfd, drmModeConnector *connector)
>   {
> -
>   	return check_dsc_debugfs(drmfd, connector, "FEC_Sink_Support: yes");
>   }
>   
> diff --git a/tests/i915/kms_dsc.c b/tests/i915/kms_dsc.c
> index 22d2216e..3adca600 100644
> --- a/tests/i915/kms_dsc.c
> +++ b/tests/i915/kms_dsc.c
> @@ -44,13 +44,16 @@
>   #include <fcntl.h>
>   #include <termios.h>
>   
> -/* currently dsc compression is verifying on 8bpc frame only */
> +IGT_TEST_DESCRIPTION("Test to validate display stream compression");
> +
> +#define HDISPLAY_5K	5120
> +
> +/* currently dsc is verifed on 8bpc frame only */
>   #define XRGB8888_DRM_FORMAT_MIN_BPP 8
>   
> -enum dsc_test_type
> -{
> -	test_basic_dsc_enable,
> -	test_dsc_compression_bpp
> +enum dsc_test_type {
> +	TEST_BASIC_DSC_ENABLE,
> +	TEST_DSC_COMPRESSION_BPP
>   };
>   
>   typedef struct {
> @@ -59,9 +62,6 @@ typedef struct {
>   	igt_display_t display;
>   	struct igt_fb fb_test_pattern;
>   	igt_output_t *output;
> -	int mode_valid;
> -	drmModeEncoder *encoder;
> -	int crtc;
>   	int compression_bpp;
>   	int n_pipes;
>   	enum pipe pipe;
> @@ -80,9 +80,9 @@ static void force_dsc_enable(data_t *data)
>   {
>   	int ret;
>   
> -	igt_debug ("Forcing DSC enable on %s\n", data->conn_name);
> +	igt_debug("Forcing DSC enable on %s\n", data->conn_name);
>   	ret = igt_force_dsc_enable(data->drm_fd,
> -				      data->output->config.connector);
> +				   data->output->config.connector);
>   	igt_assert_f(ret > 0, "debugfs_write failed");
>   }
>   
> @@ -93,8 +93,8 @@ static void force_dsc_enable_bpp(data_t *data)
>   	igt_debug("Forcing DSC BPP to %d on %s\n",
>   		  data->compression_bpp, data->conn_name);
>   	ret = igt_force_dsc_enable_bpp(data->drm_fd,
> -					  data->output->config.connector,
> -					  data->compression_bpp);
> +				       data->output->config.connector,
> +				       data->compression_bpp);
>   	igt_assert_f(ret > 0, "debugfs_write failed");
>   }
>   
> @@ -105,7 +105,7 @@ static void save_force_dsc_en(data_t *data)
>   					 data->output->config.connector);
>   	force_dsc_restore_fd =
>   		igt_get_dsc_debugfs_fd(data->drm_fd,
> -					  data->output->config.connector);
> +				       data->output->config.connector);
>   	igt_assert(force_dsc_restore_fd >= 0);
>   }
>   
> @@ -121,19 +121,6 @@ static void restore_force_dsc_en(void)
>   	force_dsc_restore_fd = -1;
>   }
>   
> -static void test_cleanup(data_t *data)
> -{
> -	igt_plane_t *primary;
> -
> -	if (data->output) {
> -		primary = igt_output_get_plane_type(data->output,
> -						    DRM_PLANE_TYPE_PRIMARY);
> -		igt_plane_set_fb(primary, NULL);
> -		igt_output_set_pipe(data->output, PIPE_NONE);
> -		igt_display_commit(&data->display);
> -	}
> -}
> -
>   static void kms_dsc_exit_handler(int sig)
>   {
>   	restore_force_dsc_en();
> @@ -159,29 +146,26 @@ static int sort_drm_modes(const void *a, const void *b)
>   	return (mode1->clock < mode2->clock) - (mode2->clock < mode1->clock);
>   }
>   
> -static bool check_dsc_on_connector(data_t *data, uint32_t drmConnector)
> +static drmModeModeInfo *get_highres_mode(drmModeConnector *connector)
>   {
> -	drmModeConnector *connector;
> -	igt_output_t *output;
> -
> -	connector = drmModeGetConnectorCurrent(data->drm_fd,
> -					       drmConnector);
> -	if (connector->connection != DRM_MODE_CONNECTED)
> -		return false;
> -
> -	output = igt_output_from_connector(&data->display, connector);
> +	drmModeModeInfo *highest_mode = NULL;
>   
> -	/*
> -	 * As dsc supports >= 5k modes, we need to suppress lower
> -	 * resolutions.
> -	 */
> -	qsort(output->config.connector->modes,
> -	      output->config.connector->count_modes,
> +	qsort(connector->modes,
> +	      connector->count_modes,
>   	      sizeof(drmModeModeInfo),
>   	      sort_drm_modes);
> -	if (output->config.connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> -	    output->config.connector->modes[0].hdisplay < 5120)
> -		return NULL;
> +
> +	highest_mode = &connector->modes[0];
> +
> +	return highest_mode;
> +}
> +
> +static bool check_dsc_on_connector(data_t *data, igt_output_t *output)
> +{
> +	drmModeConnector *connector = output->config.connector;
> +
> +	if (connector->connection != DRM_MODE_CONNECTED)
> +		return false;

This check is redundant, for_each_pipe_with_valid_output() is already 
doing this check.

>   
>   	sprintf(data->conn_name, "%s-%d",

Please drop data->conn_name, instead please use data->output->name;

>   		kmstest_connector_type_str(connector->connector_type),
> @@ -192,168 +176,199 @@ static bool check_dsc_on_connector(data_t *data, uint32_t drmConnector)
>   			  data->conn_name);
>   		return false;
>   	}
> +
>   	if (is_external_panel(connector) &&
>   	    !igt_is_fec_supported(data->drm_fd, connector)) {
>   		igt_debug("DSC cannot be enabled without FEC on %s\n",
>   			  data->conn_name);
>   		return false;
>   	}
> +
>   	data->output = output;

This could be moved to test_dsc()

>   	return true;
>   }
>   
> -/*
> - * Re-probe connectors and do a modeset with DSC
> - *
> - */
> -static void update_display(data_t *data, enum dsc_test_type test_type)
> +static bool check_big_joiner_test_constraint(data_t *data, igt_output_t *output,
> +					     enum dsc_test_type test_type)
> +{
> +	drmModeConnector *connector = output->config.connector;
> +	drmModeModeInfo *mode = get_highres_mode(connector);
> +
> +	if (test_type == TEST_DSC_COMPRESSION_BPP &&
> +	    mode->hdisplay >= HDISPLAY_5K) {
> +		igt_debug("Bigjoiner does not support force bpp\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool check_big_joiner_pipe_constraint(data_t *data, igt_output_t *output,
> +					     enum pipe pipe)
> +{
> +	drmModeConnector *connector = output->config.connector;
> +	drmModeModeInfo *mode = get_highres_mode(connector);
> +
> +	if (mode->hdisplay >= HDISPLAY_5K &&
> +	    pipe == (data->n_pipes - 1 )) {
> +		igt_debug("pipe-%s not supported due to bigjoiner limitation\n",
> +			   kmstest_pipe_name(pipe));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool check_dp_gen11_constraint(data_t *data, igt_output_t *output, enum pipe pipe)
> +{
> +	uint32_t devid = intel_get_drm_devid(data->drm_fd);
> +	drmModeConnector *connector = output->config.connector;
> +
> +	if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) &&
> +	    (pipe == PIPE_A) && IS_GEN11(devid)) {
> +		igt_debug("DSC not supported on pipe A on external DP in gen11 platforms\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +/* re-probe connectors and do a modeset with DSC */
> +static void update_display(data_t *data, igt_output_t *output, enum pipe pipe,
> +			   enum dsc_test_type test_type)
>   {
>   	bool enabled;
>   	igt_plane_t *primary;
> +	drmModeModeInfo *mode;
> +	igt_display_t *display = &data->display;
> +	drmModeConnector *connector = data->output->config.connector;
>   
> -	/* Disable the output first */
> -	igt_output_set_pipe(data->output, PIPE_NONE);
> -	igt_display_commit(&data->display);
> +	/* sanitize the state before starting the subtest. */
> +	igt_display_reset(display);
> +	igt_display_commit(display);
>   
>   	igt_debug("DSC is supported on %s\n", data->conn_name);
>   	save_force_dsc_en(data);
>   	force_dsc_enable(data);
> -	if (test_type == test_dsc_compression_bpp) {
> +
> +	if (test_type == TEST_DSC_COMPRESSION_BPP) {
>   		igt_debug("Trying to set BPP to %d\n", data->compression_bpp);
>   		force_dsc_enable_bpp(data);
>   	}
>   
> -	igt_output_set_pipe(data->output, data->pipe);
> -	qsort(data->output->config.connector->modes,
> -			data->output->config.connector->count_modes,
> -			sizeof(drmModeModeInfo),
> -			sort_drm_modes);
> -	igt_output_override_mode(data->output, &data->output->config.connector->modes[0]);
> +	igt_output_set_pipe(data->output, pipe);
> +
> +	mode = get_highres_mode(connector);
> +	igt_require(mode != NULL);
> +	igt_output_override_mode(data->output, mode);
> +
>   	primary = igt_output_get_plane_type(data->output,
>   					    DRM_PLANE_TYPE_PRIMARY);
> +	igt_create_pattern_fb(data->drm_fd,
> +			      mode->hdisplay,
> +			      mode->vdisplay,
> +			      DRM_FORMAT_XRGB8888,
> +			      DRM_FORMAT_MOD_LINEAR,
> +			      &data->fb_test_pattern);
>   
> -	/* Now set the output to the desired mode */
>   	igt_plane_set_fb(primary, &data->fb_test_pattern);
>   	igt_display_commit(&data->display);
>   
>   	/*
> -	 * Until we have CRC check support, manually check if RGB test
> +	 * until we have CRC check support, manually check if RGB test
>   	 * pattern has no corruption.
>   	 */
>   	manual("RGB test pattern without corruption");
>   
> -	enabled = igt_is_dsc_enabled(data->drm_fd,
> -					data->output->config.connector);
> +	enabled = igt_is_dsc_enabled(data->drm_fd, connector);
>   	restore_force_dsc_en();
>   	igt_debug("Reset compression BPP\n");
>   	data->compression_bpp = 0;
>   	force_dsc_enable_bpp(data);
>   
>   	igt_assert_f(enabled,
> -		     "Default DSC enable failed on Connector: %s Pipe: %s\n",
> +		     "Default DSC enable failed on connector: %s pipe: %s\n",
>   		     data->conn_name,
> -		     kmstest_pipe_name(data->pipe));
> +		     kmstest_pipe_name(pipe));
> +
> +	igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> +	igt_plane_set_fb(primary, NULL);
> +	igt_output_set_pipe(data->output, PIPE_NONE);

Please do commit here.

>   }
>   
> -static void run_test(data_t *data, enum dsc_test_type test_type)
> +static void test_dsc(data_t *data, enum dsc_test_type test_type)
>   {
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
>   	enum pipe pipe;
>   	char test_name[10];
>   
> -	igt_skip_on_f(test_type == test_dsc_compression_bpp &&
> -		      data->output->config.connector->modes[0].hdisplay >= 5120,
> -		      "bigjoiner does not support force bpp\n");
> -
> -	igt_create_pattern_fb(data->drm_fd,
> -			      data->output->config.connector->modes[0].hdisplay,
> -			      data->output->config.connector->modes[0].vdisplay,
> -			      DRM_FORMAT_XRGB8888,
> -			      DRM_FORMAT_MOD_LINEAR,
> -			      &data->fb_test_pattern);
> +	for_each_pipe_with_valid_output(display, pipe, output) {
> +		if (!check_dsc_on_connector(data, output))
> +			continue;
>   
> -	for_each_pipe(&data->display, pipe) {
> -		uint32_t devid = intel_get_drm_devid(data->drm_fd);
> +		if (!check_big_joiner_test_constraint(data, output, test_type))
> +			continue;
>   
> -		if (data->output->config.connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> -		    pipe == PIPE_A && IS_GEN11(devid)) {
> -			igt_debug("DSC not supported on Pipe A on external DP in Gen11 platforms\n");
> +		if (!check_dp_gen11_constraint(data, output, pipe))
>   			continue;
> -		}
>   
> -		snprintf(test_name, sizeof(test_name), "-%dbpp", data->compression_bpp);
> -		if (!igt_pipe_connector_valid(pipe, data->output))
> +		if (!check_big_joiner_pipe_constraint(data, output, pipe))
>   			continue;
>   
> -		igt_dynamic_f("%s-pipe-%s%s", data->output->name,
> -			      kmstest_pipe_name(pipe),
> -			      (test_type == test_dsc_compression_bpp) ?
> -			      test_name : "") {
> -			data->pipe = pipe;
> -			igt_skip_on_f((data->output->config.connector->modes[0].hdisplay >= 5120) &&
> -				      (pipe  == (data->n_pipes - 1)),
> -				      "pipe-%s not supported due to bigjoiner limitation\n",
> -				      kmstest_pipe_name(pipe));
> -			update_display(data, test_type);
> +		if (!igt_pipe_connector_valid(pipe, output))
> +			continue;

This check is redundant, for_each_pipe_with_valid_output() is already 
taken care of this check.

>   
> -		}
> -		if (test_type == test_dsc_compression_bpp)
> -			break;
> +		snprintf(test_name, sizeof(test_name), "-%dbpp", data->compression_bpp);
> +		igt_dynamic_f("pipe-%s-%s%s",  kmstest_pipe_name(pipe), output->name,
> +			     (test_type == TEST_DSC_COMPRESSION_BPP) ? test_name : "")
> +		      update_display(data, output, pipe, test_type);
>   	}
> -
> -	igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
>   }
>   
>   igt_main
>   {
>   	data_t data = {};
> -	drmModeRes *res;
> -	drmModeConnector *connector = NULL;
> -	int i, j;
> +	int i;
> +
>   	igt_fixture {
>   		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
>   		data.devid = intel_get_drm_devid(data.drm_fd);
>   		kmstest_set_vt_graphics_mode();
>   		igt_install_exit_handler(kms_dsc_exit_handler);
>   		igt_display_require(&data.display, data.drm_fd);
> -		igt_require(res = drmModeGetResources(data.drm_fd));
> +		igt_display_require_output(&data.display);
>   		data.n_pipes = 0;
>   		for_each_pipe(&data.display, i)
>   			data.n_pipes++;
>   	}
> -	igt_subtest_with_dynamic("basic-dsc-enable") {
> -		for (j = 0; j < res->count_connectors; j++) {
> -			if (!check_dsc_on_connector(&data, res->connectors[j]))
> -				continue;
> -			run_test(&data, test_basic_dsc_enable);
> -		}
> -	}
> +
> +	igt_describe("Tests basic display stream compression functionality if supported "
> +		     "by a connector by forcing DSC on all connectors that support it "
> +		     "with default parameters");
> +	igt_subtest_with_dynamic("basic-dsc-enable")
> +			test_dsc(&data, TEST_BASIC_DSC_ENABLE);
> +
>   	/* currently we are validating compression bpp on XRGB8888 format only */
> +	igt_describe("Tests basic display stream compression functionality if supported "
> +		     "by a connector by forcing DSC on all connectors that support it "
> +		     "with certain BPP as the output BPP for the connector");
>   	igt_subtest_with_dynamic("XRGB8888-dsc-compression") {
>   		uint32_t bpp_list[] = {
>   			XRGB8888_DRM_FORMAT_MIN_BPP,
>   			(XRGB8888_DRM_FORMAT_MIN_BPP  +
> -			 (XRGB8888_DRM_FORMAT_MIN_BPP * 3) - 1) / 2,
> +			(XRGB8888_DRM_FORMAT_MIN_BPP * 3) - 1) / 2,
>   			(XRGB8888_DRM_FORMAT_MIN_BPP * 3) - 1
>   		};
>   
>   		igt_require(intel_display_ver(data.devid) >= 13);

This check can be in igt_fixture?

- Bhanu

>   
> -		for (j = 0; j < res->count_connectors; j++) {
> -			if (!check_dsc_on_connector(&data, res->connectors[j]))
> -				continue;
> -
> -			for (i = 0; i < ARRAY_SIZE(bpp_list); i++) {
> -				data.compression_bpp = bpp_list[i];
> -				run_test(&data, test_dsc_compression_bpp);
> -			}
> +		for (int j = 0; j < ARRAY_SIZE(bpp_list); j++) {
> +			data.compression_bpp = bpp_list[j];
> +			test_dsc(&data, TEST_DSC_COMPRESSION_BPP);
>   		}
>   	}
>   
>   	igt_fixture {
> -		if (connector)
> -			drmModeFreeConnector(connector);
> -		test_cleanup(&data);
> -		drmModeFreeResources(res);
>   		igt_display_fini(&data.display);
>   		close(data.drm_fd);
>   	}