Re: [PATCH i-g-t 4/5] tests/kms_multipipe_modeset: use igt_next_crtc()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 06, 2026 at 03:23:57PM +0200, Jani Nikula wrote:
> The test doesn't care about pipes, but rather just another CRTC, so
> prefer igt_next_crtc() over looping pipes and igt_crtc_for_pipe().
> 
> It's all a bit subtle, but previously the test depended on consecutive
> pipes. Using CRTC indices allows for fused off pipes in between.
> 
> While at it, prefer CRTC nomenclature in the variables and messages.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  tests/kms_multipipe_modeset.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/kms_multipipe_modeset.c b/tests/kms_multipipe_modeset.c
> index 1980a0c61116..3c10bb1bd5fe 100644
> --- a/tests/kms_multipipe_modeset.c
> +++ b/tests/kms_multipipe_modeset.c
> @@ -48,17 +48,16 @@ typedef struct {
>  	struct igt_fb fb;
>  } data_t;
>  
> -static void run_test(data_t *data, int valid_outputs)
> +static void run_test(data_t *data)
>  {
>  	igt_output_t *output;
>  	igt_pipe_crc_t *pipe_crcs[IGT_MAX_PIPES] = { 0 };
>  	igt_crc_t ref_crcs[IGT_MAX_PIPES], new_crcs[IGT_MAX_PIPES];
>  	igt_display_t *display = &data->display;
>  	uint16_t width = 0, height = 0;
> -	igt_crtc_t *crtc;
> +	igt_crtc_t *crtc = NULL;
>  	igt_plane_t *plane;
>  	drmModeModeInfo *mode;
> -	int i = 0;
>  
>  	for_each_connected_output(display, output) {
>  		mode = igt_output_get_mode(output);
> @@ -75,13 +74,12 @@ static void run_test(data_t *data, int valid_outputs)
>  
>  	/* Collect reference CRC by Committing individually on all outputs*/
>  	for_each_connected_output(display, output) {
> -		crtc = igt_crtc_for_pipe(display, i);
> +		crtc = igt_next_crtc(display, crtc);
>  		plane = igt_crtc_get_plane_type(crtc, DRM_PLANE_TYPE_PRIMARY);
>  
>  		mode = NULL;
>  
> -		pipe_crcs[i] = igt_crtc_crc_new(crtc,
> -						IGT_PIPE_CRC_SOURCE_AUTO);
> +		pipe_crcs[crtc->crtc_index] = igt_crtc_crc_new(crtc, IGT_PIPE_CRC_SOURCE_AUTO);
>  
>  		igt_output_set_crtc(output,
>  				    crtc);
> @@ -93,15 +91,14 @@ static void run_test(data_t *data, int valid_outputs)
>  		igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>  
>  		igt_display_commit2(display, COMMIT_ATOMIC);
> -		igt_pipe_crc_collect_crc(pipe_crcs[i], &ref_crcs[i]);
> +		igt_pipe_crc_collect_crc(pipe_crcs[crtc->crtc_index], &ref_crcs[crtc->crtc_index]);
>  		igt_output_set_crtc(output, NULL);
> -		i++;
>  	}
>  
> -	i = 0;
>  	/* Simultaneously commit on all outputs */
> +	crtc = NULL;
>  	for_each_connected_output(display, output) {
> -		crtc = igt_crtc_for_pipe(display, i);
> +		crtc = igt_next_crtc(display, crtc);
>  		plane = igt_crtc_get_plane_type(crtc, DRM_PLANE_TYPE_PRIMARY);
>  
>  		mode = NULL;
> @@ -114,22 +111,21 @@ static void run_test(data_t *data, int valid_outputs)
>  		igt_plane_set_fb(plane, &data->fb);
>  		igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
>  		igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> -		i++;
>  	}
>  
>  	igt_display_commit2(display, COMMIT_ATOMIC);
>  
>  	/* CRC Verification */
> -	for (i = 0; i < valid_outputs; i++) {
> -		igt_pipe_crc_collect_crc(pipe_crcs[i], &new_crcs[i]);
> -		igt_assert_crc_equal(&ref_crcs[i], &new_crcs[i]);
> +	for_each_crtc(display, crtc) {
> +		igt_pipe_crc_collect_crc(pipe_crcs[crtc->crtc_index], &new_crcs[crtc->crtc_index]);
> +		igt_assert_crc_equal(&ref_crcs[crtc->crtc_index], &new_crcs[crtc->crtc_index]);

We may not have used up all the CRTCs since we only picked one for
each connected output. So this would need some kind of pipe_crcs[...]==NULL
check perhaps.

>  	}
>  
>  	igt_plane_set_fb(plane, NULL);
>  	igt_remove_fb(data->drm_fd, &data->fb);
>  }
>  
> -static void test_multipipe(data_t *data, int num_pipes)
> +static void test_multipipe(data_t *data, int num_crtcs)
>  {
>  	igt_output_t *output;
>  	int valid_outputs = 0;
> @@ -137,11 +133,11 @@ static void test_multipipe(data_t *data, int num_pipes)
>  	for_each_connected_output(&data->display, output)
>  		valid_outputs++;
>  
> -	igt_require_f(valid_outputs == num_pipes,
> +	igt_require_f(valid_outputs == num_crtcs,
>  		      "Number of connected outputs(%d) not equal to the "
> -		      "number of pipes supported(%d)\n", valid_outputs, num_pipes);
> +		      "number of CRTCs supported(%d)\n", valid_outputs, num_crtcs);

Oh, apparently we do check that we have the same number of outputs and
CRTCs. That makes no sense to me. Apparently the original intention was
to make sure all CRTCs can be lit up simultaneously. For that we should
only care about having at least as many outputs as CRTCs.

Or more correctly we should make sure we have valid output for each
CRTC, which given potential CRTC<->output routing constraints isn't
even the same thing as just comparing the numbers. So it should probably
use something like for_each_crtc_with_single_output(), and then make
sure it actually found a valid output for every CRTC.

But given the already poor state of the test, this patch doesn't 
seem to make it any worse so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I guess the other thing to consider is whether this test makes
any sense at all. I'm not sure what specifically lighting up all
the CRTCs is supposed to achieve vs. just lighting up some of them...

>  
> -	run_test(data, valid_outputs);
> +	run_test(data);
>  }
>  
>  int igt_main()
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel