[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 Oct 16 21:37:32 UTC 2018
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 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 \
> >>
> >>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();
> >>>+
> >>>+}
> >>>
More information about the igt-dev
mailing list