Re: [PATCH i-g-t 01/15] lib/kms: Introduce for_each_crtc*()

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

On Fri, Jan 30, 2026 at 03:38:03PM +0200, Jani Nikula wrote:
> On Fri, 30 Jan 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add for_each_crtc*() as the crtc based replacement for
> > the for_each_pipe*() iterators (which will eventually get
> > nuked).
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  lib/igt_kms.h | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 432c67d11780..a4857ea0e574 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -682,6 +682,19 @@ static inline bool igt_pipe_connector_valid(enum pipe pipe, igt_output_t *output
> >  		output->config.valid_crtc_idx_mask & (1 << (pipe));
> >  }
> >  
> > +/**
> > + * igt_crtc_connector_valid:
> > + * @crtc: CRTC to check.
> > + * @output: #igt_output_t to check.
> > + *
> > + * Checks whether the given CRTC and output can be used together.
> > + */
> > +static inline bool igt_crtc_connector_valid(igt_crtc_t *crtc, igt_output_t *output)
> > +{
> > +	return igt_output_is_connected(output) &&
> > +		output->config.valid_crtc_idx_mask & (1 << crtc->pipe);
> 
> So should this use crtc->crtc_offset instead of crtc->pipe? Or will that
> bring complications during the conversions that try to remain
> functionally unchanged?

I wasn't thinking too had at this point. Just trying to mostly preserve
the behaviour until we can get to the point where we only have to
modify igt_kms.[ch] to change things.

> 
> (And should crtc->crtc_offset really be named crtc_index?)

Yeah, I think so. But not worrying about it right now.

> 
> > +}
> > +
> >  #define for_each_if(condition) if (!(condition)) {} else
> >  
> >  /**
> > @@ -744,6 +757,23 @@ static inline bool igt_pipe_connector_valid(enum pipe pipe, igt_output_t *output
> >  	for_each_pipe_static(pipe) \
> >  		for_each_if(igt_crtc_for_pipe((display), (pipe))->valid)
> >  
> > +/**
> > + * for_each_crtc:
> > + * @display: a pointer to an #igt_display_t structure
> > + * @crtc: The CRTC
> > + *
> > + * This for loop iterates over all CRTCs.
> > + *
> > + * Note that this cannot be used to enumerate per-CRTC subtest names since it
> > + * depends upon runtime probing of the actual kms driver that is being tested.
> > + * Use #for_each_pipe_static instead.
> > + */
> > +#define for_each_crtc(display, crtc) \
> > +	for ((crtc) = &(display)->crtcs[0]; \
> > +	     (crtc) < &(display)->crtcs[(display)->n_crtcs]; \
> > +	     (crtc)++) \
> > +		for_each_if ((crtc)->valid)
> 
> I feel like for_each_crtc() should iterate the CRTCs in CRTC order, not
> pipe order. What's the plan? Are we eventually going to have ->crtcs[]
> in CRTC order?

I don't even know what it means to have them in pipe order. It should be
one and the same right now because the kernel anyway has them in the
same order. I guess it just means we could theoeretically have holes
in the array with crtc->valid==false for fused off pipes?

But yeah, if/when we start to reorder the pipes, I think we should keep
this stuff in CRTC order, and get rid of the holes.

> And then do something special when some tests want to use
> pipe order?

Something like that. Or make sure the tests don't have some
silly assumptions where the order matters.

> The other day I was looking at making ->n_crtcs and thus
> igt_display_n_crtcs() reflect the actual CRTC count, not IGT_MAX_PIPES,
> and it's also complicated because too many places depend on
> igt_display_n_crtcs() indicating the size of ->crtcs[] and that being
> indexed by pipe instead of CRTC index. So that's also dependent on
> indexing ->crtcs[] with CRTC index.

Yeah, it all needs to get fixed properly to get rid of the pipe
indexing/holes. And then we can nuke that silly crtc->valid thing
as well. Though I admit I didn't really look to closely at what
it all entails.

> Anyway, the loops here make my head spin, but it's all

Yeah, they ugly. for_each_crtc_with_single_output() in particular since
it still shares __igt_pipe_populate_outputs() with the pipe based thing
but I didn't want to start rewriting that stuff until the old iterator
is gone.

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> I'd just like to hear what the plan is wrt the above questions, perhaps
> recorded in the commit message.

I suppose I don't really have a real solid plane yet, other than
to try to limit the damage to lib/igt_kms.[ch] when we need to
do those changes.

> 
> BR,
> Jani.
> 
> 
> > +
> >  /**
> >   * for_each_pipe_with_valid_output:
> >   * @display: a pointer to an #igt_display_t structure
> > @@ -765,6 +795,27 @@ static inline bool igt_pipe_connector_valid(enum pipe pipe, igt_output_t *output
> >  			for_each_if ((((output) = &(display)->outputs[con__]), \
> >  						igt_pipe_connector_valid((pipe), (output))))
> >  
> > +/**
> > + * for_each_crtc_with_valid_output:
> > + * @display: a pointer to an #igt_display_t structure
> > + * @crtc: CRTC for which this @crtc / @output combination is valid.
> > + * @output: The output for which this @crtc / @output combination is valid.
> > + *
> > + * This for loop is called over all connected outputs. This function
> > + * will try every combination of @crtc and @output.
> > + *
> > + * If you only need to test a single output for each pipe, use
> > + * for_each_crtc_with_single_output(), if you only need an
> > + * output for a single CRTC, use igt_get_single_output_for_pipe().
> > + */
> > +#define for_each_crtc_with_valid_output(display, crtc, output) \
> > +	for ((output) = &(display)->outputs[0], (crtc) = &(display)->crtcs[0]; \
> > +	     assert(igt_can_fail()), (crtc) < &(display)->crtcs[(display)->n_crtcs] && \
> > +		     (output) < &(display)->outputs[(display)->n_outputs]; \
> > +	     (output) = (output) + 1 < &(display)->outputs[(display)->n_outputs] ? \
> > +		     (output) + 1 : ((crtc)++, &(display)->outputs[0])) \
> > +		for_each_if ((crtc)->valid && igt_crtc_connector_valid((crtc), (output)))
> > +
> >  igt_output_t **__igt_pipe_populate_outputs(igt_display_t *display,
> >  					   igt_output_t **chosen_outputs);
> >  
> > @@ -785,6 +836,23 @@ igt_output_t **__igt_pipe_populate_outputs(igt_display_t *display,
> >  		for_each_if (*__output && \
> >  			     ((pipe) = (__output - __outputs), (output) = *__output, 1))
> >  
> > +/**
> > + * for_each_crtc_with_single_output:
> > + * @display: a pointer to an #igt_display_t structure
> > + * @crtc: The CRTC for which this @crtc / @output combination is valid.
> > + * @output: The output for which this @crtc / @output combination is valid.
> > + *
> > + * This loop is called over all CRTCs, and will try to find a compatible output
> > + * for each CRTC. Unlike for_each_pipe_with_valid_output(), this function will
> > + * be called at most once for each pipe.
> > + */
> > +#define for_each_crtc_with_single_output(display, crtc, output) \
> > +	for (igt_output_t *__outputs[igt_display_n_crtcs(display)], \
> > +	     **__output = __igt_pipe_populate_outputs((display), __outputs); \
> > +		 __output < &__outputs[igt_display_n_crtcs(display)]; __output++) \
> > +		for_each_if (*__output && \
> > +			     ((crtc) = igt_crtc_for_pipe((display), (__output - __outputs)), (output) = *__output, 1))
> > +
> >  /**
> >   * for_each_valid_output_on_pipe:
> >   * @display: a pointer to an #igt_display_t structure
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel