[igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
Petri Latvala
petri.latvala at intel.com
Tue Dec 18 12:12:52 UTC 2018
On Mon, Dec 17, 2018 at 09:44:26PM +0200, Srivatsa, Anusha wrote:
>
>
> >-----Original Message-----
> >From: Latvala, Petri
> >Sent: Monday, December 17, 2018 4:00 AM
> >To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> >Cc: igt-dev at lists.freedesktop.org; Navare, Manasi D
> ><manasi.d.navare at intel.com>; Argenziano, Antonio
> ><antonio.argenziano at intel.com>; Pandiyan, Dhinakaran
> ><dhinakaran.pandiyan at intel.com>; Ville Syrjälä <ville.syrjala at linux.intel.com>
> >Subject: Re: [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC
> >on DP/eDP
> >
> >On Wed, Dec 12, 2018 at 04:57:39PM -0800, Anusha wrote:
> >> From: Manasi Navare <manasi.d.navare at intel.com>
> >>
> >> This patch adds a basic kms test to validate the display stream
> >> compression functionality if supported on DP/eDP connector.
> >> Currently this has only two subtests to force the DSC on all the eDP
> >> and DP connectors that support it with default parameters.
> >> This will be expanded to add more subtests to tweak DSC parameters.
> >>
> >> v6: (from Anusha)
> >> * Fix run_test() (Petri)
> >> * Fix update_display() to avoid leaks. (Petri)
> >> v5:
> >> * Fix test cleanup to avoid crash (Petri)
> >> v4:
> >> * Future proof for more test types (Petri)
> >> * Fix alphabetical order (Petri)
> >> * s/igt_display_init/igt_display_require (Petri)
> >> * Remove blank lines after return (Petri)
> >> v3:
> >> * Use array of connectors and loop through (Petri)
> >> * Also check for FEC on DP connectors (Manasi)
> >> * Add a Pipe_A restriction on DP (Ville)
> >> v2:
> >> * Use IGT wrappers for all (DK, Antonio)
> >> * Split into two subtests for eDP and DP types (Petri)
> >>
> >> Cc: Petri Latvala <petri.latvala at intel.com>
> >> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> >> Signed-off-by: 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 | 264 +++++++++++++++++++++++++++++++++++++++++
> >> tests/meson.build | 1 +
> >> 3 files changed, 266 insertions(+)
> >> create mode 100644 tests/kms_dp_dsc.c
> >>
> >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources index
> >> eedde1e8..a3c24c99 100644
> >> --- a/tests/Makefile.sources
> >> +++ b/tests/Makefile.sources
> >> @@ -54,6 +54,7 @@ TESTS_progs = \
> >> kms_crtc_background_color \
> >> kms_cursor_crc \
> >> kms_cursor_legacy \
> >> + kms_dp_dsc \
> >> kms_draw_crc \
> >> kms_fbcon_fbt \
> >> kms_fence_pin_leak \
> >> diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c new file mode
> >> 100644 index 00000000..884a7717
> >> --- /dev/null
> >> +++ b/tests/kms_dp_dsc.c
> >> @@ -0,0 +1,264 @@
> >> +/*
> >> + * Copyright © 2018 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person
> >> +obtaining a
> >> + * copy of this software and associated documentation files (the
> >> +"Software"),
> >> + * to deal in the Software without restriction, including without
> >> +limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute,
> >> +sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom
> >> +the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including
> >> +the next
> >> + * paragraph) shall be included in all copies or substantial portions
> >> +of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >> +EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> >> +MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> >EVENT
> >> +SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> >DAMAGES
> >> +OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> +ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> >> +OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + *
> >> + * Displayport Display Stream Compression test
> >> + * Until the CRC support is added this needs to be invoked with
> >> +--interactive
> >> + * to manually verify if the test pattern is seen without corruption
> >> +for each
> >> + * subtest.
> >> + *
> >> + * Authors:
> >> + * Manasi Navare <manasi.d.navare at intel.com>
> >> + *
> >> + */
> >> +#include "igt.h"
> >> +#include "igt_sysfs.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>
> >> +
> >> +enum dsc_test_type
> >> +{
> >> + test_basic_dsc_enable
> >> +};
> >> +
> >> +typedef struct {
> >> + int drm_fd;
> >> + int debugfs_fd;
> >> + uint32_t id;
> >> + igt_display_t display;
> >> + struct igt_fb fb_test_pattern;
> >> + igt_output_t *output;
> >> + int mode_valid;
> >> + drmModeModeInfo *mode;
> >> + drmModeConnector *connector;
> >> + drmModeEncoder *encoder;
> >> + int crtc;
> >> + enum pipe pipe;
> >> + char conn_name[128];
> >> +} data_t;
> >> +
> >> +static inline void manual(const char *expected) {
> >> + igt_debug_manual_check("all", expected); }
> >> +
> >> +static bool is_dp_dsc_supported(data_t *data) {
> >> + char file_name[128] = {0};
> >> + char buf[512];
> >> +
> >> + strcpy(file_name, data->conn_name);
> >> + strcat(file_name, "/i915_dsc_fec_support");
> >> + igt_require(igt_debugfs_simple_read(data->debugfs_fd, file_name, buf,
> >> + sizeof(buf)) > 0);
> >> + igt_debugfs_read(data->drm_fd, file_name, buf);
> >> +
> >> + return strstr(buf, "DSC_Sink_Support: yes"); }
> >> +
> >> +static bool is_dp_fec_supported(data_t *data) {
> >> + char file_name[128] = {0};
> >> + char buf[512];
> >> +
> >> + strcpy(file_name, data->conn_name);
> >> + strcat(file_name, "/i915_dsc_fec_support");
> >> + igt_debugfs_read(data->drm_fd, file_name, buf);
> >> +
> >> + return strstr(buf, "FEC_Sink_Support: yes"); }
> >> +
> >> +static bool is_dp_dsc_enabled(data_t *data) {
> >> + char file_name[128] = {0};
> >> + char buf[512];
> >> +
> >> + strcpy(file_name, data->conn_name);
> >> + strcat(file_name, "/i915_dsc_fec_support");
> >> + igt_debugfs_read(data->drm_fd, file_name, buf);
> >> +
> >> + return strstr(buf, "DSC_Enabled: yes"); }
> >> +
> >> +static void force_dp_dsc_enable(data_t *data) {
> >> + char file_name[128] = {0};
> >> + int ret;
> >> +
> >> + strcpy(file_name, data->conn_name);
> >> + strcat(file_name, "/i915_dsc_fec_support");
> >> + igt_debug ("Forcing DSC enable on %s\n", data->conn_name);
> >> + ret = igt_sysfs_write(data->debugfs_fd, file_name, "1", 1);
> >> + igt_assert_f(ret > 0, "debugfs_write failed"); }
> >> +
> >> +static void clear_dp_dsc_enable(data_t *data) {
> >> + char file_name[128] = {0};
> >> + int ret;
> >> +
> >> + strcpy(file_name, data->conn_name);
> >> + strcat(file_name, "/i915_dsc_fec_support");
> >> + igt_debug ("Clearing DSC enable on %s\n", data->conn_name);
> >> + ret = igt_sysfs_write(data->debugfs_fd, file_name, "0", 1);
> >> + igt_assert_f(ret > 0, "debugfs_write failed"); }
> >> +
> >> +static void test_cleanup(data_t *data) {
> >> + igt_plane_t *primary;
> >> +
> >> + if (data->output) {
> >> + primary = igt_output_get_plane_type(data->output,
> >> + DRM_PLANE_TYPE_PRIMARY);
> >> + igt_plane_set_fb(primary, NULL);
> >> + igt_display_commit(&data->display);
> >> +
> >> + igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> >> + }
> >> +}
> >
> >
> >Indentation on the if is wrong.
>
> Oops...will fix it.
>
> >
> >> +
> >> +
> >> +/*
> >> + * Re-probe connectors and do a modeset with DSC
> >> + *
> >> + */
> >> +static void update_display(data_t *data, enum dsc_test_type
> >> +test_type) {
> >> + igt_plane_t *primary;
> >> + data->mode = igt_output_get_mode(data->output);
> >> + data->connector = data->output->config.connector;
> >> +
> >> + if (data->connector->connector_type ==
> >DRM_MODE_CONNECTOR_DisplayPort &&
> >> + data->pipe == PIPE_A) {
> >> + igt_debug ("DSC not supported on Pipe A on external DP\n");
> >> + return;
> >> + }
> >> +
> >> + if (test_type == test_basic_dsc_enable) {
> >> + igt_debug ("DSC is supported on %s\n",
> >> + data->conn_name);
> >> + force_dp_dsc_enable(data);
> >> + /* Clear the fb in order to avoid leaks */
> >> + igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> >> + igt_create_pattern_fb(data->drm_fd, data->mode->hdisplay,
> >> + data->mode->vdisplay,
> >> + DRM_FORMAT_XRGB8888,
> >> + LOCAL_DRM_FORMAT_MOD_NONE,
> >> + &data->fb_test_pattern);
> >> + primary = igt_output_get_plane_type(data->output,
> >> +
> >DRM_PLANE_TYPE_PRIMARY);
> >> + igt_plane_set_fb(primary, &data->fb_test_pattern);
> >> + igt_display_commit(&data->display);
> >> + /* Until we have CRC check support, manually check if RGB test
> >pattern has no corruption */
> >> + manual ("RGB test pattern without corruption");
> >> + igt_assert_f(is_dp_dsc_enabled(data),
> >> + "Default DSC enable failed on Connector: %s Pipe:
> >%s",
> >> + data->conn_name,
> >> + kmstest_pipe_name(data->pipe));
> >> + clear_dp_dsc_enable(data);
> >> + } else {
> >> + igt_assert(!"Unknown test type\n");
> >> + }
> >> +}
> >> +
> >> +static void run_test(data_t *data, igt_output_t *output,
> >> + enum dsc_test_type test_type)
> >> +{
> >> + enum pipe pipe;
> >> +
> >> + for_each_pipe_static(pipe) {
> >
> >
> >This doesn't need to be the _static loop, for_each_pipe(data->display, pipe) can
> >be used.
>
> Hmmm...looking at both again, it says for_each_pipe_static() is to be used to enumerate per-pipe subtests and for_
> each_pipe() cannot be used to enumerate per-pipe subtests. So, if we want to add more subtests wont having _static be useful?
It means for_each_pipe_static is to be used if you're not inside an
igt_subtest or igt_fixture. Inside them you can use real runtime
information and for_each_pipe is ok.
In pseudocode, these are the use cases for the different forms:
for_each_pipe_static(pipe) {
igt_subtest_f("do-things-on-pipe-%s", kmstest_pipe_name(pipe)) {
...
}
}
igt_subtest("do-things") {
for_each_pipe(...) {
...
}
}
>
> >
> >> + if (igt_pipe_connector_valid(pipe, output)) {
> >> + igt_output_set_pipe(output, pipe);
> >> + data->pipe = pipe;
> >> + data->output = output;
> >> + update_display(data, test_type);
> >> + }
> >> + }
> >> +}
> >> +
> >> +igt_main
> >> +{
> >> + data_t data = {};
> >> + igt_output_t *output;
> >> + drmModeRes *res;
> >> + drmModeConnector *connector;
> >> + int i, test_conn_cnt, test_cnt;
> >> + int tests[] = {DRM_MODE_CONNECTOR_eDP,
> >> +DRM_MODE_CONNECTOR_DisplayPort};
> >> +
> >> + igt_fixture {
> >> + data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> >> + data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
> >> + kmstest_set_vt_graphics_mode();
> >> + igt_display_require(&data.display, data.drm_fd);
> >> + //igt_display_require_output(&data.display);
> >> + igt_require(res = drmModeGetResources(data.drm_fd));
> >> + }
> >> +
> >> + for (test_cnt = 0; test_cnt < ARRAY_SIZE(tests); test_cnt++) {
> >> +
> >> + igt_subtest_f("basic-dsc-enable-%s",
> >> + kmstest_connector_type_str(tests[test_cnt])) {
> >> + test_conn_cnt = 0;
> >> + for (i = 0; i < res->count_connectors; i++) {
> >> + connector =
> >drmModeGetConnectorCurrent(data.drm_fd,
> >> + res-
> >>connectors[i]);
> >> + if (connector->connection !=
> >DRM_MODE_CONNECTED ||
> >> + connector->connector_type !=
> >> + tests[test_cnt])
> >> + continue;
> >> + test_conn_cnt++;
> >> + output =
> >igt_output_from_connector(&data.display, connector);
> >> + sprintf(data.conn_name, "%s-%d",
> >> + kmstest_connector_type_str(connector-
> >>connector_type),
> >> + test_conn_cnt);
> >> + igt_require_f(is_dp_dsc_supported(&data),
> >> + "DSC not supported on connector %s
> >\n",
> >> + data.conn_name);
> >
> >This will make the subtest skip when checking DP-1, without trying
> >DP-2 at all. Is that intended?
> >
> No. it has to test for DP-2 as well..
> But even if the igt_require() here fails, the loop will move on to
> the next connector right?
No, it jumps out of the igt_subtest block, to the next iteration of
the outer loop, which loops over connector types.
--
Petri Latvala
More information about the igt-dev
mailing list