Re: [igt-dev] [PATCH i-g-t v4 5/8] tests/i915/kms_dsc: Enable validation for VDSC YCbCr420

Hogander, Jouni
2023-01-13T08:30:04Z

Hello Swati, couple of inline comments below.

On Thu, 2023-01-12 at 13:05 +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)
> v4: -optimized while loop (Jouni)
>     -used only try commit (Jouni)
>     -fixed get_next_mode() (Jouni)
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  lib/igt_kms.c               | 17 ++++++++
>  lib/igt_kms.h               |  6 +++
>  tests/i915/kms_dsc.c        | 84 +++++++++++++++++++++++++++--------
> --
>  tests/i915/kms_dsc_helper.c | 45 ++++++++++++++++++++
>  tests/i915/kms_dsc_helper.h |  4 ++
>  5 files changed, 133 insertions(+), 23 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 0bac9d26b..366205a4e 100644
> --- a/tests/i915/kms_dsc.c
> +++ b/tests/i915/kms_dsc.c
> @@ -80,6 +80,17 @@ static drmModeModeInfo
> *get_highres_mode(igt_output_t *output)
>         return highest_mode;
>  }
>
> +static drmModeModeInfo *get_next_mode(igt_output_t *output, int
> index)
> +{
> +       drmModeConnector *connector = output->config.connector;
> +       drmModeModeInfo *next_mode = NULL;
> +
> +       if (index < connector->count_modes)
> +               next_mode = &connector->modes[index];
> +
> +       return next_mode;
> +}
> +
>  static bool check_big_joiner_pipe_constraint(data_t *data)
>  {
>         igt_output_t *output = data->output;
> @@ -109,9 +120,11 @@ 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;
> +       int index = 0;
>         igt_plane_t *primary;
>         drmModeModeInfo *mode;
>         igt_output_t *output = data->output;
> @@ -125,33 +138,51 @@ static void update_display(data_t *data, enum
> dsc_test_type test_type,
>         save_force_dsc_en(data->drm_fd, data->output);
>         force_dsc_enable(data->drm_fd, data->output);
>
> +       if (output_format == DSC_FORMAT_YCBCR420) {
> +               igt_debug("DSC YCbCr420 is supported on %s\n", data-
> >output->name);
> +               save_force_dsc_ycbcr420_en(data->drm_fd, data-
> >output);
> +               force_dsc_ycbcr420_enable(data->drm_fd, data-
> >output);
> +       }
> +
>         if (test_type == TEST_DSC_BPC) {
>                 igt_debug("Trying to set input BPC to %d\n", data-
> >input_bpc);
>                 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);
> -
>         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);
> -       igt_display_commit(display);
> -
> -       /* until we have CRC check support, manually check if RGB
> test
> +       do {
> +               if (output_format == DSC_FORMAT_YCBCR420) {
> +                       mode = get_next_mode(output, index);
> +                       index++;
> +               } else
> +                       mode = get_highres_mode(output);
> +
> +               igt_require(mode != NULL);
> +               igt_output_override_mode(output, mode);
> +               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 (output_format == DSC_FORMAT_YCBCR420 && ret != 0)
> {
> +                       igt_remove_fb(data->drm_fd, &data-
> >fb_test_pattern);
> +                               continue;

Indentation.

> +               } else
> +                       break;

Else is not necessary. You can just break. I'm fine if you think it's
more clear this way. If you decide to keep it: please add braces to
else as well (coding style).

> +       } while(1);
> +
> +       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");
> @@ -164,7 +195,10 @@ 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");
> +       if (output_format == DSC_FORMAT_YCBCR420)
> +               restore_force_dsc_ycbcr420_en();
> +
> +       igt_debug("Reset input BPC\n");
>         data->input_bpc = 0;
>         force_dsc_enable_bpc(data->drm_fd, data->output, data-
> >input_bpc);
>
> @@ -177,7 +211,7 @@ static void update_display(data_t *data, enum
> dsc_test_type test_type,
>  }
>
>  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;
> @@ -206,15 +240,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->drm_fd, data->output)))
> +               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 1a11f84ef..135e9cf58 100644
> --- a/tests/i915/kms_dsc_helper.c
> +++ b/tests/i915/kms_dsc_helper.c
> @@ -6,7 +6,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;

Static

>
>  void force_dsc_enable(int drmfd, igt_output_t *output)
>  {
> @@ -51,6 +53,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(int drmfd, igt_output_t *output)
> @@ -97,3 +100,45 @@ bool check_gen11_bpc_constraint(int drmfd,
> igt_output_t *output, int input_bpc)
>
>         return true;
>  }
> +
> +void force_dsc_ycbcr420_enable(int drmfd, igt_output_t *output)
> +{
> +       int ret;
> +
> +       igt_debug("Forcing DSC YCbCr420 on %s\n", output->name);
> +       ret = igt_force_dsc_ycbcr420_enable(drmfd,
> +                                           output->name);
> +       igt_assert_f(ret > 0, "forcing dsc ycbcr420 debugfs_write
> failed\n");
> +}
> +
> +void save_force_dsc_ycbcr420_en(int drmfd, igt_output_t *output)
> +{
> +       force_dsc_ycbcr420_en_orig =
> +               igt_is_force_dsc_ycbcr420_enabled(drmfd, output-
> >name);
> +       force_dsc_ycbcr420_restore_fd =
> +               igt_get_dsc_ycbcr420_debugfs_fd(drmfd, 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(int drmfd, igt_output_t *output)
> +{
> +       if (!igt_is_dsc_ycbcr420_supported(drmfd, 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 fe479dac4..83905fafe 100644
> --- a/tests/i915/kms_dsc_helper.h
> +++ b/tests/i915/kms_dsc_helper.h
> @@ -31,5 +31,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_ycbcr420_enable(int drmfd, igt_output_t *output);
> +void save_force_dsc_ycbcr420_en(int drmfd, igt_output_t *output);
> +void restore_force_dsc_ycbcr420_en(void);
> +bool is_dsc_ycbcr420_supported(int drmfd, igt_output_t *output);
>
>  #endif