Re: [PATCH i-g-t 3/3] lib/kms: use CRTC index for __igt_output_crtc_populate()

Ville Syrjälä
2026-03-26T11:36:28Z

On Mon, Mar 23, 2026 at 02:06:26PM +0200, Jani Nikula wrote:
> Fill the valid output/crtc combos in __igt_output_crtc_populate() using
> CRTC index instead of pipe. This avoids buffer overruns if we reduce
> igt_display_n_crtcs() to the actual number of CRTCs on the device.
>
> To keep the for_each_crtc_with_single_output() iteration in pipe order
> on Intel devices, sort the resulting array in a separate
> step. Presumably the performance impact of qsort() on an array of at
> most 16 elements is neglible.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> lib/igt_kms.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 42329ad0303c..b92bb16aaf9c 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3588,6 +3588,18 @@ bool output_is_internal_panel(igt_output_t *output)
> }
> }
>
> +static int output_crtc_pipe_compare(const void *_a, const void *_b)
> +{
> + const igt_output_crtc_t *a = _a, *b = _b;
> +
> + /* pipe order for valid output/crtc combos */
> + if (a->crtc && b->crtc)
> + return a->crtc->pipe - b->crtc->pipe;
> +
> + /* valid combos before empty elements */
> + return !b->crtc - !a->crtc;
> +}
> +
> igt_output_crtc_t *__igt_output_crtc_populate(igt_display_t *display, igt_output_crtc_t *chosen_outputs)
> {
> unsigned int full_crtc_index_mask = 0, assigned_crtc_index_mask = 0;
> @@ -3634,17 +3646,17 @@ igt_output_crtc_t *__igt_output_crtc_populate(igt_display_t *display, igt_output
> /* We found an unassigned CRTC, use it! */
> found = true;
> assigned_crtc_index_mask |= 1 << crtc->crtc_index;
> - chosen_outputs[crtc->pipe].output = output;
> - chosen_outputs[crtc->pipe].crtc = crtc;
> - } else if (!chosen_outputs[crtc->pipe].output ||
> - output_is_internal_panel(chosen_outputs[crtc->pipe].output)) {
> + chosen_outputs[crtc->crtc_index].output = output;
> + chosen_outputs[crtc->crtc_index].crtc = crtc;
> + } else if (!chosen_outputs[crtc->crtc_index].output ||
> + output_is_internal_panel(chosen_outputs[crtc->crtc_index].output)) {
> /*
> * Overwrite internal panel if not
> * assigned, external outputs are faster
> * to do modesets
> */
> - chosen_outputs[crtc->pipe].output = output;
> - chosen_outputs[crtc->pipe].crtc = crtc;
> + chosen_outputs[crtc->crtc_index].output = output;
> + chosen_outputs[crtc->crtc_index].crtc = crtc;
> }
> }
>
> @@ -3654,6 +3666,11 @@ igt_output_crtc_t *__igt_output_crtc_populate(igt_display_t *display, igt_output
> }
> }
>
> + /* For Intel, sort the output/crtc combos in pipe order */
> + if (is_intel_device(display->drm_fd))
> + qsort(chosen_outputs, igt_display_n_crtcs(display),
> + sizeof(*chosen_outputs), output_crtc_pipe_compare);

Maybe one day we can drop this sorting entirely. But that needs some
testing, so this seems good for the moment.

Didn't spot anything bad, so for the series
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> return chosen_outputs;
> }
>
> --
> 2.47.3

--
Ville Syrjälä
Intel