From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6824710EC11 for ; Tue, 21 Jun 2022 06:39:44 +0000 (UTC) Message-ID: Date: Tue, 21 Jun 2022 12:09:15 +0530 Content-Language: en-US To: Bhanuprakash Modem , References: <20220615061123.4158764-1-bhanuprakash.modem@intel.com> From: "Nautiyal, Ankit K" In-Reply-To: <20220615061123.4158764-1-bhanuprakash.modem@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Subject: Re: [igt-dev] [i-g-t] tests/kms_dither: Skip if current & requested BPC doesn't match List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Hi Bhanu, Please find the comments inline. On 6/15/2022 11:41 AM, Bhanuprakash Modem wrote: > The "max bpc" property only ensures that the bpc will not go beyond > the value set through this property. It does not guarantee that the > same bpc will be used for the given mode. > > If clock/bandwidth constraints permit, the max bpc will be used to > show the mode, otherwise the bpc will be reduced. So, if we really > want a particular bpc set, we can try reducing the resolution, till > we get the bpc that we set in max bpc property. > > This patch will skip the test, if there is no valid resolution to get > the same bpc as set by max_bpc property. > > Cc: Swati Sharma > CC: Ankit Nautiyal > Signed-off-by: Bhanuprakash Modem > --- > tests/kms_dither.c | 72 ++++++++++++++++++++++++---------------------- > 1 file changed, 37 insertions(+), 35 deletions(-) > > diff --git a/tests/kms_dither.c b/tests/kms_dither.c > index c72f83be..02896b37 100644 > --- a/tests/kms_dither.c > +++ b/tests/kms_dither.c > @@ -46,10 +46,6 @@ IGT_TEST_DESCRIPTION("Test Dithering block status"); > typedef struct data { > igt_display_t display; > igt_plane_t *primary; > - igt_output_t *output; > - igt_pipe_t *pipe; > - drmModeModeInfo *mode; > - enum pipe pipe_id; Perhaps commit message can have a line for this change as well. > int drm_fd; > igt_fb_t fb; > } data_t; > @@ -60,30 +56,23 @@ typedef struct { > } dither_status_t; > > /* Prepare test data. */ > -static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe) > +static void prepare_test(data_t *data, igt_output_t *output, enum pipe p) > { > igt_display_t *display = &data->display; > + igt_pipe_t *pipe = &data->display.pipes[p]; > > - data->pipe_id = pipe; > - data->pipe = &data->display.pipes[data->pipe_id]; > - igt_assert(data->pipe); > + igt_assert(pipe); > > igt_display_reset(display); > > - data->output = output; > - igt_assert(data->output); > - > - data->mode = igt_output_get_mode(data->output); > - igt_assert(data->mode); > - > data->primary = > - igt_pipe_get_plane_type(data->pipe, DRM_PLANE_TYPE_PRIMARY); > + igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY); > > - igt_output_set_pipe(data->output, data->pipe_id); > + igt_output_set_pipe(output, p); > } > > /* Returns the current state of dithering from the crtc debugfs. */ > -static dither_status_t get_dither_state(data_t *data) > +static dither_status_t get_dither_state(data_t *data, enum pipe pipe) > { > char buf[512], tmp[5]; > char *start_loc; > @@ -103,11 +92,34 @@ static dither_status_t get_dither_state(data_t *data) > igt_assert_eq(sscanf(start_loc, ", dither=%s", tmp), 1); > status.dither = !strcmp(tmp, "yes,"); > > - status.bpc = igt_get_pipe_current_bpc(data->drm_fd, data->pipe_id); > + status.bpc = igt_get_pipe_current_bpc(data->drm_fd, pipe); > > return status; > } > > +static bool i915_clock_constraint(data_t *data, enum pipe pipe, > + igt_output_t *output, int bpc) > +{ > + drmModeConnector *connector = output->config.connector; > + igt_display_t *display = &data->display; > + > + igt_sort_connector_modes(connector, sort_drm_modes_by_clk_dsc); > + > + for_each_connector_mode(output) { > + igt_output_override_mode(output, &connector->modes[j__]); > + igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); I think this should be igt_display_try_commit2, otherwise the test will fail and not try different modes. Otherwise patch seems to be fine to me. Regards, Ankit > + > + if (!igt_check_output_bpc_equal(data->drm_fd, pipe, > + output->name, bpc)) > + continue; > + > + return true; > + } > + > + igt_output_override_mode(output, NULL); > + return false; > +} > + > static void test_dithering(data_t *data, enum pipe pipe, > igt_output_t *output, > int fb_bpc, int fb_format, > @@ -121,14 +133,12 @@ static void test_dithering(data_t *data, enum pipe pipe, > output->name, kmstest_pipe_name(pipe)); > prepare_test(data, output, pipe); > > - igt_assert(igt_create_fb(data->drm_fd, data->mode->hdisplay, > - data->mode->vdisplay, fb_format, > + igt_assert(igt_create_fb(data->drm_fd, 512, 512, fb_format, > DRM_FORMAT_MOD_LINEAR, &data->fb)); > igt_plane_set_fb(data->primary, &data->fb); > - igt_plane_set_size(data->primary, data->mode->hdisplay, data->mode->vdisplay); > > bpc = igt_output_get_prop(output, IGT_CONNECTOR_MAX_BPC); > - igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, output_bpc); > + igt_output_set_prop_value(output, IGT_CONNECTOR_MAX_BPC, output_bpc); > > if (display->is_atomic) > ret = igt_display_try_commit_atomic(display, > @@ -141,12 +151,9 @@ static void test_dithering(data_t *data, enum pipe pipe, > igt_require_f(!ret, "%s don't support %d-bpc\n", > output->name, output_bpc); > > - igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > - > - if (!igt_check_output_bpc_equal(data->drm_fd, pipe, output->name, output_bpc)) { > - igt_output_set_prop_value(output, IGT_CONNECTOR_MAX_BPC, bpc); > - igt_fail_on_f(true, "Failed to set max_bpc as: %d\n", output_bpc); > - } > + igt_require_f(i915_clock_constraint(data, pipe, output, output_bpc), > + "No supported mode found to use %d-bpc on %s\n", > + output_bpc, output->name); > > /* > * Check the status of Dithering block: > @@ -155,7 +162,7 @@ static void test_dithering(data_t *data, enum pipe pipe, > * If fb_bpc is greater than output_bpc, Dithering should be enabled > * Else disabled > */ > - status = get_dither_state(data); > + status = get_dither_state(data, pipe); > > igt_info("FB BPC:%d, Panel BPC:%d, Pipe BPC:%d, Expected Dither:%s, Actual result:%s\n", > fb_bpc, output_bpc, status.bpc, > @@ -167,17 +174,12 @@ static void test_dithering(data_t *data, enum pipe pipe, > * Otherwise, previously updated value will stay forever and > * may cause the failures for next/other subtests. > */ > - igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, bpc); > + igt_output_set_prop_value(output, IGT_CONNECTOR_MAX_BPC, bpc); > igt_plane_set_fb(data->primary, NULL); > igt_output_set_pipe(output, PIPE_NONE); > igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY); > igt_remove_fb(data->drm_fd, &data->fb); > > - /* Check if crtc bpc is updated with requested one. */ > - igt_require_f((status.bpc == output_bpc), > - "%s can support max %u-bpc, but requested %d-bpc\n", > - output->name, status.bpc, output_bpc); > - > /* Compute the result. */ > if (fb_bpc > output_bpc) > igt_assert_f(status.dither, "(fb_%dbpc > output_%dbpc): Dither should be enabled\n",