[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
Wed Oct 17 20:38:47 UTC 2018
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)?
>>>
>>> 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