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 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?

> 
> >  	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 @@
> >+/*
> >+ * Copyright © 2017 Intel Corporation
> >+ *
> >+ * Displayport Display Stream Compression test
> >+ *
> >+ * Authors:
> >+ * Manasi Navare <manasi.d.navare@intel.com>
> >+ *
> >+ * Elements of the modeset code adapted from David Herrmann's
> >+ * DRM modeset example
> >+ *
> >+ */
> >+#include "igt.h"
> >+#include <errno.h>
> >+#include <getopt.h>
> >+#include <math.h>
> >+#include <stdint.h>
> >+#include <stdbool.h>
> >+#include <strings.h>
> >+#include <unistd.h>
> >+#include <stdlib.h>
> >+#include <signal.h>
> >+#include <time.h>
> >+#include <fcntl.h>
> >+#include <termios.h>
> >+
> >+int drm_fd;
> >+drmModeRes *resources;
> >+uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
> 
> ^ Seems to be used only in one place, why not use the macro directly?

Hmm, yea I could use the macro directly since I dont think any subtests would change
the tiling format.

> 
> >+
> >+struct connector {
> >+	uint32_t id;
> >+	int mode_valid;
> >+	drmModeModeInfo mode;
> >+	drmModeConnector *connector;
> >+	drmModeEncoder *encoder;
> >+	int crtc, pipe;
> >+};
> >+
> >+struct dsc_config {
> >+	char test_name[128];
> >+	int mode_width;
> >+	int mode_height;
> >+	int bpp;
> >+	int depth;
> >+	bool dsc_supported;
> >+	bool dsc_enable;
> >+};
> >+
> >+static FILE *fopenat(int dir, const char *name, const char *mode)
> >+{
> >+	int fd = openat(dir, name, O_RDWR);
> >+	return fdopen(fd, mode);
> >+}
> >+
> >+static bool get_dp_dsc_support(char *test_connector_name)
> >+{
> >+	int dir = igt_debugfs_dir(drm_fd);
> >+	FILE *dsc_support_fp = NULL;
> >+	char *line = NULL;
> >+	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)?

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 void set_dp_dsc_enable(bool enable, char *test_connector_name)
> >+{
> >+	int dir = igt_debugfs_dir(drm_fd);
> >+	FILE *dsc_enable_fp = NULL;
> >+	char buf[128] = {0};
> >+
> >+	strcpy(buf, test_connector_name);
> >+	strcat(buf, "/i915_dsc_support");
> >+	dsc_enable_fp = fopenat(dir, buf, "w+");
> >+	igt_require(dsc_enable_fp);
> >+	rewind(dsc_enable_fp);
> >+	igt_info("Setting DSC_ENABLE to %s for %s\n", (enable) ? "true" : "false", test_connector_name);
> >+	fprintf(dsc_enable_fp, "%d", enable);
> >+	fflush(dsc_enable_fp);
> >+	fclose(dsc_enable_fp);
> >+}
> >+
> >+static void dump_connectors_fd(int drmfd)
> >+{
> >+	int i, j;
> >+
> >+	drmModeRes *mode_resources = drmModeGetResources(drmfd);
> >+
> >+        if (!mode_resources) {
> >+		igt_warn("drmModeGetResources failed: %s\n", strerror(errno));
> >+		return;
> >+	}
> >+
> >+	igt_info("Connectors:\n");
> >+	igt_info("id\tencoder\tstatus\t\ttype\tsize (mm)\tmodes\n");
> >+	for (i = 0; i < mode_resources->count_connectors; i++) {
> >+		drmModeConnector *connector;
> >+
> >+		connector = drmModeGetConnectorCurrent(drmfd,
> >+						       mode_resources->connectors[i]);
> >+		if (!connector) {
> >+			igt_warn("could not get connector %i: %s\n",
> >+				 mode_resources->connectors[i], strerror(errno));
> >+			continue;
> >+		}
> >+
> >+		igt_info("%d\t%d\t%s\t%s\t%dx%d\t\t%d\n",
> >+			 connector->connector_id, connector->encoder_id,
> >+			 kmstest_connector_status_str(connector->connection),
> >+			 kmstest_connector_type_str(connector->connector_type),
> >+			 connector->mmWidth, connector->mmHeight,
> >+			 connector->count_modes);
> >+
> >+		if (!connector->count_modes)
> 
> Need to free the connector here.

Yes wil add drmModeFreeConnector(connector); here before continue

> 
> >+			continue;
> >+
> >+		igt_info("  modes:\n");
> >+		igt_info("  name refresh (Hz) hdisp hss hse htot vdisp ""vss vse vtot flags type clock\n");
> >+		for (j = 0; j < connector->count_modes; j++){
> >+			igt_info("[%d]", j);
> >+			kmstest_dump_mode(&connector->modes[j]);
> >+		}
> >+
> >+		drmModeFreeConnector(connector);
> >+	}
> >+	igt_info("\n");
> >+
> >+	drmModeFreeResources(mode_resources);
> >+}
> >+
> >+static void dump_crtcs_fd(int drmfd)
> >+{
> >+	int i;
> >+	drmModeRes *mode_resources = drmModeGetResources(drmfd);
> >+
> >+	igt_info("CRTCs:\n");
> >+	igt_info("id\tfb\tpos\tsize\n");
> >+	for (i = 0; i < mode_resources->count_crtcs; i++) {
> >+		drmModeCrtc *crtc;
> >+
> >+		crtc = drmModeGetCrtc(drmfd, mode_resources->crtcs[i]);
> >+		if (!crtc) {
> >+			igt_warn("could not get crtc %i: %s\n",
> >+				 mode_resources->crtcs[i], strerror(errno));
> >+			continue;
> >+		}
> >+		igt_info("%d\t%d\t(%d,%d)\t(%dx%d)\n",
> >+			 crtc->crtc_id, crtc->buffer_id, crtc->x,
> >+			 crtc->y, crtc->width, crtc->height);
> >+		kmstest_dump_mode(&crtc->mode);
> >+
> >+		drmModeFreeCrtc(crtc);
> >+	}
> >+	igt_info("\n");
> >+
> >+	drmModeFreeResources(mode_resources);
> >+}
> >+
> >+static void dump_info(void)
> >+{
> >+	dump_connectors_fd(drm_fd);
> >+	dump_crtcs_fd(drm_fd);
> >+}
> >+
> >+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?

> 
> >+	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() ?

> 
> >+	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.

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