All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
@ 2018-10-05 23:34 Manasi Navare
  2018-10-06  1:53 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Manasi Navare @ 2018-10-05 23:34 UTC (permalink / raw)
  To: igt-dev; +Cc: Manasi Navare, Anusha Srivatsa

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 \
 	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;
+
+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);
+
+	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;
+		}
+	}
+
+	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)
+			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);
+	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)
+{
+	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);
+		igt_assert_eq(ret, 0);
+	}
+
+	close(drm_fd);
+
+	igt_assert_eq(ret, 0);
+
+	igt_info("DSC testing done\n");
+	igt_exit();
+
+}
-- 
2.18.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
@ 2018-10-06  1:53 ` Patchwork
  2018-10-06 10:14 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-06  1:53 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev

== Series Details ==

Series: test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
URL   : https://patchwork.freedesktop.org/series/50652/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4944 -> IGTPW_1920 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50652/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1920 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106248, fdo#106725)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    
    ==== Possible fixes ====

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#107726) -> PASS

    
    ==== Warnings ====

    igt@amdgpu/amd_prime@amd-to-i915:
      fi-kbl-8809g:       FAIL (fdo#107341) -> DMESG-FAIL (fdo#107762)

    
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726
  fdo#107762 https://bugs.freedesktop.org/show_bug.cgi?id=107762


== Participating hosts (45 -> 38) ==

  Missing    (7): fi-ilk-m540 fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-gdg-551 


== Build changes ==

    * IGT: IGT_4670 -> IGTPW_1920

  CI_DRM_4944: 66bd263b99fd264b57432c232756baf95b0a6255 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1920: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1920/
  IGT_4670: 7e066794d2ea860f4199fd67549080de17b6b852 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1920/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
  2018-10-06  1:53 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-10-06 10:14 ` Patchwork
  2018-10-10 15:48 ` [igt-dev] [PATCH i-g-t] " Antonio Argenziano
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-06 10:14 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev

== Series Details ==

Series: test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
URL   : https://patchwork.freedesktop.org/series/50652/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4670_full -> IGTPW_1920_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1920_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1920_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50652/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1920_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in IGTPW_1920_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-tiled2untiled:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@gem_eio@in-flight-suspend:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106702)

    igt@gem_exec_bad_domains@conflicting-write-domain:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +6

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#106023)

    igt@kms_color@pipe-b-ctm-max:
      shard-apl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +43

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> FAIL (fdo#103232) +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
      shard-kbl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-apl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          PASS -> FAIL (fdo#103167) +5

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-kbl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166) +4

    igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
      shard-apl:          PASS -> FAIL (fdo#103166)

    
    ==== Possible fixes ====

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-apl:          FAIL (fdo#106641) -> PASS

    igt@kms_busy@extended-pageflip-hang-newfb-render-a:
      shard-glk:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_cursor_crc@cursor-128x42-random:
      shard-glk:          FAIL (fdo#103232) -> PASS +1

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +8

    igt@kms_cursor_crc@cursor-64x21-sliding:
      shard-kbl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          FAIL (fdo#103191, fdo#103232) -> PASS

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          DMESG-WARN (fdo#105763, fdo#106538) -> PASS +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-apl:          FAIL (fdo#103167) -> PASS +5

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-blt:
      shard-glk:          FAIL (fdo#103167) -> PASS +2

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-snb:          FAIL (fdo#107749, fdo#103166) -> PASS

    {igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb}:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
      shard-apl:          FAIL (fdo#103166) -> PASS +5
      shard-kbl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_rotation_crc@primary-rotation-180:
      shard-snb:          FAIL (fdo#103925) -> PASS

    igt@kms_universal_plane@universal-plane-pipe-c-functional:
      shard-glk:          FAIL (fdo#103166) -> PASS +4

    
    ==== Warnings ====

    {igt@kms_plane_alpha_blend@pipe-a-alpha-7efc}:
      shard-apl:          FAIL (fdo#108145) -> DMESG-FAIL (fdo#103558, fdo#105602) +1

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106702 https://bugs.freedesktop.org/show_bug.cgi?id=106702
  fdo#107749 https://bugs.freedesktop.org/show_bug.cgi?id=107749
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-skl 


== Build changes ==

    * IGT: IGT_4670 -> IGTPW_1920
    * Linux: CI_DRM_4933 -> CI_DRM_4944

  CI_DRM_4933: 6b7a44d1597791524f46d7ea17620db54dffdc8c @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4944: 66bd263b99fd264b57432c232756baf95b0a6255 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1920: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1920/
  IGT_4670: 7e066794d2ea860f4199fd67549080de17b6b852 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1920/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
  2018-10-06  1:53 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
  2018-10-06 10:14 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2018-10-10 15:48 ` Antonio Argenziano
  2018-10-16 18:47   ` Manasi Navare
  2018-10-11 21:21 ` Radhakrishna Sripada
  2018-11-02 22:39 ` Dhinakaran Pandiyan
  4 siblings, 1 reply; 20+ messages in thread
From: Antonio Argenziano @ 2018-10-10 15:48 UTC (permalink / raw)
  To: Manasi Navare, igt-dev; +Cc: Anusha Srivatsa



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.

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

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

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

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

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

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

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
                   ` (2 preceding siblings ...)
  2018-10-10 15:48 ` [igt-dev] [PATCH i-g-t] " Antonio Argenziano
@ 2018-10-11 21:21 ` Radhakrishna Sripada
  2018-10-12  7:20   ` Radhakrishna Sripada
  2018-10-16 19:11   ` Manasi Navare
  2018-11-02 22:39 ` Dhinakaran Pandiyan
  4 siblings, 2 replies; 20+ messages in thread
From: Radhakrishna Sripada @ 2018-10-11 21:21 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev, Anusha Srivatsa

On Fri, Oct 05, 2018 at 04:34:37PM -0700, 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 \
>  	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;
> +
> +struct connector {
> +	uint32_t id;
> +	int mode_valid;
> +	drmModeModeInfo mode;
> +	drmModeConnector *connector;
> +	drmModeEncoder *encoder;
> +	int crtc, pipe;
> +};
Can kmstest_connector_config be used to simplify this structure?
> +
> +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);
> +
> +	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;
> +		}
> +	}
> +
This function could make use of igt_debugfs_simple_read the way kms_psr does to make it cleaner.
> +	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);
Can we use igt_sysfs_write here?
> +	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)
> +			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);
> +	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;
> +
Is it possible to use igt_display_t, followed by igt_display_require in igt_fixture?
Or is it way too much overhead in collecting all non-required resources?
> +	/* 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")) {
Will this code path run?
> +					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)
> +{
> +	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);
> +		igt_assert_eq(ret, 0);
> +	}
> +
> +	close(drm_fd);
> +
> +	igt_assert_eq(ret, 0);
Is this assert required?

- Radhakrishna(RK) Sripada
> +
> +	igt_info("DSC testing done\n");
> +	igt_exit();
> +
> +}
> -- 
> 2.18.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-11 21:21 ` Radhakrishna Sripada
@ 2018-10-12  7:20   ` Radhakrishna Sripada
  2018-10-16 19:11   ` Manasi Navare
  1 sibling, 0 replies; 20+ messages in thread
