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

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

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