[igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
Dhinakaran Pandiyan
dhinakaran.pandiyan at intel.com
Fri Nov 2 22:39:57 UTC 2018
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
> + *
> + * 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
> + *
> + */
> +#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();
> +
> +}
>
More information about the igt-dev
mailing list