From: Radhakrishna Sripada @ 2018-10-12  7:20 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev, Anusha Srivatsa

On Thu, Oct 11, 2018 at 02:21:52PM -0700, Radhakrishna Sripada wrote:
> On Fri, Oct 05, 2018 at 04:34:37PM -0700, 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 \
> >  	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;
> > +
> > +struct connector {
> > +	uint32_t id;
> > +	int mode_valid;
> > +	drmModeModeInfo mode;
> > +	drmModeConnector *connector;
> > +	drmModeEncoder *encoder;
> > +	int crtc, pipe;
> > +};
> Can kmstest_connector_config be used to simplify this structure?
> > +
> > +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);
> > +
> > +	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;
> > +		}
> > +	}
> > +
> This function could make use of igt_debugfs_simple_read the way kms_psr does to make it cleaner.
> > +	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);
> Can we use igt_sysfs_write here?
> > +	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)
> > +			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);
> > +	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;
> > +
> Is it possible to use igt_display_t, followed by igt_display_require in igt_fixture?
> Or is it way too much overhead in collecting all non-required resources?
> > +	/* 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")) {
> Will this code path run?
Ignore the above comment.

Thanks,
RK
> > +					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)
> > +{
> > +	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);
> > +		igt_assert_eq(ret, 0);
> > +	}
> > +
> > +	close(drm_fd);
> > +
> > +	igt_assert_eq(ret, 0);
> Is this assert required?
> 
> - Radhakrishna(RK) Sripada
> > +
> > +	igt_info("DSC testing done\n");
> > +	igt_exit();
> > +
> > +}
> > -- 
> > 2.18.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-10 15:48 ` [igt-dev] [PATCH i-g-t] " Antonio Argenziano
@ 2018-10-16 18:47   ` Manasi Navare
  2018-10-16 21:14     ` Antonio Argenziano
  0 siblings, 1 reply; 20+ messages in thread
From: Manasi Navare @ 2018-10-16 18:47 UTC (permalink / raw)
  To: Antonio Argenziano; +Cc: igt-dev, Anusha Srivatsa

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-11 21:21 ` Radhakrishna Sripada
  2018-10-12  7:20   ` Radhakrishna Sripada
@ 2018-10-16 19:11   ` Manasi Navare
  1 sibling, 0 replies; 20+ messages in thread
From: Manasi Navare @ 2018-10-16 19:11 UTC (permalink / raw)
  To: Radhakrishna Sripada; +Cc: igt-dev, Anusha Srivatsa

On Thu, Oct 11, 2018 at 02:21:52PM -0700, Radhakrishna Sripada wrote:
> On Fri, Oct 05, 2018 at 04:34:37PM -0700, 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 \
> >  	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;
> > +
> > +struct connector {
> > +	uint32_t id;
> > +	int mode_valid;
> > +	drmModeModeInfo mode;
> > +	drmModeConnector *connector;
> > +	drmModeEncoder *encoder;
> > +	int crtc, pipe;
> > +};
> Can kmstest_connector_config be used to simplify this structure?

Usually what I have seen is that kms_connector_config would be used to obtain
the config through the kms helpers and then this local struct variables are assigned
things from kms_connector_config.
But i will take a look at if I can simplify and use kms_connector_config directly

> > +
> > +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);
> > +
> > +	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;
> > +		}
> > +	}
> > +
> This function could make use of igt_debugfs_simple_read the way kms_psr does to make it cleaner.

I iwll look at the igt_debugfs_read() and see if that can be used.

> > +	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);
> Can we use igt_sysfs_write here?

But this is not a sysfs node, just a debugfs. Will have to see if there is any igt debugfs wrapper

> > +	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)
> > +			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);
> > +	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;
> > +
> Is it possible to use igt_display_t, followed by igt_display_require in igt_fixture?
> Or is it way too much overhead in collecting all non-required resources?

Not sure what you mean here. Are you suggesting removing the update_display() function and using
igt_display_init instead? But I am not sure how I can set mode per connector in that case like I am doing
in update_display with the for loop

> > +	/* 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")) {
> Will this code path run?
> > +					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)
> > +{
> > +	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);
> > +		igt_assert_eq(ret, 0);
> > +	}
> > +
> > +	close(drm_fd);
> > +
> > +	igt_assert_eq(ret, 0);
> Is this assert required?

The goal is to use this assert to check the ret value of update display.

Manasi

> 
> - Radhakrishna(RK) Sripada
> > +
> > +	igt_info("DSC testing done\n");
> > +	igt_exit();
> > +
> > +}
> > -- 
> > 2.18.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-16 18:47   ` Manasi Navare
@ 2018-10-16 21:14     ` Antonio Argenziano
  2018-10-16 21:37       ` Manasi Navare
  2018-10-17  7:48       ` Petri Latvala
  0 siblings, 2 replies; 20+ messages in thread
From: Antonio Argenziano @ 2018-10-16 21:14 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev, Anusha Srivatsa



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.

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

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.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-16 21:14     ` Antonio Argenziano
@ 2018-10-16 21:37       ` Manasi Navare
  2018-10-17 20:38         ` Antonio Argenziano
  2018-10-17  7:48       ` Petri Latvala
  1 sibling, 1 reply; 20+ messages in thread
From: Manasi Navare @ 2018-10-16 21:37 UTC (permalink / raw)
  To: Antonio Argenziano; +Cc: igt-dev, Anusha Srivatsa

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

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-16 21:14     ` Antonio Argenziano
  2018-10-16 21:37       ` Manasi Navare
@ 2018-10-17  7:48       ` Petri Latvala
  2018-10-17 16:01         ` Manasi Navare
  1 sibling, 1 reply; 20+ messages in thread
From: Petri Latvala @ 2018-10-17  7:48 UTC (permalink / raw)
  To: Antonio Argenziano, Manasi Navare; +Cc: igt-dev, Anusha Srivatsa

On Tue, Oct 16, 2018 at 02:14:59PM -0700, Antonio Argenziano wrote:
> > > > +		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.

It's not only redundant, it's invalid to call igt_assert when not
inside an igt_fixture or a subtest. See lib/igt_core.c:1180,
igt_can_fail().



-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-17  7:48       ` Petri Latvala
@ 2018-10-17 16:01         ` Manasi Navare
  2018-10-17 16:49           ` Antonio Argenziano
  0 siblings, 1 reply; 20+ messages in thread
From: Manasi Navare @ 2018-10-17 16:01 UTC (permalink / raw)
  To: Antonio Argenziano, igt-dev, Anusha Srivatsa

On Wed, Oct 17, 2018 at 10:48:24AM +0300, Petri Latvala wrote:
> On Tue, Oct 16, 2018 at 02:14:59PM -0700, Antonio Argenziano wrote:
> > > > > +		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.
> 
> It's not only redundant, it's invalid to call igt_assert when not
> inside an igt_fixture or a subtest. See lib/igt_core.c:1180,
> igt_can_fail().
>

Thanks Antonio and Petri for pointing this out.
I will remove the igt_assert from there. The igt_fixture() should already take care
of having that after the test right? So I dont need to call it explicitly in igt_fixture()

Manasi 
> 
> 
> -- 
> Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-17 16:01         ` Manasi Navare
@ 2018-10-17 16:49           ` Antonio Argenziano
  2018-10-20  0:06             ` Manasi Navare
  0 siblings, 1 reply; 20+ messages in thread
From: Antonio Argenziano @ 2018-10-17 16:49 UTC (permalink / raw)
  To: Manasi Navare, igt-dev, Anusha Srivatsa



On 17/10/18 09:01, Manasi Navare wrote:
> On Wed, Oct 17, 2018 at 10:48:24AM +0300, Petri Latvala wrote:
>> On Tue, Oct 16, 2018 at 02:14:59PM -0700, Antonio Argenziano wrote:
>>>>>> +		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.
>>
>> It's not only redundant, it's invalid to call igt_assert when not
>> inside an igt_fixture or a subtest. See lib/igt_core.c:1180,
>> igt_can_fail().
>>
> 
> Thanks Antonio and Petri for pointing this out.
> I will remove the igt_assert from there. The igt_fixture() should already take care
> of having that after the test right? So I dont need to call it explicitly in igt_fixture()

You want to call asserts mainly inside the test itself, close(drm_fd) 
should probably go in a fixture at the end to avoid calling it when 
listing subtests only.

Antonio

> 
> Manasi
>>
>>
>> -- 
>> Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-16 21:37       ` Manasi Navare
@ 2018-10-17 20:38         ` Antonio Argenziano
  2018-10-26 23:28           ` Manasi Navare
  0 siblings, 1 reply; 20+ messages in thread
From: Antonio Argenziano @ 2018-10-17 20:38 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev, Anusha Srivatsa



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-17 16:49           ` Antonio Argenziano
@ 2018-10-20  0:06             ` Manasi Navare
  0 siblings, 0 replies; 20+ messages in thread
From: Manasi Navare @ 2018-10-20  0:06 UTC (permalink / raw)
  To: Antonio Argenziano; +Cc: igt-dev, Anusha Srivatsa

On Wed, Oct 17, 2018 at 09:49:56AM -0700, Antonio Argenziano wrote:
> 
> 
> On 17/10/18 09:01, Manasi Navare wrote:
> >On Wed, Oct 17, 2018 at 10:48:24AM +0300, Petri Latvala wrote:
> >>On Tue, Oct 16, 2018 at 02:14:59PM -0700, Antonio Argenziano wrote:
> >>>>>>+		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.
> >>
> >>It's not only redundant, it's invalid to call igt_assert when not
> >>inside an igt_fixture or a subtest. See lib/igt_core.c:1180,
> >>igt_can_fail().
> >>
> >
> >Thanks Antonio and Petri for pointing this out.
> >I will remove the igt_assert from there. The igt_fixture() should already take care
> >of having that after the test right? So I dont need to call it explicitly in igt_fixture()
> 
> You want to call asserts mainly inside the test itself, close(drm_fd) should
> probably go in a fixture at the end to avoid calling it when listing
> subtests only.
>

Ok, got it! So I can call it in update_display which essentially is the test.

Manasi
 
> Antonio
> 
> >
> >Manasi
> >>
> >>
> >>-- 
> >>Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-17 20:38         ` Antonio Argenziano
@ 2018-10-26 23:28           ` Manasi Navare
  0 siblings, 0 replies; 20+ messages in thread
From: Manasi Navare @ 2018-10-26 23:28 UTC (permalink / raw)
  To: Antonio Argenziano; +Cc: igt-dev, Anusha Srivatsa

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
                   ` (3 preceding siblings ...)
  2018-10-11 21:21 ` Radhakrishna Sripada
@ 2018-11-02 22:39 ` Dhinakaran Pandiyan
  2018-11-05 20:58   ` Manasi Navare
  4 siblings, 1 reply; 20+ messages in thread
From: Dhinakaran Pandiyan @ 2018-11-02 22:39 UTC (permalink / raw)
  To: igt-dev; +Cc: Manasi Navare, Anusha Srivatsa

On Friday, October 5, 2018 4:34:37 PM PDT 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 \
>  	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;
> +
> +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);
> +
> +	while (getline(&line, &line_size, dsc_support_fp) > 0) {
This loop looks wasteful.

> +		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;
What is the use of this check? If all that we need to know is whether DSC is supported, just check for "Supported: yes"

> +			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;
Skip the test here.
> +		else
> +			igt_fail_on_f(true, "Unknown DSC supported status '%s'\n",
> +				      supported_str);
> +		break;
> +		}
> +	}
> +
> +	free(line);

You could simplify the logic in this function with igt_debugfs_read() and strstr()
There are examples in kms_psr.c, kms_fbt_fbcon etc.

> +	fclose(dsc_support_fp);
Open the file once, cache the fd and close it when you are done with the test.

> +
> +	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);
Should be  a lot simpler using sysfs_write(). Again, lib/igt_psr.c has examples.

> +	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)
> +			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? If this for manual testing, please use manual(). There are plenty of examples.

> +	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));
This can't be a warning, the test should fail if the mode can't be set.

> +		igt_remove_fb(drm_fd, &fb_info);

Read back debugfs to check whether DSC was enabled? I'm can't tell what is being tested here.
> +
> +	}
> +
> +	return ret;
> +}
> +
> +
> +/*
> + * Re-probe connectors and do a modeset with and wihout DSC
> + *
> + * Returns:
> + * 0: On Success
> + * -1: On failure
int is pointless, just use a bool return if the function doesn't make use of error codes.

> + */
> +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;
> +	}

