[PATCH i-g-t] tests/intel/kms_cdclk: Handle scenario where ref and new cdclk is same

[Nautiyal, Ankit K]

On 11/21/2024 11:16 PM, Swati Sharma wrote:
> There are couple of issues with the mode-transition-all-outputs
> subtest. Firstly, test should execute only if we have minimun
> of 2 connected outputs. Secondly, if hdisplay and vdisplay of
> highest and lowest modes are same for an output, don't consider
> that output. Both these issues are addressed here.
>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1887
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
>   tests/intel/kms_cdclk.c | 67 +++++++++++++++++++++++++----------------
>   1 file changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/tests/intel/kms_cdclk.c b/tests/intel/kms_cdclk.c
> index 382b3e9d1..61d73c8c6 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 DISPLAY_COUNT	2
>   
>   /* Test flags */
>   enum {
> @@ -274,83 +275,97 @@ 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;
> +	igt_output_t *valid_outputs[IGT_MAX_PIPES];
>   	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;
> +	int i = 0, j = 0, k = 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) {
> +	for_each_connected_output(&data->display, output) {
>   		mode = igt_output_get_mode(output);
>   		igt_assert(mode);
>   
> +		mode_hi = get_highres_mode(output);
> +		igt_require(mode_hi != NULL);
> +
> +		mode_lo = get_lowres_mode(output);
> +
> +		if (mode_hi->hdisplay == mode_lo->hdisplay && mode_hi->vdisplay == mode_lo->vdisplay) {
> +		      igt_debug("Highest and lowest mode resolutions are same; no transition\n");
> +		      continue;
> +		}
> +
> +		valid_outputs[count++] = output;

I think we can store mode_hi and mode_low for valid_outputs, in an array 
to avoid calling get_low/high_res_mode again.


> +	}
> +
> +	igt_skip_on_f(count < DISPLAY_COUNT,
> +		      "Valid outputs (%d) should be equal or greater than %d\n", count, DISPLAY_COUNT);
> +
> +	for (i = 0; i < count; i++) {
> +		mode = igt_output_get_mode(valid_outputs[i]);
> +		igt_assert(mode);
> +
>   		width = max(width, mode->hdisplay);
>   		height = max(height, mode->vdisplay);
>   
> -		mode_hi = get_highres_mode(output);
> +		mode_hi = get_highres_mode(valid_outputs[i]);
>   		igt_require(mode_hi != NULL);
>   
> -		igt_output_set_pipe(output, i);
> -		igt_output_override_mode(output, mode_hi);
> -		i++;
> +		igt_output_set_pipe(valid_outputs[i], i);
> +		igt_output_override_mode(valid_outputs[i], mode_hi);
>   	}
> +
>   	igt_require(intel_pipe_output_combo_valid(display));
>   	igt_display_reset(display);
>   
>   	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];
> +
> +	for (k = 0; k < count; k++) {
> +		pipe = &display->pipes[k];
>   		plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
>   		mode = NULL;
>   
> -		igt_output_set_pipe(output, i);
> -		mode = igt_output_get_mode(output);
> +		igt_output_set_pipe(valid_outputs[k], k);
> +		mode = igt_output_get_mode(valid_outputs[k]);

We dont seem to be using this.


>   		igt_assert(mode);
>   
> -		mode_lo = get_lowres_mode(output);
> +		mode_lo = get_lowres_mode(valid_outputs[k]);
>   
> -		igt_output_override_mode(output, mode_lo);
> +		igt_output_override_mode(valid_outputs[k], 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++;

This whole thing is repeated, perhaps this can be separated into a 
function and then called for mode_hi and mode_lo.

I understand this was already there before this patch, so I guess it 
will be better to just optimize the existing thing in one patch and 
tackle the issues in a separate patch.

Regards,

Ankit

>   	}
>   
>   	igt_display_commit2(display, COMMIT_ATOMIC);
>   	cdclk_ref = get_current_cdclk_freq(debugfs_fd);
>   
> -	j = 0;
> -	for_each_connected_output(display, output) {
> +	for (j = 0; j < count; j++) {
>   		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_output_set_pipe(valid_outputs[j], j);
> +		mode = igt_output_get_mode(valid_outputs[j]);
>   		igt_assert(mode);
>   
> -		mode_hi = get_highres_mode(output);
> +		mode_hi = get_highres_mode(valid_outputs[j]);
>   		igt_require(mode_hi != NULL);
>   
> -		igt_output_override_mode(output, mode_hi);
> +		igt_output_override_mode(valid_outputs[j], 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++;
>   	}
>   
>   	igt_display_commit2(display, COMMIT_ATOMIC);