[igt-dev] [PATCH i-g-t 2/2] tests/i915/kms_dsc: Enable validation for VDSC output formats
Modem, Bhanuprakash
bhanuprakash.modem at intel.com
Thu May 4 05:40:38 UTC 2023
Hi Swati,
On Wed-03-05-2023 05:10 pm, Swati Sharma wrote:
> Existing i-g-t is extended to enable validation for VDSC output
> formats. Output format is selected as per driver policy.For ex:
> If a mode is supported in both RGB and YCbCr420 output formats
> by the sink, i915 driver policy is to try RGB first and fall
> back to YCbCr420, if mode cannot be shown using RGB.
>
> To test DSC output format, a debugfs entry is created to force
> this output format. However, before setting debugfs entry, we
> have checked capability i.e. output format is supported by both
> platform and sink.
>
> Also, each mode doesn't support all output formats; so if both
> sink and platform support an output format, we will do a try
> commit with each mode till we get a successful commit.
>
> v2: -used is_dsc_ycbcr420_supported() (Ankit)
> -handled try-commit correctly (Ankit)
> -instead of flag use enum for output formats (Ankit)
> v3: -instead of global count, pass count as para (Ankit)
> -print output format (Ankit)
> v4: -optimized while loop (Jouni)
> -used only try commit (Jouni)
> -fixed get_next_mode() (Jouni)
> v5: -made generic test to validate all output formats (Jani N)
> v6: -fixed assert (Jouni)
> -if mode == NULL output_format debugfs reset didn't happen, fixed
> v7: -indentation fixes (Kamil)
> v8: -rebase
> -removed pipe_output_combo_valid() (Bhanu)
>
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> Reviewed-by: Jouni Högander <jouni.hogander at intel.com> (v5)
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> ---
> lib/igt_kms.c | 18 +++++
> lib/igt_kms.h | 1 +
> tests/i915/kms_dsc.c | 136 +++++++++++++++++++++++++-----------
> tests/i915/kms_dsc_helper.c | 36 ++++++++++
> tests/i915/kms_dsc_helper.h | 4 ++
> 5 files changed, 156 insertions(+), 39 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e085e234..c5484148 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -967,6 +967,24 @@ const char *kmstest_scaling_filter_str(int filter)
> return find_type_name(scaling_filter_names, filter);
> }
>
> +static const struct type_name dsc_output_format_names[] = {
> + { DSC_FORMAT_RGB, "RGB" },
> + { DSC_FORMAT_YCBCR420, "YCBCR420" },
> + { DSC_FORMAT_YCBCR444, "YCBCR444" },
> + {}
> +};
> +
> +/**
> + * kmstest_dsc_output_format_str:
> + * @output_format: DSC_FORMAT_* output format value
> + *
> + * Returns: A string representing the output format @output format.
> + */
> +const char *kmstest_dsc_output_format_str(int output_format)
> +{
> + return find_type_name(dsc_output_format_names, output_format);
> +}
> +
> static const struct type_name connector_type_names[] = {
> { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
> { DRM_MODE_CONNECTOR_VGA, "VGA" },
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 41e041b6..1b6988c1 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -118,6 +118,7 @@ const char *kmstest_encoder_type_str(int type);
> const char *kmstest_connector_status_str(int status);
> const char *kmstest_connector_type_str(int type);
> const char *kmstest_scaling_filter_str(int filter);
> +const char *kmstest_dsc_output_format_str(int output_format);
>
> void kmstest_dump_mode(drmModeModeInfo *mode);
> #define MAX_HDISPLAY_PER_PIPE 5120
> diff --git a/tests/i915/kms_dsc.c b/tests/i915/kms_dsc.c
> index c4dfc925..d441c356 100644
> --- a/tests/i915/kms_dsc.c
> +++ b/tests/i915/kms_dsc.c
> @@ -36,7 +36,8 @@ IGT_TEST_DESCRIPTION("Test to validate display stream compression");
>
> enum dsc_test_type {
> TEST_DSC_BASIC,
> - TEST_DSC_BPC
> + TEST_DSC_BPC,
> + TEST_DSC_OUTPUT_FORMAT,
> };
>
> typedef struct {
> @@ -44,6 +45,7 @@ typedef struct {
> uint32_t devid;
> igt_display_t display;
> struct igt_fb fb_test_pattern;
> + enum dsc_output_format output_format;
> unsigned int plane_format;
> igt_output_t *output;
> int input_bpc;
> @@ -51,7 +53,8 @@ typedef struct {
> enum pipe pipe;
> } data_t;
>
> -static int format_list[] = {DRM_FORMAT_XYUV8888, DRM_FORMAT_XRGB2101010, DRM_FORMAT_XRGB16161616F, DRM_FORMAT_YUYV};
> +static int output_format_list[] = {DSC_FORMAT_YCBCR420, DSC_FORMAT_YCBCR444};
> +static int format_list[] = {DRM_FORMAT_XYUV8888, DRM_FORMAT_XRGB2101010, DRM_FORMAT_XRGB16161616F, DRM_FORMAT_YUYV};
> static uint32_t bpc_list[] = {12, 10, 8};
>
> static inline void manual(const char *expected)
> @@ -71,24 +74,26 @@ static drmModeModeInfo *get_highres_mode(igt_output_t *output)
> return highest_mode;
> }
>
> -static bool pipe_output_combo_valid(data_t *data)
> +static drmModeModeInfo *get_next_mode(igt_output_t *output, int index)
> {
> - igt_output_t *output = data->output;
> - drmModeModeInfo *mode;
> - bool ret = true;
> -
> - igt_display_reset(&data->display);
> - mode = get_highres_mode(output);
> + drmModeConnector *connector = output->config.connector;
> + drmModeModeInfo *next_mode = NULL;
>
> - igt_output_set_pipe(output, data->pipe);
> - igt_output_override_mode(output, mode);
> + if (index < connector->count_modes)
> + next_mode = &connector->modes[index];
>
> - if (!i915_pipe_output_combo_valid(&data->display))
> - ret = false;
> + return next_mode;
> +}
>
> - igt_output_set_pipe(output, PIPE_NONE);
> +static void test_reset(data_t *data)
> +{
> + igt_debug("Reset input BPC\n");
> + data->input_bpc = 0;
> + force_dsc_enable_bpc(data->drm_fd, data->output, data->input_bpc);
>
> - return ret;
> + igt_debug("Reset DSC output format\n");
> + data->output_format = DSC_FORMAT_RGB;
> + force_dsc_output_format(data->drm_fd, data->output, data->output_format);
> }
>
> static void test_cleanup(data_t *data)
> @@ -106,7 +111,9 @@ static void test_cleanup(data_t *data)
> /* re-probe connectors and do a modeset with DSC */
> static void update_display(data_t *data, enum dsc_test_type test_type)
> {
> + int ret;
> bool enabled;
> + int index = 0;
> igt_plane_t *primary;
> drmModeModeInfo *mode;
> igt_output_t *output = data->output;
> @@ -125,26 +132,57 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> force_dsc_enable_bpc(data->drm_fd, data->output, data->input_bpc);
> }
>
> - igt_output_set_pipe(output, data->pipe);
> -
> - mode = get_highres_mode(output);
> - igt_require(mode != NULL);
> - igt_output_override_mode(output, mode);
> + if (test_type == TEST_DSC_OUTPUT_FORMAT) {
> + igt_debug("Trying to set DSC %s output format\n",
> + kmstest_dsc_output_format_str(data->output_format));
> + force_dsc_output_format(data->drm_fd, data->output, data->output_format);
> + }
>
> + igt_output_set_pipe(output, data->pipe);
> primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>
> igt_skip_on(!igt_plane_has_format_mod(primary, data->plane_format,
> DRM_FORMAT_MOD_LINEAR));
>
> - igt_create_pattern_fb(data->drm_fd,
> - mode->hdisplay,
> - mode->vdisplay,
> - data->plane_format,
> - DRM_FORMAT_MOD_LINEAR,
> - &data->fb_test_pattern);
> + do {
> + if (data->output_format == DSC_FORMAT_RGB)
> + mode = get_highres_mode(output);
> + else
> + mode = get_next_mode(output, index++);
>
> - igt_plane_set_fb(primary, &data->fb_test_pattern);
> - igt_display_commit(display);
> + if (mode == NULL)
> + goto reset;
> +
> + igt_output_override_mode(output, mode);
> +
> + if (!i915_pipe_output_combo_valid(display)) {
> + if (data->output_format == DSC_FORMAT_RGB) {
> + igt_info("No valid pipe/output/mode found.\n");
> +
> + mode = NULL;
> + goto reset;
> + } else {
> + continue;
> + }
> + }
> +
> + igt_create_pattern_fb(data->drm_fd,
> + mode->hdisplay,
> + mode->vdisplay,
> + data->plane_format,
> + DRM_FORMAT_MOD_LINEAR,
> + &data->fb_test_pattern);
> + igt_plane_set_fb(primary, &data->fb_test_pattern);
> +
> + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> + if (data->output_format == DSC_FORMAT_RGB || ret == 0)
> + break;
> +
> + igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> + } while (1);
It is already well takencare, but still while(1) (an infinite loop)
looks bit dangerous to me.
How about using "while (index < connector->count_modes)"?
Still this patch LGTM
Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
- Bhanu
> +
> + if (ret != 0)
> + goto reset;
>
> /* until we have CRC check support, manually check if RGB test
> * pattern has no corruption.
> @@ -159,20 +197,22 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> enabled ? "ON" : "OFF");
>
> restore_force_dsc_en();
> - igt_debug("Reset compression BPC\n");
> - data->input_bpc = 0;
> - force_dsc_enable_bpc(data->drm_fd, data->output, data->input_bpc);
>
> igt_assert_f(enabled,
> "Default DSC enable failed on connector: %s pipe: %s\n",
> output->name,
> kmstest_pipe_name(data->pipe));
>
> +reset:
> + test_reset(data);
> +
> test_cleanup(data);
> + igt_skip_on(mode == NULL);
> + igt_assert_eq(ret, 0);
> }
>
> static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
> - unsigned int plane_format)
> + unsigned int plane_format, enum dsc_output_format output_format)
> {
> igt_display_t *display = &data->display;
> igt_output_t *output;
> @@ -180,6 +220,7 @@ static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
> enum pipe pipe;
>
> for_each_pipe_with_valid_output(display, pipe, output) {
> + data->output_format = output_format;
> data->plane_format = plane_format;
> data->input_bpc = bpc;
> data->output = output;
> @@ -188,17 +229,21 @@ static void test_dsc(data_t *data, enum dsc_test_type test_type, int bpc,
> if (!check_dsc_on_connector(data->drm_fd, data->output))
> continue;
>
> - if (!check_gen11_dp_constraint(data->drm_fd, data->output, data->pipe))
> + if (!is_dsc_output_format_supported(data->drm_fd, data->disp_ver,
> + data->output, data->output_format))
> continue;
>
> - if (!check_gen11_bpc_constraint(data->drm_fd, data->output, data->input_bpc))
> + if (!check_gen11_dp_constraint(data->drm_fd, data->output, data->pipe))
> continue;
>
> - if (!pipe_output_combo_valid(data))
> + if (!check_gen11_bpc_constraint(data->drm_fd, data->output, data->input_bpc))
> continue;
>
> if (test_type == TEST_DSC_BPC)
> snprintf(name, sizeof(name), "-%dbpc-%s", data->input_bpc, igt_format_str(data->plane_format));
> + else if (test_type == TEST_DSC_OUTPUT_FORMAT)
> + snprintf(name, sizeof(name), "-%s-%s", kmstest_dsc_output_format_str(data->output_format),
> + igt_format_str(data->plane_format));
> else
> snprintf(name, sizeof(name), "-%s", igt_format_str(data->plane_format));
>
> @@ -226,14 +271,16 @@ igt_main
> "by a connector by forcing DSC on all connectors that support it "
> "with default parameters");
> igt_subtest_with_dynamic("dsc-basic")
> - test_dsc(&data, TEST_DSC_BASIC, 0, DRM_FORMAT_XRGB8888);
> + test_dsc(&data, TEST_DSC_BASIC, 0,
> + DRM_FORMAT_XRGB8888, DSC_FORMAT_RGB);
>
> igt_describe("Tests basic display stream compression functionality if supported "
> "by a connector by forcing DSC on all connectors that support it "
> "with default parameters and creating fb with diff formats");
> igt_subtest_with_dynamic("dsc-with-formats") {
> for (int k = 0; k < ARRAY_SIZE(format_list); k++)
> - test_dsc(&data, TEST_DSC_BASIC, 0, format_list[k]);
> + test_dsc(&data, TEST_DSC_BASIC, 0,
> + format_list[k], DSC_FORMAT_RGB);
> }
>
> igt_describe("Tests basic display stream compression functionality if supported "
> @@ -241,7 +288,8 @@ igt_main
> "with certain input BPC for the connector");
> igt_subtest_with_dynamic("dsc-with-bpc") {
> for (int j = 0; j < ARRAY_SIZE(bpc_list); j++)
> - test_dsc(&data, TEST_DSC_BPC, bpc_list[j], DRM_FORMAT_XRGB8888);
> + test_dsc(&data, TEST_DSC_BPC, bpc_list[j],
> + DRM_FORMAT_XRGB8888, DSC_FORMAT_RGB);
> }
>
> igt_describe("Tests basic display stream compression functionality if supported "
> @@ -250,11 +298,21 @@ igt_main
> igt_subtest_with_dynamic("dsc-with-bpc-formats") {
> for (int j = 0; j < ARRAY_SIZE(bpc_list); j++) {
> for (int k = 0; k < ARRAY_SIZE(format_list); k++) {
> - test_dsc(&data, TEST_DSC_BPC, bpc_list[j], format_list[k]);
> + test_dsc(&data, TEST_DSC_BPC, bpc_list[j],
> + format_list[k], DSC_FORMAT_RGB);
> }
> }
> }
>
> + igt_describe("Tests basic display stream compression functionality if supported "
> + "by a connector by forcing DSC and output format on all connectors "
> + "that support it");
> + igt_subtest_with_dynamic("dsc-with-output-formats") {
> + for (int k = 0; k < ARRAY_SIZE(output_format_list); k++)
> + test_dsc(&data, TEST_DSC_OUTPUT_FORMAT, 0, DRM_FORMAT_XRGB8888,
> + output_format_list[k]);
> + }
> +
> igt_fixture {
> igt_display_fini(&data.display);
> close(data.drm_fd);
> diff --git a/tests/i915/kms_dsc_helper.c b/tests/i915/kms_dsc_helper.c
> index e2c278c7..02d1a484 100644
> --- a/tests/i915/kms_dsc_helper.c
> +++ b/tests/i915/kms_dsc_helper.c
> @@ -97,3 +97,39 @@ bool check_gen11_bpc_constraint(int drmfd, igt_output_t *output, int input_bpc)
>
> return true;
> }
> +
> +void force_dsc_output_format(int drmfd, igt_output_t *output,
> + enum dsc_output_format output_format)
> +{
> + int ret;
> +
> + igt_debug("Forcing DSC %s output format on %s\n",
> + kmstest_dsc_output_format_str(output_format), output->name);
> + ret = igt_force_dsc_output_format(drmfd, output->name, output_format);
> + igt_assert_f(ret == 0, "forcing dsc output format debugfs_write failed\n");
> +}
> +
> +/* YCbCr420 DSC is supported on display version 14+ with DSC1.2a */
> +static bool is_dsc_output_format_supported_by_platform(int disp_ver, enum dsc_output_format output_format)
> +{
> + if (disp_ver < 14 && output_format == DSC_FORMAT_YCBCR420) {
> + igt_debug("Output format DSC YCBCR420 not supported on D13 and older platforms\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +bool is_dsc_output_format_supported(int drmfd, int disp_ver, igt_output_t *output,
> + enum dsc_output_format output_format)
> +{
> + if (!(igt_is_dsc_output_format_supported_by_sink(drmfd, output->name, output_format)) &&
> + (is_dsc_output_format_supported_by_platform(disp_ver, output_format))) {
> + igt_debug("DSC %s output format not supported on connector %s\n",
> + kmstest_dsc_output_format_str(output_format),
> + output->name);
> + return false;
> + }
> +
> + return true;
> +}
> diff --git a/tests/i915/kms_dsc_helper.h b/tests/i915/kms_dsc_helper.h
> index b3828dcd..198f5afa 100644
> --- a/tests/i915/kms_dsc_helper.h
> +++ b/tests/i915/kms_dsc_helper.h
> @@ -29,5 +29,9 @@ void kms_dsc_exit_handler(int sig);
> bool check_dsc_on_connector(int drmfd, igt_output_t *output);
> bool check_gen11_dp_constraint(int drmfd, igt_output_t *output, enum pipe pipe);
> bool check_gen11_bpc_constraint(int drmfd, igt_output_t *output, int input_bpc);
> +void force_dsc_output_format(int drmfd, igt_output_t *output,
> + enum dsc_output_format output_format);
> +bool is_dsc_output_format_supported(int disp_ver, int drmfd, igt_output_t *output,
> + enum dsc_output_format output_format);
>
> #endif
More information about the igt-dev
mailing list