Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_dp_dsc: Add a subtest to force DSC output BPP

[Manasi Navare <manasi.d.navare@intel.com>]

On Mon, Jun 10, 2019 at 04:26:17PM -0700, Anusha Srivatsa wrote:
> This subtest uses the accepted DSC BPPs and tries to
> force a modeset by setting a certain BPP as the output
> BPP for a connector.
> 
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  tests/kms_dp_dsc.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> index 7f2bf276..f936eb33 100644
> --- a/tests/kms_dp_dsc.c
> +++ b/tests/kms_dp_dsc.c
> @@ -46,7 +46,8 @@
>  
>  enum dsc_test_type
>  {
> -	test_basic_dsc_enable
> +	test_basic_dsc_enable,
> +	test_basic_dsc_enable_bpp
>  };
>  
>  typedef struct {
> @@ -67,6 +68,7 @@ typedef struct {
>  
>  bool force_dsc_en_orig;
>  int force_dsc_restore_fd = -1;
> +int new_bpp;
>  
>  static inline void manual(const char *expected)
>  {
> @@ -157,6 +159,17 @@ static void restore_force_dsc_en(void)
>  	force_dsc_restore_fd = -1;
>  }
>  
> +static void force_dp_dsc_enable_bpp(data_t *data)
> +{
> +	char file_name[128] = {0};
> +	char buffer[20];
> +	sprintf(buffer, "%d", new_bpp);
> +	strcpy(file_name, data->conn_name);
> +	strcat(file_name, "/i915_dsc_bpp_slice_support");
> +	igt_debug ("Forcing DSC BPP to %d on %s\n", new_bpp, data->conn_name);
> +	igt_sysfs_write(data->debugfs_fd, file_name, buffer, sizeof(buffer));

Check the return value of igt_sysfs_write and assert if write failed

> +}
> +
>  static void test_cleanup(data_t *data)
>  {
>  	igt_plane_t *primary;
> @@ -229,6 +242,44 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
>  			     "Default DSC enable failed on Connector: %s Pipe: %s\n",
>  			     data->conn_name,
>  			     kmstest_pipe_name(data->pipe));
> +	} else if (test_type == test_basic_dsc_enable_bpp) {
> +		bool enabled;
> +
> +		igt_debug("DSC is supported on %s\n", data->conn_name);
> +

Unnecessary newline

> +		save_force_dsc_en(data);
> +		force_dp_dsc_enable(data);
> +
> +		igt_debug("Trying to set BPP to %d\n", new_bpp);
> +
> +		force_dp_dsc_enable_bpp(data);
> +
> +		igt_output_set_pipe(data->output, data->pipe);
> +		igt_create_pattern_fb(data->drm_fd, data->mode->hdisplay,
> +				      data->mode->vdisplay,
> +				      DRM_FORMAT_XRGB8888,
> +				      LOCAL_DRM_FORMAT_MOD_NONE,
> +				      &data->fb_test_pattern);
> +		primary = igt_output_get_plane_type(data->output,
> +					    DRM_PLANE_TYPE_PRIMARY);
> +		/* 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
> +		 * pattern has no corruption.
> +		 */
> +		manual("RGB test pattern without corruption");
> +
> +		enabled = is_dp_dsc_enabled(data);
> +		restore_force_dsc_en();

Not sure if we need to restore the DSC BPP value? What does the kernel code set the force_dsc_bpp value to if
not forced by IGT?

> +
> +		igt_assert_f(enabled,
> +			     "Default DSC BPP enable failed on Connector: %s Pipe: %s\n",
> +			     data->conn_name,
> +			     kmstest_pipe_name(data->pipe));
> +
>  	} else {
>  		igt_assert(!"Unknown test type\n");
>  	}
> @@ -256,7 +307,9 @@ igt_main
>  	igt_output_t *output;
>  	drmModeRes *res;
>  	drmModeConnector *connector;
> -	int i, test_conn_cnt, test_cnt;
> +	int i, j, test_conn_cnt, test_cnt;
> +	int dp_dsc_supported_compressed_bpp[] = {8, 10, 12, 15};

 May be a add a comment here saying these are the supported compressed BPPs on Gen 11

> +
>  	int tests[] = {DRM_MODE_CONNECTOR_eDP, DRM_MODE_CONNECTOR_DisplayPort};
>  
>  	igt_fixture {
> @@ -300,6 +353,43 @@ igt_main
>  			}
>  			igt_skip_on(test_conn_cnt == 0);
>  		}
> +
> +		for (j = 0; j < ARRAY_SIZE(dp_dsc_supported_compressed_bpp); j++) {
> +			new_bpp = dp_dsc_supported_compressed_bpp[j];
> +			igt_subtest_f("basic-dsc-enable-%dbpp-%s", new_bpp,
> +				      kmstest_connector_type_str(tests[test_cnt])) {
> +			test_conn_cnt = 0;

Check the indentation for the code within the igt_subtest_f, it needs one more tab

> +			for (i = 0; i < res->count_connectors; i++) {
> +				connector = drmModeGetConnectorCurrent(data.drm_fd,
> +								       res->connectors[i]);
> +				if (connector->connection != DRM_MODE_CONNECTED ||
> +				    connector->connector_type !=
> +				    tests[test_cnt])
> +					continue;
> +				output = igt_output_from_connector(&data.display, connector);
> +				sprintf(data.conn_name, "%s-%d",
> +					kmstest_connector_type_str(connector->connector_type),
> +					connector->connector_type_id);
> +				printf("connector name: %s test_conn_count %d\n", data.conn_name, test_conn_cnt++);

printf is redundant here, if you want you could have this in igt_debug so its printed only for debug

> +				if(!is_dp_dsc_supported(&data)) {
> +				   igt_debug("DSC not supported on connector %s \n",
> +					     data.conn_name);
> +					continue;
> +                                 }
> +				if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> +				    !is_dp_fec_supported(&data)) {
> +					igt_debug("DSC cannot be enabled without FEC on %s\n",
> +						  data.conn_name);
> +					continue;
> +				}
> +

Redundant newline

Manasi


> +				test_conn_cnt++;
> +				run_test(&data, output, test_basic_dsc_enable_bpp);
> +				}
> +				igt_skip_on(test_conn_cnt == 0);
> +			}
> +		}
> +
>  	}
>  
>  	igt_fixture {
> -- 
> 2.17.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev