Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP

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

On Wed, Oct 17, 2018 at 01:38:47PM -0700, Antonio Argenziano wrote:
> 
> 
> On 16/10/18 14:37, Manasi Navare wrote:
> >On Tue, Oct 16, 2018 at 02:14:59PM -0700, Antonio Argenziano wrote:
> >>
> >>
> >>On 16/10/18 11:47, Manasi Navare wrote:
> >>>On Wed, Oct 10, 2018 at 08:48:37AM -0700, Antonio Argenziano wrote:
> >>>>
> >>>>
> >>>>On 05/10/18 16:34, Manasi Navare wrote:
> >>>>>This patch adds a basic kms test to validate the display stream
> >>>>>compression functionality if supported on DP/eDP connector.
> >>>>>Currently this has only one subtest to force the DSC on all
> >>>>>the connectors that support it with default parameters.
> >>>>>This will be expanded to add more subtests to tweak DSC parameters.
> >>>>>
> >>>>>Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >>>>>Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >>>>>---
> >>>>>  tests/Makefile.sources |   1 +
> >>>>>  tests/kms_dp_dsc.c     | 372 +++++++++++++++++++++++++++++++++++++++++
> >>>>>  2 files changed, 373 insertions(+)
> >>>>>  create mode 100644 tests/kms_dp_dsc.c
> >>>>>
> >>>>>diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> >>>>>index c84933f1..c807aad3 100644
> >>>>>--- a/tests/Makefile.sources
> >>>>>+++ b/tests/Makefile.sources
> >>>>>@@ -207,6 +207,7 @@ TESTS_progs = \
> >>>>>  	kms_tv_load_detect \
> >>>>>  	kms_universal_plane \
> >>>>>  	kms_vblank \
> >>>>>+	kms_dp_dsc \
> >>>>
> >>>>Add test in meson build as well.
> >>>
> >>>But I dont see any kms tests added in meson.build file, is there any other
> >>>file that adds the test names to meson build?
> >>
> >>tests/meson.build does have kms tests in my tree. I think that is the only
> >>place with tests.
> >
> >Ok got it! Thanks.
> >
> >>
> >>>
> >>>>
> >>>>>  	meta_test \
> >>>>>  	perf \
> >>>>>  	perf_pmu \
> >>>>>diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> >>>>>new file mode 100644
> >>>>>index 00000000..d0fd2ae3
> >>>>>--- /dev/null
> >>>>>+++ b/tests/kms_dp_dsc.c
> >>>>>@@ -0,0 +1,372 @@
> >>
> >>>>>+	size_t line_size = 0;
> >>>>>+	char buf[128] = {0}, supported_str[5], enabled_str[5];
> >>>>>+	bool supported = false;
> >>>>>+
> >>>>>+	strcpy(buf, test_connector_name);
> >>>>>+	strcat(buf, "/i915_dsc_support");
> >>>>>+	dsc_support_fp = fopenat(dir, buf, "r");
> >>>>>+	igt_require(dsc_support_fp);
> >>>>
> >>>>don't we have something similar in igt_debugfs already (igt_debugfs_read)?

I am thinking of replacing the direct fopenat call by igt_debugfs_open() and then
still use the file pointer to pass to getline so that i can scan it line by line.

The problem with using the igt_debugfs_read is that then it copies it in a buffer and then
scanning line by line becomes difficult. That will be easy if there is a single line.

Does this sound okay?

Manasi