Why hand roll IOCTLs when the IGT library provides a lot of these functionality?
> +
> +	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")) {
Hmm, there is just one subtest, what else could this be?


> +					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);
ret gets overwritten in the next loop, if set_mode fails, then the test should too.

> +					} else {
skip if the panel does not support DSC  and do it inside the support check helper.

> +						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)
> +{
> +	bool *dump = opt_dump;
> +
> +	switch (opt) {
> +	case 'i':
> +		*dump = true;
Don't see much point adding an option,  if you really want to dump the  full mode, just use igt_debug()
and run the test with --debug.

> +		break;
> +	default:
> +		igt_assert(0);
> +	}
> +
> +	return 0;
> +}
> +
> +int main(int argc, char *argv[])
Remove dump and switch to  igt_main
> +{
> +	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);
This won't be needed either.

> +
> +	igt_skip_on_simulation();
> +
> +	igt_fixture {
> +		kmstest_set_vt_graphics_mode();
> +		drm_fd = drm_open_driver(DRIVER_ANY);

Missing igt_display_require()

> +		if (opt_dump)
> +			dump_info();

> +	}
> +
> +	igt_subtest("force_dsc_enable_basic") {
> +		strcpy(test_config.test_name,"force_dsc_enable_basic");
Why? 
> +		ret = update_display(&test_config);
> +		igt_assert_eq(ret, 0);
> +	}
> +
The test doesn't make use of display_init() and display_fini() like  other kms tests do, what is the reason?
> +	close(drm_fd);
> +
> +	igt_assert_eq(ret, 0);
> +
> +	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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-11-02 22:39 ` Dhinakaran Pandiyan
@ 2018-11-05 20:58   ` Manasi Navare
  2018-11-05 23:22     ` Dhinakaran Pandiyan
  0 siblings, 1 reply; 20+ messages in thread
From: Manasi Navare @ 2018-11-05 20:58 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: igt-dev, Anusha Srivatsa

Thanks for the review, following are my comments based on our discussion on Friday:

On Fri, Nov 02, 2018 at 03:39:57PM -0700, Dhinakaran Pandiyan wrote:
> On Friday, October 5, 2018 4:34:37 PM PDT 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 \
> >  	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

I will add more comments here mainly specifying that since we do not have any wy to validate
the quality of compression, we are mainly validating the driver configuration for DSC here.

> > + *
> > + */
> > +#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;
> > +
> > +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);
> > +
> > +	while (getline(&line, &line_size, dsc_support_fp) > 0) {
> This loop looks wasteful.
> 
> > +		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;
> What is the use of this check? If all that we need to know is whether DSC is supported, just check for "Supported: yes"

Well the idea was to just ignore Supported value if Enabled is true because that means it was already enabled by default.
But our discussion makes me think that if we are just validating the configuration, I can just read Supported here and force
DSC and then after modeset, read back the value of Enabled to check that it was enabled. But even in the case where it cannot
be enabled, thats just because our platform probably doesnt support the configuration. So its not a test fail really.

May be the real pass fail test would be if its enabled == yes, then disable DSC and enable again and check if it is enabled
and test this for all configurations.
What do you think?

> 
> > +			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;
> Skip the test here.
> > +		else
> > +			igt_fail_on_f(true, "Unknown DSC supported status '%s'\n",
> > +				      supported_str);
> > +		break;
> > +		}
> > +	}
> > +
> > +	free(line);
> 
> You could simplify the logic in this function with igt_debugfs_read() and strstr()
> There are examples in kms_psr.c, kms_fbt_fbcon etc.

Yes will do that, thanks for pointing out.

> 
> > +	fclose(dsc_support_fp);
> Open the file once, cache the fd and close it when you are done with the test.
>

Ok
 
> > +
> > +	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);
> Should be  a lot simpler using sysfs_write(). Again, lib/igt_psr.c has examples.
> 
> > +	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)
> > +			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? If this for manual testing, please use manual(). There are plenty of examples.

Manual testing for visually testing results of compression since we dont have any CRC ways to check this.
But I will probably remove this since it was for my debug purposes and code validation.

> 
> > +	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));
> This can't be a warning, the test should fail if the mode can't be set.
> 
> > +		igt_remove_fb(drm_fd, &fb_info);
> 
> Read back debugfs to check whether DSC was enabled? I'm can't tell what is being tested here.

Sure can add this for the test strategy of disabling and then enabling, just to validate the driver.

> > +
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +
> > +/*
> > + * Re-probe connectors and do a modeset with and wihout DSC
> > + *
> > + * Returns:
> > + * 0: On Success
> > + * -1: On failure
> int is pointless, just use a bool return if the function doesn't make use of error codes.
> 
> > + */
> > +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;
> > +	}
> 
> Why hand roll IOCTLs when the IGT library provides a lot of these functionality?

I will take a look at the library functions, but since the DSC enabling logic is inserted per connector
it was hard to use igt lib functions.

> > +
> > +	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")) {
> Hmm, there is just one subtest, what else could this be?
> 
> 
> > +					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);
> ret gets overwritten in the next loop, if set_mode fails, then the test should too.
> 
> > +					} else {
> skip if the panel does not support DSC  and do it inside the support check helper.
> 
> > +						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)
> > +{
> > +	bool *dump = opt_dump;
> > +
> > +	switch (opt) {
> > +	case 'i':
> > +		*dump = true;
> Don't see much point adding an option,  if you really want to dump the  full mode, just use igt_debug()
> and run the test with --debug.

Hmm ok will test what --debug dumps and if thats enough.

> 
> > +		break;
> > +	default:
> > +		igt_assert(0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int main(int argc, char *argv[])
> Remove dump and switch to  igt_main
> > +{
> > +	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);
> This won't be needed either.
> 
> > +
> > +	igt_skip_on_simulation();
> > +
> > +	igt_fixture {
> > +		kmstest_set_vt_graphics_mode();
> > +		drm_fd = drm_open_driver(DRIVER_ANY);
> 
> Missing igt_display_require()
> 
> > +		if (opt_dump)
> > +			dump_info();
> 
> > +	}
> > +
> > +	igt_subtest("force_dsc_enable_basic") {
> > +		strcpy(test_config.test_name,"force_dsc_enable_basic");
> Why? 

This is to pass the test name, more tests will be added with diff names.

Manasi


> > +		ret = update_display(&test_config);
> > +		igt_assert_eq(ret, 0);
> > +	}
> > +
> The test doesn't make use of display_init() and display_fini() like  other kms tests do, what is the reason?
> > +	close(drm_fd);
> > +
> > +	igt_assert_eq(ret, 0);
> > +
> > +	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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-11-05 20:58   ` Manasi Navare
@ 2018-11-05 23:22     ` Dhinakaran Pandiyan
  2018-11-06  0:29       ` Manasi Navare
  0 siblings, 1 reply; 20+ messages in thread
