[PATCH i-g-t] tests/intel/kms_cdclk: Refactor mode setting and CD clock verification
Samala, Pranay
pranay.samala at intel.com
Mon Dec 16 08:49:26 UTC 2024
Hi Swati,
> -----Original Message-----
> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Swati
> Sharma
> Sent: Friday, December 13, 2024 11:32 AM
> To: igt-dev at lists.freedesktop.org
> Cc: Sharma, Swati2 <swati2.sharma at intel.com>
> Subject: [PATCH i-g-t] tests/intel/kms_cdclk: Refactor mode setting and CD clock
> verification
>
> Add conditions to filter valid outputs. Encapsulate the mode-setting functionality
> in the set_mode function to reduce redundancy. Remove redundant framebuffer
> and size setting operations for the plane. Simplify the CD clock frequency checks
> to ensure it increases after the mode transition. Clean up unnecessary plane
> operations and redundant framebuffer handling. Enhance the readability and
> modularity of the mode transition logic to handle multiple outputs more
> effectively.
>
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
> tests/intel/kms_cdclk.c | 112 +++++++++++++++++++---------------------
> 1 file changed, 54 insertions(+), 58 deletions(-)
>
> diff --git a/tests/intel/kms_cdclk.c b/tests/intel/kms_cdclk.c index
> 382b3e9d1..93276f45e 100644
> --- a/tests/intel/kms_cdclk.c
> +++ b/tests/intel/kms_cdclk.c
> @@ -54,6 +54,7 @@ IGT_TEST_DESCRIPTION("Test cdclk features : crawling and
> squashing");
> #define VDISPLAY_4K 2160
> #define VREFRESH 60
> #define MAX_CDCLK_4K 307200
> +#define MIN_DISPLAYS 2
>
> /* Test flags */
> enum {
> @@ -269,90 +270,86 @@ static void test_mode_transition(data_t *data, enum
> pipe pipe, igt_output_t *out
> igt_remove_fb(display->drm_fd, &fb);
> }
>
> +
Please avoid this blank line
> +static void set_mode(data_t *data, drmModeModeInfo *mode, int count,
> + struct igt_fb fb, igt_output_t **valid_outputs) {
> + igt_display_t *display = &data->display;
> + igt_pipe_t *pipe;
> + igt_plane_t *plane;
> +
> + for (int k = 0; k < count; k++) {
> + pipe = &display->pipes[k];
> + plane = igt_pipe_get_plane_type(pipe,
> DRM_PLANE_TYPE_PRIMARY);
> +
> + igt_output_override_mode(valid_outputs[k], &mode[k]);
> +
> + igt_plane_set_fb(plane, &fb);
> + igt_fb_set_size(&fb, plane, mode[k].hdisplay, mode[k].vdisplay);
> + igt_plane_set_size(plane, mode[k].hdisplay, mode[k].vdisplay);
> + }
> +}
> +
> static void test_mode_transition_on_all_outputs(data_t *data) {
> igt_display_t *display = &data->display;
> int debugfs_fd = data->debugfs_fd;
> - drmModeModeInfo *mode, *mode_hi, *mode_lo;
> + drmModeModeInfo *mode, mode_hi[IGT_MAX_PIPES] = {0},
> mode_lo[IGT_MAX_PIPES] = {0};
> + igt_output_t *valid_outputs[IGT_MAX_PIPES] = {NULL};
> igt_output_t *output;
> - int valid_outputs = 0;
> + int count = 0;
> int cdclk_ref, cdclk_new;
> uint16_t width = 0, height = 0;
> struct igt_fb fb;
> - igt_pipe_t *pipe;
> - igt_plane_t *plane;
> - int i = 0, j = 0;
>
> do_cleanup_display(display);
> igt_display_reset(display);
>
> - for_each_connected_output(&data->display, output)
> - valid_outputs++;
> -
> - i = 0;
> for_each_connected_output(display, output) {
> - mode = igt_output_get_mode(output);
> - igt_assert(mode);
> + const drmModeModeInfo *highres_mode;
> + const drmModeModeInfo *lowres_mode;
>
> - width = max(width, mode->hdisplay);
> - height = max(height, mode->vdisplay);
> + highres_mode = get_highres_mode(output);
> + igt_require(highres_mode != NULL);
> + mode_hi[count] = *highres_mode;
>
> - mode_hi = get_highres_mode(output);
> - igt_require(mode_hi != NULL);
> + lowres_mode = get_lowres_mode(output);
> + mode_lo[count] = *lowres_mode;
>
> - igt_output_set_pipe(output, i);
> - igt_output_override_mode(output, mode_hi);
> - i++;
> - }
> - igt_require(intel_pipe_output_combo_valid(display));
> - igt_display_reset(display);
> + if (mode_hi[count].hdisplay == mode_lo[count].hdisplay &&
> + mode_hi[count].vdisplay == mode_lo[count].vdisplay) {
> + igt_debug("Highest and lowest mode resolutions are
> same; no transition\n");
> + continue;
> + }
>
> - igt_create_pattern_fb(data->drm_fd, width, height,
> DRM_FORMAT_XRGB8888,
> - DRM_FORMAT_MOD_LINEAR, &fb);
> - i = 0;
> - for_each_connected_output(display, output) {
> - pipe = &display->pipes[i];
> - plane = igt_pipe_get_plane_type(pipe,
> DRM_PLANE_TYPE_PRIMARY);
> + valid_outputs[count++] = output;
> + }
>
> - mode = NULL;
> + igt_skip_on_f(count < MIN_DISPLAYS,
> + "Number of valid outputs (%d) must be greater than or equal
> to the "
Please add backslash (\) at the end of this line to ensure that the string is
concatenated correctly.
> + "minimum required displays (%d)\n", count, MIN_DISPLAYS);
>
> - igt_output_set_pipe(output, i);
> - mode = igt_output_get_mode(output);
> + for (int i = 0; i < count; i++) {
> + mode = igt_output_get_mode(valid_outputs[i]);
> igt_assert(mode);
>
> - mode_lo = get_lowres_mode(output);
> + width = max(width, mode->hdisplay);
> + height = max(height, mode->vdisplay);
>
> - igt_output_override_mode(output, mode_lo);
> - igt_plane_set_fb(plane, &fb);
> - igt_fb_set_size(&fb, plane, mode_lo->hdisplay, mode_lo-
> >vdisplay);
> - igt_plane_set_size(plane, mode_lo->hdisplay, mode_lo-
> >vdisplay);
> - i++;
> + igt_output_set_pipe(valid_outputs[i], i);
> + igt_output_override_mode(valid_outputs[i], &mode_hi[i]);
> }
>
> - igt_display_commit2(display, COMMIT_ATOMIC);
> - cdclk_ref = get_current_cdclk_freq(debugfs_fd);
> -
> - j = 0;
> - for_each_connected_output(display, output) {
> - pipe = &display->pipes[j];
> - plane = igt_pipe_get_plane_type(pipe,
> DRM_PLANE_TYPE_PRIMARY);
> -
> - mode = NULL;
> -
> - igt_output_set_pipe(output, j);
> - mode = igt_output_get_mode(output);
> - igt_assert(mode);
> + igt_require(intel_pipe_output_combo_valid(display));
>
> - mode_hi = get_highres_mode(output);
> - igt_require(mode_hi != NULL);
> + igt_create_pattern_fb(data->drm_fd, width, height,
> DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_MOD_LINEAR, &fb);
>
> - igt_output_override_mode(output, mode_hi);
> - igt_plane_set_fb(plane, &fb);
> - igt_fb_set_size(&fb, plane, mode_hi->hdisplay, mode_hi-
> >vdisplay);
> - igt_plane_set_size(plane, mode_hi->hdisplay, mode_hi-
> >vdisplay);
> - j++;
> - }
> + set_mode(data, mode_lo, count, fb, valid_outputs);
> + igt_display_commit2(display, COMMIT_ATOMIC);
> + cdclk_ref = get_current_cdclk_freq(debugfs_fd);
>
> + set_mode(data, mode_hi, count, fb, valid_outputs);
> igt_display_commit2(display, COMMIT_ATOMIC);
> cdclk_new = get_current_cdclk_freq(debugfs_fd);
> igt_info("CD clock frequency %d -> %d\n", cdclk_ref, cdclk_new); @@ -
> 360,7 +357,6 @@ static void test_mode_transition_on_all_outputs(data_t *data)
> /* cdclk should bump */
> igt_assert_lt(cdclk_ref, cdclk_new);
>
> - igt_plane_set_fb(plane, NULL);
With these changes fixed, code LGTM.
Regards,
Pranay Samala
> do_cleanup_display(display);
> igt_remove_fb(data->drm_fd, &fb);
> }
> --
> 2.25.1
More information about the igt-dev
mailing list