> >>>
> >>>Hmm, yea I think I could be able to use igt_debugfs_read and pass the buf which will
> >>>be concatenated str with connector_name/file name Eg: DP-1/i915_dsc_support
> >>>
> >>>But I will still need the below while loop in order to parse the supported and enabled
> >>>strings. I will play with the igt_debugfs_read and strstr to see if I can get rid of this while
> >>>loop below.
> >>>
> >>>>
> >>>>>+
> >>>>>+	while (getline(&line, &line_size, dsc_support_fp) > 0) {
> >>>>>+		char *support, *enabled;
> >>>>>+
> >>>>>+		enabled = strstr(line, "Enabled: ");
> >>>>>+		igt_assert_eq(sscanf(enabled, "Enabled: %s", enabled_str), 1);
> >>>>>+		if (strcmp(enabled_str, "yes") == 0) {
> >>>>>+			igt_info("DSC is enabled on %s\n", test_connector_name);
> >>>>>+			supported = true;
> >>>>>+			break;
> >>>>>+		} else {
> >>>>>+			getline(&line, &line_size, dsc_support_fp);
> >>>>>+			support = strstr(line, "Supported: ");
> >>>>>+		igt_assert_eq(sscanf(support, "Supported: %s", supported_str), 1);
> >>>>>+		if (strcmp(supported_str, "yes") == 0)
> >>>>>+			supported = true;
> >>>>>+		else if (strcmp(supported_str, "no") == 0)
> >>>>>+			supported = false;
> >>>>>+		else
> >>>>>+			igt_fail_on_f(true, "Unknown DSC supported status '%s'\n",
> >>>>>+				      supported_str);
> >>>>>+		break;
> >>>>>+		}https://jira01.devtools.intel.com/browse/GUC-306
> >>>>>+	}
> >>>>>+
> >>>>>+	free(line);
> >>>>>+	fclose(dsc_support_fp);
> >>>>>+
> >>>>>+	return supported;
> >>>>>+}
> >>>>>+
> >>
> >>>>>+
> >>>>>+static int
> >>>>>+set_mode(struct connector *c, struct dsc_config *test_config, bool set_mode)
> >>>>>+{
> >>>>>+	unsigned int fb_id = 0;
> >>>>>+	struct igt_fb fb_info;
> >>>>>+	int ret = 0;
> >>>>>+
> >>>>>+	if (!set_mode) {
> >>>>>+		ret = drmModeSetCrtc(drm_fd, c->crtc, 0, 0, 0,
> >>>>>+				     NULL, 0, NULL);
> >>>>>+		if (ret)
> >>>>>+			igt_warn("Failed to unset mode");
> >>>>>+		return ret;
> >>>>>+	}
> >>>>>+
> >>>>>+	fb_id = igt_create_pattern_fb(drm_fd, test_config->mode_width,
> >>>>>+				      test_config->mode_height,
> >>>>>+				      igt_bpp_depth_to_drm_format(test_config->bpp,
> >>>>>+								  test_config->depth),
> >>>>>+				      tiling, &fb_info);
> >>>>>+
> >>>>>+	igt_info("CRTC(%u):[%d]", c->crtc, 0);
> >>>>>+	kmstest_dump_mode(&c->mode);
> >>>>>+	drmModeSetCrtc(drm_fd, c->crtc, -1, 0, 0, NULL, 0, NULL);
> >>>>>+	ret = drmModeSetCrtc(drm_fd, c->crtc, fb_id, 0, 0,
> >>>>>+			     &c->id, 1, &c->mode);
> >>>>>+	sleep(5);
> >>>>
> >>>>Why the sleep before checking the return code?
> >>>
> >>>This is so that it displays the requested pattern for few secs before cycling to the next
> >>>connector so that it can be validated visually.
> >>>Makes sense?
> >>
> >>How does it fail? Should you wait for user input?
> >
> >Actually this will fail if drmModeSetCrtc fails say in cases where DSC could not be enabled
> >and then encoder config will fail.
> >
> >>
> >>Maybe add a comment for that. Is this test always visually validated? I mean
> >>can we skip the sleep in an automated run like CI for instance.
> >>
> >
> >Yes I will add a comment for this. And yes the DSC output will always need visual validation in terms of corruption etc.
> >But its a good idea to skip the sleep in case of CI. Is there a way to conditionall add sleep or skip sleep()?
> 
> I don't think we have something like that already, a command-line parameter
> maybe?
> 
> Antonio
> 
> >
> >Manasi
> >>>
> >>>>
> >>>>>+	if (ret < 0) {
> >>>>>+		igt_warn("Failed to set mode (%dx%d@%dHz): %s\n",
> >>>>>+			 test_config->mode_width, test_config->mode_height,
> >>>>>+			 c->mode.vrefresh, strerror(errno));
> >>>>>+		igt_remove_fb(drm_fd, &fb_info);
> >>>>>+
> >>>>>+	}
> >>>>>+
> >>>>>+	return ret;
> >>>>>+}
> >>>>>+
> >>>>>+
> >>>>>+/*
> >>>>>+ * Re-probe connectors and do a modeset with and wihout DSC
> >>>>>+ *
> >>>>>+ * Returns:
> >>>>>+ * 0: On Success
> >>>>>+ * -1: On failure
> >>>>>+ */
> >>>>>+static int update_display(struct dsc_config *test_config)
> >>>>>+{
> >>>>>+	struct connector *connectors;
> >>>>>+	struct kmstest_connector_config config;
> >>>>>+	int cnt, conn_cnt = 0, ret = 0, edp_cnt = 0, dp_cnt = 0;
> >>>>>+	unsigned long crtc_idx_mask = -1UL;
> >>>>>+	char conn_buf[128] = {0};
> >>>>>+
> >>>>>+	resources = drmModeGetResources(drm_fd);
> >>>>>+	if (!resources) {
> >>>>>+		igt_warn("drmModeGetResources failed: %s\n", strerror(errno));
> >>>>>+		return -1;
> >>>>>+	}
> >>>>>+
> >>>>>+	connectors = calloc(resources->count_connectors,
> >>>>>+			    sizeof(struct connector));
> >>>>>+	if (!connectors)
> >>>>>+		return -1;
> >>>>>+
> >>>>>+	/* Find any connected displays */
> >>>>>+	for (cnt = 0; cnt < resources->count_connectors; cnt++) {
> >>>>>+		struct connector *c = &connectors[cnt];
> >>>>>+		c->id = resources->connectors[cnt];
> >>>>>+		if(!kmstest_get_connector_config(drm_fd, c->id, crtc_idx_mask,
> >>>>>+						&config)) {
> >>>>>+			c->mode_valid = 0;
> >>>>>+			continue;
> >>>>>+		}
> >>>>>+		c->connector = config.connector;
> >>>>>+		c->encoder = config.encoder;
> >>>>>+		c->mode = config.default_mode;
> >>>>>+		c->crtc = config.crtc->crtc_id;
> >>>>>+		c->pipe = config.pipe;
> >>>>>+		c->mode_valid = 1;
> >>>>>+
> >>>>>+		if (c->connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> >>>>>+		    c->connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> >>>>>+
> >>>>>+			if (c->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> >>>>>+				conn_cnt = ++ edp_cnt;
> >>>>>+			else
> >>>>>+				conn_cnt = ++ dp_cnt;
> >>>>>+			if (c->connector->connection == DRM_MODE_CONNECTED) {
> >>>>>+				sprintf(conn_buf, "%s-%d",
> >>>>>+					kmstest_connector_type_str(c->connector->connector_type),
> >>>>>+					conn_cnt);
> >>>>>+				test_config->dsc_supported =
> >>>>>+					get_dp_dsc_support(conn_buf);
> >>>>>+				if (!strcmp(test_config->test_name,
> >>>>>+					    "force_dsc_enable_basic")) {
> >>>>>+					if (test_config->dsc_supported) {
> >>>>>+						igt_info ("DSC is supported on %s\n",
> >>>>>+							  conn_buf);
> >>>>>+						test_config->mode_width = c->mode.hdisplay;
> >>>>>+						test_config->mode_height = c->mode.vdisplay;
> >>>>>+						test_config->bpp = 32;
> >>>>>+						test_config->depth = 24;
> >>>>>+						set_dp_dsc_enable(test_config->dsc_enable,
> >>>>>+							  conn_buf);
> >>>>>+						ret = set_mode(c, test_config,
> >>>>>+							       true);
> >>>>>+					} else {
> >>>>>+						igt_info ("DSC is not supported on %s\n",
> >>>>>+							  conn_buf);
> >>>>>+					}
> >>>>>+				}
> >>>>>+				crtc_idx_mask &= ~(1 << c->pipe);
> >>>>>+			}
> >>>>>+		}
> >>>>>+	}
> >>>>>+
> >>>>>+	free(connectors);
> >>>>>+	drmModeFreeResources(resources);
> >>>>>+	return ret;
> >>>>>+}
> >>>>>+
> >>>>>+static int opt_handler(int opt, int opt_index, void *opt_dump)
> >>>>>+{
> >>>>
> >>>>Why not wrapping all the optional prints in igt_debug so you could re-use
> >>>>the --debug option?
> >>>
> >>>You mean instead of igt_info and all connector dump prints have them in igt_debug() ?
> >>
> >>Something like that or returning a string from the dump functions and pint
> >>it into an igt_debug() call.
> >>
> >>>
> >>>>
> >>>>>+	bool *dump = opt_dump;
> >>>>>+
> >>>>>+	switch (opt) {
> >>>>>+	case 'i':
> >>>>>+		*dump = true;
> >>>>>+		break;
> >>>>>+	default:
> >>>>>+		igt_assert(0);
> >>>>>+	}
> >>>>>+
> >>>>>+	return 0;
> >>>>>+}
> >>>>>+
> >>>>>+int main(int argc, char *argv[])
> >>>>>+{
> >>>>>+	const char *help_str =
> >>>>>+		"  --info\t\tDump Information about connectors. (still needs DRM access)\n";
> >>>>>+	static struct option long_options[] = {
> >>>>>+		{"info", 0, 0, 'i'},
> >>>>>+		{0, 0, 0, 0}
> >>>>>+	};
> >>>>>+	int ret = 0;
> >>>>>+	struct dsc_config test_config;
> >>>>>+	bool opt_dump = false;
> >>>>>+	igt_subtest_init_parse_opts(&argc, argv, "", long_options, help_str,
> >>>>>+				    opt_handler, &opt_dump);
> >>>>>+
> >>>>>+	igt_skip_on_simulation();
> >>>>>+
> >>>>>+	igt_fixture {
> >>>>>+		kmstest_set_vt_graphics_mode();
> >>>>>+		drm_fd = drm_open_driver(DRIVER_ANY);
> >>>>>+		if (opt_dump)
> >>>>>+			dump_info();
> >>>>>+	}
> >>>>>+
> >>>>>+	igt_subtest("force_dsc_enable_basic") {
> >>>>>+		strcpy(test_config.test_name,"force_dsc_enable_basic");
> >>>>>+		ret = update_display(&test_config);
> >>>>
> >>>>'test_config' seems to be an output parameter but it is not used in the
> >>>>test.
> >>>
> >>>test_config is used in the update_display()
> >>>
> >>>>
> >>>>>+		igt_assert_eq(ret, 0);
> >>>>>+	}
> >>>>>+
> >>>>>+	close(drm_fd);
> >>>>>+
> >>>>>+	igt_assert_eq(ret, 0);
> >>>>
> >>>>'ret' is already checked in the subtest also, this would fail if you don't
> >>>>run the subtest.
> >>>
> >>>If I dont run the subtest, it will use the initialized value of ret which is 0.
> >>>It will only fail if update_display returns a negative value.
> >>
> >>Sorry, you are right I misread that. But, it will not be this assert to fire
> >>if ret != 0 but the one in the subtest. My point was that this statement
> >>seems a bit redundant and could be eliminated.
> >>
> >>Antonio
> >>
> >>>
> >>>Manasi
> >>>
> >>>>
> >>>>Comments mostly from a rapid read. You will still need a proper reviewer for
> >>>>the KMS stuff.
> >>>>
> >>>>Thanks,
> >>>>Antonio
> >>>>
> >>>>>+
> >>>>>+	igt_info("DSC testing done\n");
> >>>>>+	igt_exit();
> >>>>>+
> >>>>>+}
> >>>>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev