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

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

On Mon, Feb 02, 2026 at 01:10:35PM +0200, Jani Nikula wrote:
> On Fri, 30 Jan 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > 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:
> >> > +/**
> >> > + * 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.
> 
> In igt_display_require(), the loop is:
> 
> 	for (i = 0; i < resources->count_crtcs; i++) {
> 		igt_crtc_t *crtc;
> 		int pipe_enum = is_intel_dev ? __intel_get_pipe_from_crtc_index(drm_fd, i) : i;
> 
> 		crtc = igt_crtc_for_pipe(display, pipe_enum);
> 		crtc->pipe = pipe_enum;
> 
> 		crtc->valid = true;
> 		crtc->crtc_id = resources->crtcs[i];
> 		/* offset of a pipe in crtcs list */
> 		crtc->crtc_offset = i;
> 	}
> 
> And igt_crtc_for_pipe() returns &display->crtcs[pipe] i.e. the
> display->crtcs is currently in pipe order, and it could have holes if
> pipes have been fused off.
> 
> Feels like this is so convoluted all over the place that we'd be better
> off resetting everything back to assuming pipe === crtc index, and
> fixing stuff from ground up. :/

Yeah, pretty much.

I guess once the bigger cocci stuff is done we should do something
like:
- leave most crtc->pipe vs. PIPE_<whatever> checks alone as those
  are probably looking for a specific hardware pipe
- most other crtc->pipe usages should just be converted to
  crtc_index instead

That could perhaps be done with cocci as well.

And then we can start cleaning igt_kms itself:
- make display->crtcs[] keep the kernel crtc order and get rid of
  the holes, and have igt_crtc_for_pipe() just search through the
  array for the right crtc
- change all the pending_pipe/valid_crtc_idx_mask/etc. stuff to use
  igt_crtc_t or crtc_index
- etc.

-- 
Ville Syrjälä
Intel