[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 at intel.com
Tue Nov 6 00:29:18 UTC 2018
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 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.
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();
> > > > +
> > > > +}
> > > >
> > >
> > >
> > >
> > >
>
More information about the igt-dev
mailing list