[igt-dev] [PATCH i-g-t] test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP
Srivatsa, Anusha
anusha.srivatsa at intel.com
Thu Dec 20 19:09:06 UTC 2018
>-----Original Message-----
>From: Latvala, Petri
>Sent: Tuesday, December 18, 2018 4:13 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 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(...) {
> ...
> }
>}
Thanks for the explanation Petri, I will make this change.
>
>>
>> >
>> >> + 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.
>
Yes, will avoid using igt_require here.
Thanks,
Anusha
>--
>Petri Latvala
More information about the igt-dev
mailing list