Re: [PATCH i-g-t] tests/intel/kms_cdclk: Refactor mode setting and CD clock verification

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/13/2024 11:32 AM, Swati Sharma wrote:
> 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.

This patch is doing more things along with refactoring like skipping if 
there is only one mode, and checking if there are minimum 2 displays.

So lets refactor be one patch, with no functional change.

Then add other enhancements etc.


>
> Signed-off-by: Swati Sharma <swati2.sharma@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);
>   }
>   
> +
> +static void set_mode(data_t *data, drmModeModeInfo *mode, int count,
> +		     struct igt_fb fb, igt_output_t **valid_outputs)

let count be after data, as this is count of modes and valid outputs. 
Perhaps a better order will be:

set_mode(data_t *data, int count, drmModeModeInfo *mode, igt_output_t **valid_outputs, struct igt_fb fb)

> +{
> +	igt_display_t *display = &data->display;
> +	igt_pipe_t *pipe;
> +	igt_plane_t *plane;
> +
> +	for (int k = 0; k < count; k++) {

lets use i for iterator.


> +		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;

I think it will be better to set the mode_hi and mode_lo later after we 
set valid_output.

If both are equal we are overwriting mode_hi and mode_lo.


>   
> -		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) {
In line with above comment, use highres_mode and lowres_mode to compare.
> +			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 "
> +		      "minimum required displays (%d)\n", count, MIN_DISPLAYS);

I think what we mean here is that the count should be more than 1, 
meaning there should be more than 1 output.

We dont need to have a macro MIN_DISPLAYS as count >= 1 is self explanatory.

Regards,

Ankit

>   
> -		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);
>   	do_cleanup_display(display);
>   	igt_remove_fb(data->drm_fd, &fb);
>   }