Re: [PATCH i-g-t 3/6] tests/intel/kms_cdclk: Add conditions to filter valid outputs

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

On 12/31/2024 12:33 AM, Swati Sharma wrote:
> Conditions are added to filter valid outputs. Also, highres and
> lowres of valid outputs are stored in an array to be re-used
> later.

Imperative mood.

Also mention why the condition are added.

Perhaps to check if both low res and high res are same there would be no 
cdclk transition.

>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>   tests/intel/kms_cdclk.c | 52 +++++++++++++++++++++++++++++------------
>   1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/tests/intel/kms_cdclk.c b/tests/intel/kms_cdclk.c
> index e78d22e1d..dfc64ae2e 100644
> --- a/tests/intel/kms_cdclk.c
> +++ b/tests/intel/kms_cdclk.c
> @@ -261,8 +261,10 @@ 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_highres[IGT_MAX_PIPES] = {0}, mode_lowres[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;
> @@ -273,31 +275,51 @@ static void test_mode_transition_on_all_outputs(data_t *data)
>   	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);
> +		drmModeModeInfo highres_mode;
> +		drmModeModeInfo lowres_mode;
> +
> +		highres_mode = *igt_output_get_highres_mode(output);
> +		igt_require_f(highres_mode.hdisplay >= HDISPLAY_4K && highres_mode.vdisplay >= VDISPLAY_4K &&
> +			      highres_mode.vrefresh >= VREFRESH, "Mode >= 4K not found on output %s.\n",
> +			      igt_output_name(output));
> +
> +		lowres_mode = *get_lowres_mode(output);
> +
> +		if (highres_mode.hdisplay == lowres_mode.hdisplay &&
> +		    highres_mode.vdisplay == lowres_mode.vdisplay) {
> +			igt_info("Highest and lowest mode resolutions are same on output %s; no transition will occur, skipping\n",
> +				  igt_output_name(output));
> +			continue;
> +		}
> +
> +		valid_outputs[count++] = output;
> +	}
> +
> +	igt_skip_on_f(count < 2,
> +		      "Number of valid outputs (%d) must be greater than or equal to 2\n", count);
> +
> +	for (int k = 0; k < count; k++) {

Do we need two loops?

At least highres mode and lowres mode can be stored in the previous loop 
itself.

> +		mode_highres[k] = *igt_output_get_highres_mode(valid_outputs[k]);
> +		mode_lowres[k] = *get_lowres_mode(valid_outputs[k]);
> +
> +		mode = igt_output_get_mode(valid_outputs[k]);
>   		igt_assert(mode);
>   
> +		// Update the maximum width and height for the framebuffer

I dont think we need these comments.


>   		width = max(width, mode->hdisplay);
>   		height = max(height, mode->vdisplay);
>   
> -		mode_hi = *igt_output_get_highres_mode(output);
> -		igt_require_f(mode_hi.hdisplay >= HDISPLAY_4K && mode_hi.vdisplay >= VDISPLAY_4K &&
> -			      mode_hi.vrefresh >= VREFRESH, "Mode >= 4K not found on output %s.\n",
> -			      igt_output_name(output));
> -
> -		igt_output_set_pipe(output, i);
> -		igt_output_override_mode(output, &mode_hi);
> -		i++;
> +		igt_output_set_pipe(valid_outputs[k], k);
> +		igt_output_override_mode(valid_outputs[k], &mode_highres[k]);
>   	}
> +
>   	igt_require(intel_pipe_output_combo_valid(display));
> -	igt_display_reset(display);
>   
> +	// Create a framebuffer with the maximum dimensions calculated earlier

Ditto.

Regards,

Ankit

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