[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
Fri Oct 26 23:28:22 UTC 2018
On Wed, Oct 17, 2018 at 01:38:47PM -0700, Antonio Argenziano wrote:
>
>
> On 16/10/18 14:37, Manasi Navare wrote:
> >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)?
I am thinking of replacing the direct fopenat call by igt_debugfs_open() and then
still use the file pointer to pass to getline so that i can scan it line by line.
The problem with using the igt_debugfs_read is that then it copies it in a buffer and then
scanning line by line becomes difficult. That will be easy if there is a single line.
Does this sound okay?
Manasi
> >>>
> >>>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()?
>
> I don't think we have something like that already, a command-line parameter
> maybe?
>
> Antonio
>
> >
> >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