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

[Dhinakaran Pandiyan]

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 at intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare at 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 at 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();
> > > +
> > > +}
> > > 
> > 
> > 
> > 
> >