[igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
Antonio Argenziano
antonio.argenziano at intel.com
Tue Oct 16 21:14:59 UTC 2018
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.
>
>>
>>> 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?
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.
>
>>
>>> + 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