[igt-dev] [PATCH i-g-t 5/8] tests/i915/kms_dsc: Enable validation for VDSC YCbCr420
Hogander, Jouni
jouni.hogander at intel.com
Tue Jan 10 11:56:14 UTC 2023
Hello,
See my inline comments below.
On Mon, 2023-01-09 at 20:58 +0530, Swati Sharma wrote:
> Existing i-g-t is extended to enable validation for VDSC YCbCr420.
> 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 YCbCr420, we need a debugfs
> entry (force_dsc_ycbcr420) to force this output format; so that
> YCbCr420 code
> gets executed.
> From i-g-t, we have set this debugfs entry. However, before setting
> debugfs entry, we have checked capability i.e. YCbCr420 is supported
> by
> both platform (D14+) and sink.
> Also, all the modes doesn't support both YCbCr420 and RGB formats; so
> if
> sink and platform supports YCbCr420 we will do 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)
>
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
> lib/igt_kms.c | 17 ++++
> lib/igt_kms.h | 6 ++
> tests/i915/kms_dsc.c | 157 +++++++++++++++++++++++-----------
> --
> tests/i915/kms_dsc_helper.c | 49 +++++++++++
> tests/i915/kms_dsc_helper.h | 4 +
> 5 files changed, 177 insertions(+), 56 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 31e6dfda0..0f1f7c6b9 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -966,6 +966,23 @@ 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_RGB444, "RGB444" },
> + { DSC_FORMAT_YCBCR420, "YCBCR420" },
> + {}
> +};
> +
> +/**
> + * 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 be5482e08..61ec8fbec 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -106,10 +106,16 @@ enum igt_custom_edid_type {
> */
> #define kmstest_port_name(port) ((port) + 'A')
>
> +enum dsc_output_format {
> + DSC_FORMAT_RGB444,
> + DSC_FORMAT_YCBCR420
> +};
> +
> 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 f264d37a7..689cb81b1 100644
> --- a/tests/i915/kms_dsc.c
> +++ b/tests/i915/kms_dsc.c
> @@ -68,6 +68,19 @@ static drmModeModeInfo
> *get_highres_mode(igt_output_t *output)
> return highest_mode;
> }
>
> +static drmModeModeInfo *get_next_mode(igt_output_t *output, int
> count)
> +{
> + drmModeConnector *connector = output->config.connector;
> + drmModeModeInfo *next_mode = NULL;
> +
> + for (int i = count; i < connector->count_modes; i++) {
> + next_mode = &connector->modes[i];
> + break;
> + }
> +
> + return next_mode;
return output->config.connector->modes[count]; ?
> +}
> +
> static bool check_big_joiner_pipe_constraint(data_t *data)
> {
> igt_output_t *output = data->output;
> @@ -97,75 +110,103 @@ 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,
> - unsigned int plane_format)
> + unsigned int plane_format, enum
> dsc_output_format output_format)
> {
> + int ret;
> bool enabled;
> igt_plane_t *primary;
> drmModeModeInfo *mode;
> + int count = 0;
> + bool test_complete = false;
> igt_output_t *output = data->output;
> igt_display_t *display = &data->display;
>
> - /* sanitize the state before starting the subtest */
> - igt_display_reset(display);
> - igt_display_commit(display);
> + while (!test_complete) {
> + /* sanitize the state before starting the subtest */
> + igt_display_reset(display);
> + igt_display_commit(display);
>
> - igt_debug("DSC is supported on %s\n", data->output->name);
> - save_force_dsc_en(data);
> - force_dsc_enable(data);
> + igt_debug("DSC is supported on %s\n", data->output-
> >name);
> + save_force_dsc_en(data);
> + force_dsc_enable(data);
>
> - if (test_type == TEST_DSC_BPC) {
> - igt_debug("Trying to set input BPC to %d\n", data-
> >input_bpc);
> - force_dsc_enable_bpc(data);
> - }
> + if (output_format == DSC_FORMAT_YCBCR420) {
> + igt_debug("DSC YCbCr420 is supported on
> %s\n", data->output->name);
> + save_force_dsc_ycbcr420_en(data);
> + force_dsc_ycbcr420_enable(data);
> + }
>
> - igt_output_set_pipe(output, data->pipe);
> + if (test_type == TEST_DSC_BPC) {
> + igt_debug("Trying to set input BPC to %d\n",
> data->input_bpc);
> + force_dsc_enable_bpc(data);
> + }
>
> - mode = get_highres_mode(output);
> - igt_require(mode != NULL);
> - igt_output_override_mode(output, mode);
> + igt_output_set_pipe(output, data->pipe);
>
> - primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> + if (output_format == DSC_FORMAT_YCBCR420)
> + mode = get_next_mode(output, count);
> + else
> + mode = get_highres_mode(output);
If I understood your code correctly what you really want to loop is:
count = 0;
do {
if (output_format == DSC_FORMAT_YCBCR420)
mode = get_next_mode(output, count);
else
mode = get_highres_mode(output);
igt_require(mode != NULL);
igt_output_override_mode(output, mode);
} while(igt_display_try_commit_atomic(display,
DRM_MODE_ATOMIC_ALLOW_MODESET, NULL));
everything else seems to remain same over iterations and if
output_format != DSC_FORMAT_YCBCR420 even the mode is not changed.
Currently you are looping everything. E.g. igt_create_pattern_fb is
called several times and the igt_remove_fb is done only once in
test_cleanup.
> +
> + igt_require(mode != NULL);
> + igt_output_override_mode(output, mode);
> +
> + primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> +
> + igt_skip_on(!igt_plane_has_format_mod(primary,
> plane_format,
> + DRM_FORMAT_MOD_LINEAR));
> +
> + igt_create_pattern_fb(data->drm_fd,
> + mode->hdisplay,
> + mode->vdisplay,
> + 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 (ret == 0) {
> + igt_display_commit(display);
Isn't igt_display_try_commit_atomic enough or why you are doing
igt_display_commit even if igt_display_try_commit_atomic succeeded?
> + test_complete = true;
> + } else if (output_format == DSC_FORMAT_YCBCR420) {
> + count++;
> + continue;
> + }
> +
> + igt_assert_eq(ret, 0);
> +
> + /*
> + * Until we have CRC check support, manually check if
> RGB test
> + * pattern has no corruption.
> + */
> + manual("RGB test pattern without corruption");
> +
> + enabled = igt_is_dsc_enabled(data->drm_fd, output-
> >name);
> + igt_info("Current mode is: %dx%d @%dHz -- DSC is:
> %s\n",
> + mode->hdisplay,
> + mode->vdisplay,
> + mode->vrefresh,
> + enabled ? "ON" : "OFF");
> +
> + restore_force_dsc_en();
> + if (output_format == DSC_FORMAT_YCBCR420)
> + restore_force_dsc_ycbcr420_en();
>
> - igt_skip_on(!igt_plane_has_format_mod(primary, plane_format,
> - DRM_FORMAT_MOD_LINEAR));
> -
> - igt_create_pattern_fb(data->drm_fd,
> - mode->hdisplay,
> - mode->vdisplay,
> - plane_format,
> - DRM_FORMAT_MOD_LINEAR,
> - &data->fb_test_pattern);
> -
> - igt_plane_set_fb(primary, &data->fb_test_pattern);
> - igt_display_commit(display);
> -
> - /* until we have CRC check support, manually check if RGB
> test
> - * pattern has no corruption.
> - */
> - manual("RGB test pattern without corruption");
> -
> - enabled = igt_is_dsc_enabled(data->drm_fd, output->name);
> - igt_info("Current mode is: %dx%d @%dHz -- DSC is: %s\n",
> - mode->hdisplay,
> - mode->vdisplay,
> - mode->vrefresh,
> - enabled ? "ON" : "OFF");
> -
> - restore_force_dsc_en();
> - igt_debug("Reset compression BPC\n");
> - data->input_bpc = 0;
> - force_dsc_enable_bpc(data);
> -
> - igt_assert_f(enabled,
> - "Default DSC enable failed on connector: %s
> pipe: %s\n",
> - output->name,
> - kmstest_pipe_name(data->pipe));
> -
> - test_cleanup(data);
> + igt_debug("Reset input BPC\n");
> + data->input_bpc = 0;
> + force_dsc_enable_bpc(data);
> +
> + igt_assert_f(enabled,
> + "Default DSC enable failed on connector:
> %s pipe: %s\n",
> + output->name,
> + kmstest_pipe_name(data->pipe));
> +
> + test_cleanup(data);
> + }
> }
>
> 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;
> @@ -194,15 +235,19 @@ static void test_dsc(data_t *data, enum
> dsc_test_type test_type, int bpc,
> else
> snprintf(name, sizeof(name), "-%s",
> igt_format_str(plane_format));
>
> - igt_dynamic_f("pipe-%s-%s%s",
> kmstest_pipe_name(data->pipe), data->output->name, name)
> - update_display(data, test_type,
> plane_format);
> + igt_dynamic_f("pipe-%s-%s%s-%s",
> kmstest_pipe_name(data->pipe), data->output->name, name,
> kmstest_dsc_output_format_str(output_format))
> + update_display(data, test_type, plane_format,
> output_format);
> }
> }
>
> static void run_test(data_t *data, enum dsc_test_type test_type, int
> bpc,
> unsigned int plane_format)
> {
> - test_dsc(data, test_type, bpc, plane_format);
> + test_dsc(data, test_type, bpc, plane_format,
> DSC_FORMAT_RGB444);
> +
> + /* YCbCr420 DSC is supported on display version 14+ */
> + if ((data->disp_ver >= 14) &&
> (is_dsc_ycbcr420_supported(data)))
> + test_dsc(data, test_type, bpc, plane_format,
> DSC_FORMAT_YCBCR420);
> }
>
> igt_main
> diff --git a/tests/i915/kms_dsc_helper.c
> b/tests/i915/kms_dsc_helper.c
> index 7b75be82e..8f0326f13 100644
> --- a/tests/i915/kms_dsc_helper.c
> +++ b/tests/i915/kms_dsc_helper.c
> @@ -30,7 +30,9 @@
> #include "kms_dsc_helper.h"
>
> bool force_dsc_en_orig;
> +bool force_dsc_ycbcr420_en_orig;
> int force_dsc_restore_fd = -1;
> +int force_dsc_ycbcr420_restore_fd = -1;
>
> void force_dsc_enable(data_t *data)
> {
> @@ -80,6 +82,7 @@ void restore_force_dsc_en(void)
> void kms_dsc_exit_handler(int sig)
> {
> restore_force_dsc_en();
> + restore_force_dsc_ycbcr420_en();
> }
>
> bool check_dsc_on_connector(data_t *data)
> @@ -129,3 +132,49 @@ bool check_gen11_bpc_constraint(data_t *data)
>
> return true;
> }
> +
> +void force_dsc_ycbcr420_enable(data_t *data)
> +{
> + int ret;
> +
> + igt_debug("Forcing DSC YCbCr420 on %s\n", data->output-
> >name);
> + ret = igt_force_dsc_ycbcr420_enable(data->drm_fd,
> + data->output->name);
> + igt_assert_f(ret > 0, "forcing dsc ycbcr420 debugfs_write
> failed\n");
> +}
> +
> +void save_force_dsc_ycbcr420_en(data_t *data)
> +{
> + force_dsc_ycbcr420_en_orig =
> + igt_is_force_dsc_ycbcr420_enabled(data->drm_fd,
> + data->output-
> >name);
> + force_dsc_ycbcr420_restore_fd =
> + igt_get_dsc_ycbcr420_debugfs_fd(data->drm_fd,
> + data->output->name);
> + igt_assert(force_dsc_ycbcr420_restore_fd >= 0);
> +}
> +
> +void restore_force_dsc_ycbcr420_en(void)
> +{
> + if (force_dsc_ycbcr420_restore_fd < 0)
> + return;
> +
> + igt_debug("Restoring DSC YCbCr420 enable\n");
> + igt_assert(write(force_dsc_ycbcr420_restore_fd,
> force_dsc_ycbcr420_en_orig ? "1" : "0", 1) == 1);
> +
> + close(force_dsc_ycbcr420_restore_fd);
> + force_dsc_ycbcr420_restore_fd = -1;
> +}
> +
> +bool is_dsc_ycbcr420_supported(data_t *data)
> +{
> + igt_output_t *output = data->output;
> +
> + if (!igt_is_dsc_ycbcr420_supported(data->drm_fd, output-
> >name)) {
> + igt_debug("DSC YCbCr420 not supported on connector
> %s\n",
> + output->name);
> + return false;
> + }
> +
> + return true;
> +}
> diff --git a/tests/i915/kms_dsc_helper.h
> b/tests/i915/kms_dsc_helper.h
> index cfca2ed98..57cb67737 100644
> --- a/tests/i915/kms_dsc_helper.h
> +++ b/tests/i915/kms_dsc_helper.h
> @@ -62,5 +62,9 @@ void kms_dsc_exit_handler(int sig);
> bool check_dsc_on_connector(data_t *data);
> bool check_gen11_dp_constraint(data_t *data);
> bool check_gen11_bpc_constraint(data_t *data);
> +void force_dsc_ycbcr420_enable(data_t *data);
> +void save_force_dsc_ycbcr420_en(data_t *data);
> +void restore_force_dsc_ycbcr420_en(void);
> +bool is_dsc_ycbcr420_supported(data_t *data);
>
> #endif
More information about the igt-dev
mailing list