From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0A52110E38B for ; Thu, 4 May 2023 05:41:11 +0000 (UTC) Message-ID: Date: Thu, 4 May 2023 11:10:38 +0530 Content-Language: en-US To: Swati Sharma , References: <20230503114014.132968-1-swati2.sharma@intel.com> <20230503114014.132968-3-swati2.sharma@intel.com> From: "Modem, Bhanuprakash" In-Reply-To: <20230503114014.132968-3-swati2.sharma@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/i915/kms_dsc: Enable validation for VDSC output formats List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 > Reviewed-by: Jouni Högander (v5) > Reviewed-by: Ankit Nautiyal > --- > 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 - 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