Re: [PATCH i-g-t 08/12] lib/kms: Add igt_crtc_t based CRC wrappers

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

On Wed, Jan 21, 2026 at 11:33:26AM +0200, Jani Nikula wrote:
> On Tue, 20 Jan 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add wrappers for igt_pipe_crc_new*() that take igt_crtc_t rather
> > than the bare crtc index. Should help eliminate the pipe usage
> > from tests.
> 
> I think the basic idea is good, but a couple of comments inline.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  lib/igt_kms.c | 14 ++++++++++++++
> >  lib/igt_kms.h |  4 ++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 74ad82d77dd3..901bbfab5934 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -59,6 +59,7 @@
> >  #include "intel_chipset.h"
> >  #include "igt_debugfs.h"
> >  #include "igt_device.h"
> > +#include "igt_pipe_crc.h"
> >  #include "igt_sysfs.h"
> >  #include "sw_sync.h"
> >  #ifdef HAVE_CHAMELIUM
> > @@ -5299,6 +5300,19 @@ int igt_output_preferred_vrefresh(igt_output_t *output)
> >  		return 60;
> >  }
> >  
> > +igt_pipe_crc_t *igt_crtc_crc_new(igt_crtc_t *crtc, const char *source)
> > +{
> > +	return igt_pipe_crc_new(crtc->display->drm_fd, crtc->crtc_offset,
> > +				source);
> > +}
> > +
> > +igt_pipe_crc_t *igt_crtc_crc_new_nonblock(igt_crtc_t *crtc, const char *source)
> > +{
> > +	return igt_pipe_crc_new_nonblock(crtc->display->drm_fd,
> > +					 crtc->crtc_offset,
> > +					 source);
> > +}
> > +
> >  static const char *igt_crtc_name(igt_crtc_t *crtc)
> >  {
> >  	if (crtc == NULL)
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 71a2f2c4d89c..b291673c2f7f 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -584,6 +584,10 @@ static inline igt_crtc_t *igt_crtc_for_pipe(igt_display_t *display, enum pipe pi
> >  	return &display->pipes[pipe];
> >  }
> >  
> > +typedef struct _igt_pipe_crc igt_pipe_crc_t;
> 
> So this is a dupe typedef, also defined in igt_pipe_crc.h. It works,
> it's probably better than including the other header here, but also a
> bit meh.

Yeah, forward declarations seem be to be painful with typedefs.

> 
> Another idea is naming this based on crtc instead of pipe, even if "crtc
> crc" is a mouthful:
> 
> typedef struct _igt_pipe_crc igt_crtc_crc_t;
> 
> I think it'll work, the types are compatible with the underlying struct
> being the same. So we could (gradually) convert from pipe_crc to
> crtc_crc.

Shrug.

> 
> With the wrappers being in igt_kms.[ch], is the idea that most tests
> could cease to include igt_pipe_crc.h after the conversions?

I expect the tests will need a bunch of other stuff from there
still.

Originally I put the crtc wrappers into igt_pipe_crc.[ch], but
then I needed to add forward declarations and includes for
igt_crtc_t which felt even worse.

-- 
Ville Syrjälä
Intel