From: Dhinakaran Pandiyan @ 2018-11-05 23:22 UTC (permalink / raw)
  To: Manasi Navare; +Cc: igt-dev, Anusha Srivatsa

On Mon, 2018-11-05 at 12:58 -0800, Manasi Navare wrote:
> Thanks for the review, following are my comments based on our
> discussion on Friday:
> 
> On Fri, Nov 02, 2018 at 03:39:57PM -0700, Dhinakaran Pandiyan wrote:
> > On Friday, October 5, 2018 4:34:37 PM PDT 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 \
> > >  	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
You might want to fix the date here.

> > > + *
> > > + * 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
Missing license.

> 
> I will add more comments here mainly specifying that since we do not
> have any wy to validate
> the quality of compression, we are mainly validating the driver
> configuration for DSC here.
> 
> > > + *
> > > + */
> > > +#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;
> > > +
> > > +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);
> > > +
> > > +	while (getline(&line, &line_size, dsc_support_fp) > 0) {
> > 
> > This loop looks wasteful.
> > 
> > > +		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;
> > 
> > What is the use of this check? If all that we need to know is
> > whether DSC is supported, just check for "Supported: yes"
> 
> Well the idea was to just ignore Supported value if Enabled is true
> because that means it was already enabled by default.
> But our discussion makes me think that if we are just validating the
> configuration, I can just read Supported here and force
> DSC and then after modeset, read back the value of Enabled to check
> that it was enabled. But even in the case where it cannot
> be enabled, thats just because our platform probably doesnt support
> the configuration. So its not a test fail really.
> 
> May be the real pass fail test would be if its enabled == yes, then
> disable DSC and enable again and check if it is enabled
> and test this for all configurations.
> What do you think?
That doesn't tell us much.

If we want to just validate encoder->compute_config(), I'd recommend
enumerating a subtest for each valid bpp (based on bspec) and failing
the test if the driver does not enable DSC when it is expected to.
Related comment below.

But, we should really get a panel in CI that needs DSC for the
preferred mode and demonstrate that the mode works because DSC was
enabled.

> 
> > 
> > > +			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;
> > 
> > Skip the test here.
> > > +		else
> > > +			igt_fail_on_f(true, "Unknown DSC supported
> > > status '%s'\n",
> > > +				      supported_str);
> > > +		break;
> > > +		}
> > > +	}
> > > +
> > > +	free(line);
> > 
> > You could simplify the logic in this function with
> > igt_debugfs_read() and strstr()
> > There are examples in kms_psr.c, kms_fbt_fbcon etc.
> 
> Yes will do that, thanks for pointing out.
> 
> > 
> > > +	fclose(dsc_support_fp);
> > 
> > Open the file once, cache the fd and close it when you are done
> > with the test.
> > 
> 
> Ok
>  
> > > +
> > > +	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);
> > 
> > Should be  a lot simpler using sysfs_write(). Again, lib/igt_psr.c
> > has examples.
> > 
> > > +	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)
> > > +			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? If this for manual testing, please use manual(). There are
> > plenty of examples.
> 
> Manual testing for visually testing results of compression since we
> dont have any CRC ways to check this.
> But I will probably remove this since it was for my debug purposes
> and code validation.
> 
> > 
> > > +	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));
> > 
> > This can't be a warning, the test should fail if the mode can't be
> > set.
> > 
> > > +		igt_remove_fb(drm_fd, &fb_info);
> > 
> > Read back debugfs to check whether DSC was enabled? I'm can't tell
> > what is being tested here.
> 
> Sure can add this for the test strategy of disabling and then
> enabling, just to validate the driver.
> 
> > > +
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +
> > > +/*
> > > + * Re-probe connectors and do a modeset with and wihout DSC
> > > + *
> > > + * Returns:
> > > + * 0: On Success
> > > + * -1: On failure
> > 
> > int is pointless, just use a bool return if the function doesn't
> > make use of error codes.
> > 
> > > + */
> > > +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;
> > > +	}
> > 
> > Why hand roll IOCTLs when the IGT library provides a lot of these
> > functionality?
> 
> I will take a look at the library functions, but since the DSC
> enabling logic is inserted per connector
> it was hard to use igt lib functions.
> 
> > > +
> > > +	connectors = calloc(resources->count_connectors,
> > > +			    sizeof(struct connector));
> > > +	if (!connectors)
> > > +		return -1;
> > > +
> > > +	/* Find any connected displays */
> > > +	for (cnt = 0; cnt < resources->count_connectors; cnt++) {
Separate sub-test for each connector, we want to know which one fails.

> > > +		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"))
> > > {
> > 
> > Hmm, there is just one subtest, what else could this be?
> > 
> > 
> > > +					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;

Are there other such test configurations for which the kernel is
expected to enable DSC? If yes, generate a subtest for each valid
combination.

> > > +						set_dp_dsc_enable(test_
> > > config->dsc_enable,
> > > +							  conn_buf);
> > > +						ret = set_mode(c,
> > > test_config,
> > > +							       true);
> > 
> > ret gets overwritten in the next loop, if set_mode fails, then the
> > test should too.
> > 
> > > +					} else {
> > 
> > skip if the panel does not support DSC  and do it inside the
> > support check helper.
> > 
> > > +						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)
> > > +{
> > > +	bool *dump = opt_dump;
> > > +
> > > +	switch (opt) {
> > > +	case 'i':
> > > +		*dump = true;
> > 
> > Don't see much point adding an option,  if you really want to dump
> > the  full mode, just use igt_debug()
> > and run the test with --debug.
> 
> Hmm ok will test what --debug dumps and if thats enough.
> 
> > 
> > > +		break;
> > > +	default:
> > > +		igt_assert(0);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int main(int argc, char *argv[])
> > 
> > Remove dump and switch to  igt_main
> > > +{
> > > +	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);
> > 
> > This won't be needed either.
> > 
> > > +
> > > +	igt_skip_on_simulation();
> > > +
> > > +	igt_fixture {
> > > +		kmstest_set_vt_graphics_mode();
> > > +		drm_fd = drm_open_driver(DRIVER_ANY);
> > 
> > Missing igt_display_require()
> > 
> > > +		if (opt_dump)
> > > +			dump_info();
> > > +	}
> > > +
> > > +	igt_subtest("force_dsc_enable_basic") {
> > > +		strcpy(test_config.test_name,"force_dsc_enable_basic");
> > 
> > Why? 
> 
> This is to pass the test name, more tests will be added with diff
> names.
> 
Let's not add things that are not needed now. Also, please use enums
and/or boolean flags to setup the test configuration. That is what IGTs
typically do, passing a string and comparing isn't efficient.


> Manasi
> 
> 
> > > +		ret = update_display(&test_config);
> > > +		igt_assert_eq(ret, 0);
> > > +	}
> > > +
> > 
> > The test doesn't make use of display_init() and display_fini()
> > like  other kms tests do, what is the reason?
> > > +	close(drm_fd);
> > > +
> > > +	igt_assert_eq(ret, 0);
> > > +
> > > +	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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
  2018-11-05 23:22     ` Dhinakaran Pandiyan
@ 2018-11-06  0:29       ` Manasi Navare
  0 siblings, 0 replies; 20+ messages in thread
From: Manasi Navare @ 2018-11-06  0:29 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: igt-dev, Anusha Srivatsa

On Mon, Nov 05, 2018 at 03:22:03PM -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-11-05 at 12:58 -0800, Manasi Navare wrote:
> > Thanks for the review, following are my comments based on our
> > discussion on Friday:
> > 
> > On Fri, Nov 02, 2018 at 03:39:57PM -0700, Dhinakaran Pandiyan wrote:
> > > On Friday, October 5, 2018 4:34:37 PM PDT 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 \
> > > >  	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
> You might want to fix the date here.
> 
> > > > + *
> > > > + * 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
> Missing license.
> 
> > 
> > I will add more comments here mainly specifying that since we do not
> > have any wy to validate
> > the quality of compression, we are mainly validating the driver
> > configuration for DSC here.
> > 
> > > > + *
> > > > + */
> > > > +#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;
> > > > +
> > > > +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);
> > > > +
> > > > +	while (getline(&line, &line_size, dsc_support_fp) > 0) {
> > > 
> > > This loop looks wasteful.
> > > 
> > > > +		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;
> > > 
> > > What is the use of this check? If all that we need to know is
> > > whether DSC is supported, just check for "Supported: yes"
> > 
> > Well the idea was to just ignore Supported value if Enabled is true
> > because that means it was already enabled by default.
> > But our discussion makes me think that if we are just validating the
> > configuration, I can just read Supported here and force
> > DSC and then after modeset, read back the value of Enabled to check
> > that it was enabled. But even in the case where it cannot
> > be enabled, thats just because our platform probably doesnt support
> > the configuration. So its not a test fail really.
> > 
> > May be the real pass fail test would be if its enabled == yes, then
> > disable DSC and enable again and check if it is enabled
> > and test this for all configurations.
> > What do you think?
> That doesn't tell us much.
> 
> If we want to just validate encoder->compute_config(), I'd recommend
> enumerating a subtest for each valid bpp (based on bspec) and failing
> the test if the driver does not enable DSC when it is expected to.
> Related comment below.

Yes thats why this basic subtest is validating the bpc = 8 or depth = 24.
Anusha is working on adding a debugfs node that exposes supported color depths.
Then we add a test for each supported color depth.

> 
> But, we should really get a panel in CI that needs DSC for the
> preferred mode and demonstrate that the mode works because DSC was
> enabled.

Yes the panel I am using for testing and which I believe is the same that was
shipped to CI needs DSC to work since its perferred mode is 1920 x 1080 @90Hz and 
I just checked that it needs DSC by default to output a display, without DSC it will reject the mode and
give a modeset failure.
So now in this case, the pass fail would be disable -> enable-> read Enabled bit and if yes then test passed else fail.

Manasi

> 
> > 
> > > 
> > > > +			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;
> > > 
> > > Skip the test here.
> > > > +		else
> > > > +			igt_fail_on_f(true, "Unknown DSC supported
> > > > status '%s'\n",
> > > > +				      supported_str);
> > > > +		break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	free(line);
> > > 
> > > You could simplify the logic in this function with
> > > igt_debugfs_read() and strstr()
> > > There are examples in kms_psr.c, kms_fbt_fbcon etc.
> > 
> > Yes will do that, thanks for pointing out.
> > 
> > > 
> > > > +	fclose(dsc_support_fp);
> > > 
> > > Open the file once, cache the fd and close it when you are done
> > > with the test.
> > > 
> > 
> > Ok
> >  
> > > > +
> > > > +	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);
> > > 
> > > Should be  a lot simpler using sysfs_write(). Again, lib/igt_psr.c
> > > has examples.
> > > 
> > > > +	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)
> > > > +			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? If this for manual testing, please use manual(). There are
> > > plenty of examples.
> > 
> > Manual testing for visually testing results of compression since we
> > dont have any CRC ways to check this.
> > But I will probably remove this since it was for my debug purposes
> > and code validation.
> > 
> > > 
> > > > +	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));
> > > 
> > > This can't be a warning, the test should fail if the mode can't be
> > > set.
> > > 
> > > > +		igt_remove_fb(drm_fd, &fb_info);
> > > 
> > > Read back debugfs to check whether DSC was enabled? I'm can't tell
> > > what is being tested here.
> > 
> > Sure can add this for the test strategy of disabling and then
> > enabling, just to validate the driver.
> > 
> > > > +
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +
> > > > +/*
> > > > + * Re-probe connectors and do a modeset with and wihout DSC
> > > > + *
> > > > + * Returns:
> > > > + * 0: On Success
> > > > + * -1: On failure
> > > 
> > > int is pointless, just use a bool return if the function doesn't
> > > make use of error codes.
> > > 
> > > > + */
> > > > +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;
> > > > +	}
> > > 
> > > Why hand roll IOCTLs when the IGT library provides a lot of these
> > > functionality?
> > 
> > I will take a look at the library functions, but since the DSC
> > enabling logic is inserted per connector
> > it was hard to use igt lib functions.
> > 
> > > > +
> > > > +	connectors = calloc(resources->count_connectors,
> > > > +			    sizeof(struct connector));
> > > > +	if (!connectors)
> > > > +		return -1;
> > > > +
> > > > +	/* Find any connected displays */
> > > > +	for (cnt = 0; cnt < resources->count_connectors; cnt++) {
> Separate sub-test for each connector, we want to know which one fails.
> 
> > > > +		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"))
> > > > {
> > > 
> > > Hmm, there is just one subtest, what else could this be?
> > > 
> > > 
> > > > +					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;
> 
> Are there other such test configurations for which the kernel is
> expected to enable DSC? If yes, generate a subtest for each valid
> combination.
> 
> > > > +						set_dp_dsc_enable(test_
> > > > config->dsc_enable,
> > > > +							  conn_buf);
> > > > +						ret = set_mode(c,
> > > > test_config,
> > > > +							       true);
> > > 
> > > ret gets overwritten in the next loop, if set_mode fails, then the
> > > test should too.
> > > 
> > > > +					} else {
> > > 
> > > skip if the panel does not support DSC  and do it inside the
> > > support check helper.
> > > 
> > > > +						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)
> > > > +{
> > > > +	bool *dump = opt_dump;
> > > > +
> > > > +	switch (opt) {
> > > > +	case 'i':
> > > > +		*dump = true;
> > > 
> > > Don't see much point adding an option,  if you really want to dump
> > > the  full mode, just use igt_debug()
> > > and run the test with --debug.
> > 
> > Hmm ok will test what --debug dumps and if thats enough.
> > 
> > > 
> > > > +		break;
> > > > +	default:
> > > > +		igt_assert(0);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int main(int argc, char *argv[])
> > > 
> > > Remove dump and switch to  igt_main
> > > > +{
> > > > +	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);
> > > 
> > > This won't be needed either.
> > > 
> > > > +
> > > > +	igt_skip_on_simulation();
> > > > +
> > > > +	igt_fixture {
> > > > +		kmstest_set_vt_graphics_mode();
> > > > +		drm_fd = drm_open_driver(DRIVER_ANY);
> > > 
> > > Missing igt_display_require()
> > > 
> > > > +		if (opt_dump)
> > > > +			dump_info();
> > > > +	}
> > > > +
> > > > +	igt_subtest("force_dsc_enable_basic") {
> > > > +		strcpy(test_config.test_name,"force_dsc_enable_basic");
> > > 
> > > Why? 
> > 
> > This is to pass the test name, more tests will be added with diff
> > names.
> > 
> Let's not add things that are not needed now. Also, please use enums
> and/or boolean flags to setup the test configuration. That is what IGTs
> typically do, passing a string and comparing isn't efficient.
> 
> 
> > Manasi
> > 
> > 
> > > > +		ret = update_display(&test_config);
> > > > +		igt_assert_eq(ret, 0);
> > > > +	}
> > > > +
> > > 
> > > The test doesn't make use of display_init() and display_fini()
> > > like  other kms tests do, what is the reason?
> > > > +	close(drm_fd);
> > > > +
> > > > +	igt_assert_eq(ret, 0);
> > > > +
> > > > +	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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-11-06  0:26 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-05 23:34 [igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP Manasi Navare
2018-10-06  1:53 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2018-10-06 10:14 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-10-10 15:48 ` [igt-dev] [PATCH i-g-t] " Antonio Argenziano
2018-10-16 18:47   ` Manasi Navare
2018-10-16 21:14     ` Antonio Argenziano
2018-10-16 21:37       ` Manasi Navare
2018-10-17 20:38         ` Antonio Argenziano
2018-10-26 23:28           ` Manasi Navare
2018-10-17  7:48       ` Petri Latvala
2018-10-17 16:01         ` Manasi Navare
2018-10-17 16:49           ` Antonio Argenziano
2018-10-20  0:06             ` Manasi Navare
2018-10-11 21:21 ` Radhakrishna Sripada
2018-10-12  7:20   ` Radhakrishna Sripada
2018-10-16 19:11   ` Manasi Navare
2018-11-02 22:39 ` Dhinakaran Pandiyan
2018-11-05 20:58   ` Manasi Navare
2018-11-05 23:22     ` Dhinakaran Pandiyan
2018-11-06  0:29       ` Manasi Navare

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.