Re: [PATCH i-g-t 2/5] tests/nouveau_crc: Convert to dynamic subtests

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

On Fri, Mar 06, 2026 at 01:13:52PM +0200, Jani Nikula wrote:
> On Thu, 05 Mar 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > nouveau_crc is the last holdout for for_each_pipe_static(). Convert
> > the tests to use dynamic per-pipe subtests so that we can nuke
> > for_each_pipe_static() and convert everything to use igt_crtc_t.
> 
> Looks legit, although the diff is a bit hard to follow.

Aye. I forgot to note that 'git show --ignore-all-space' makes it
actually legible. But to do that one actually has to apply the
patch :/

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > Untested due to lack of hardware.
> >
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  lib/igt_kms.h       |  12 -----
> >  tests/nouveau_crc.c | 113 ++++++++++++++++++++++++++------------------
> >  2 files changed, 68 insertions(+), 57 deletions(-)
> >
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index e91c6567757e..9514ef03f0e8 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -720,18 +720,6 @@ static inline bool igt_crtc_connector_valid(igt_crtc_t *crtc, igt_output_t *outp
> >  	for_each_output((display), (output))	\
> >  		for_each_if ((!(igt_output_is_connected((output)))))
> >  
> > -/**
> > - * for_each_pipe_static:
> > - * @pipe: The pipe to iterate.
> > - *
> > - * This for loop iterates over all pipes supported by IGT libraries.
> > - *
> > - * This should be used to enumerate per-pipe subtests since it has no runtime
> > - * depencies.
> > - */
> > -#define for_each_pipe_static(pipe) \
> > -	for (pipe = 0; pipe < IGT_MAX_PIPES; pipe++)
> > -
> >  /**
> >   * for_each_crtc:
> >   * @display: a pointer to an #igt_display_t structure
> > diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> > index 393414339ab0..59fa8a2daeb8 100644
> > --- a/tests/nouveau_crc.c
> > +++ b/tests/nouveau_crc.c
> > @@ -297,6 +297,16 @@ static void test_source(data_t *data, const char *source)
> >  	free(crcs);
> >  }
> >  
> > +static void test_source_outp_complete(data_t *data)
> > +{
> > +	test_source(data, "outp-complete");
> > +}
> > +
> > +static void test_source_rg(data_t *data)
> > +{
> > +	test_source(data, "rg");
> > +}
> > +
> >  static void test_source_outp_inactive(data_t *data)
> >  {
> >  	struct color_fb colors[] = {
> > @@ -365,13 +375,17 @@ static void cleanup_test(data_t *data)
> >  	close(data->nv_crc_dir);
> >  }
> >  
> > +static void run_test(data_t *data, void (*test)(data_t *data))
> > +{
> > +	setup_test(data);
> > +	test(data);
> > +	cleanup_test(data);
> > +}
> > +
> >  data_t data = {0};
> >  
> > -#define pipe_test(name) igt_subtest_f("pipe-%s-" name, kmstest_pipe_name(pipe))
> >  int igt_main()
> >  {
> > -	int pipe;
> > -
> >  	igt_fixture() {
> >  		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> >  		igt_require_nouveau(data.drm_fd);
> > @@ -383,50 +397,59 @@ int igt_main()
> >  		igt_display_reset(&data.display);
> >  	}
> >  
> > -	for_each_pipe_static(pipe) {
> > -		igt_fixture() {
> > -			data.crtc = igt_crtc_for_pipe(&data.display, pipe);
> > -			igt_require(data.crtc->valid);
> > -
> > -			setup_test(&data);
> > +	/* We don't need to test this on every pipe, but the setup is the same */
> > +	igt_describe("Make sure that the CRC notifier context flip threshold "
> > +		     "is reset to its default value after a single capture.");
> > +	igt_subtest("ctx-flip-threshold-reset-after-capture") {
> > +		data.crtc = igt_crtc_for_pipe(&data.display, PIPE_A);
> > +		igt_require(data.crtc->valid);
> > +
> > +		run_test(&data, test_ctx_flip_threshold_reset_after_capture);
> > +	}
> > +
> > +	igt_describe("Make sure the association between each CRC and its "
> > +		     "respective frame index is not broken when the driver "
> > +		     "performs a notifier context flip.");
> > +	igt_subtest("ctx-flip-detection") {
> > +		for_each_crtc(&data.display, data.crtc) {
> > +			igt_dynamic_f("pipe-%s", igt_crtc_name(data.crtc))
> > +				run_test(&data, test_ctx_flip_detection);
> >  		}
> > -
> > -		/* We don't need to test this on every pipe, but the setup is the same */
> > -		if (pipe == PIPE_A) {
> > -			igt_describe("Make sure that the CRC notifier context flip threshold "
> > -				     "is reset to its default value after a single capture.");
> > -			igt_subtest("ctx-flip-threshold-reset-after-capture")
> > -				test_ctx_flip_threshold_reset_after_capture(&data);
> > +	}
> > +
> > +	igt_describe("Make sure that igt_pipe_crc_get_current() works even "
> > +		     "when the CRC for the current frame index is lost.");
> > +	igt_subtest("ctx-flip-skip-current-frame") {
> > +		for_each_crtc(&data.display, data.crtc) {
> > +			igt_dynamic_f("pipe-%s", igt_crtc_name(data.crtc))
> > +				run_test(&data, test_ctx_flip_skip_current_frame);
> >  		}
> > -
> > -		igt_describe("Make sure the association between each CRC and its "
> > -			     "respective frame index is not broken when the driver "
> > -			     "performs a notifier context flip.");
> > -		pipe_test("ctx-flip-detection")
> > -			test_ctx_flip_detection(&data);
> > -
> > -		igt_describe("Make sure that igt_pipe_crc_get_current() works even "
> > -			     "when the CRC for the current frame index is lost.");
> > -		pipe_test("ctx-flip-skip-current-frame")
> > -			test_ctx_flip_skip_current_frame(&data);
> > -
> > -		igt_describe("Check that basic CRC readback using the outp-complete "
> > -			     "source works.");
> > -		pipe_test("source-outp-complete")
> > -			test_source(&data, "outp-complete");
> > -
> > -		igt_describe("Check that basic CRC readback using the rg source "
> > -			     "works.");
> > -		pipe_test("source-rg")
> > -			test_source(&data, "rg");
> > -
> > -		igt_describe("Make sure that the outp-inactive source is actually "
> > -			     "capturing the inactive raster.");
> > -		pipe_test("source-outp-inactive")
> > -			test_source_outp_inactive(&data);
> > -
> > -		igt_fixture() {
> > -			cleanup_test(&data);
> > +	}
> > +
> > +	igt_describe("Check that basic CRC readback using the outp-complete "
> > +		     "source works.");
> > +	igt_subtest("source-outp-complete") {
> > +		for_each_crtc(&data.display, data.crtc) {
> > +			igt_dynamic_f("pipe-%s", igt_crtc_name(data.crtc))
> > +				run_test(&data, test_source_outp_complete);
> > +		}
> > +	}
> > +
> > +	igt_describe("Check that basic CRC readback using the rg source "
> > +		     "works.");
> > +	igt_subtest("source-rg") {
> > +		for_each_crtc(&data.display, data.crtc) {
> > +			igt_dynamic_f("pipe-%s", igt_crtc_name(data.crtc))
> > +				run_test(&data, test_source_rg);
> > +		}
> > +	}
> > +
> > +	igt_describe("Make sure that the outp-inactive source is actually "
> > +		     "capturing the inactive raster.");
> > +	igt_subtest("source-outp-inactive") {
> > +		for_each_crtc(&data.display, data.crtc) {
> > +			igt_dynamic_f("pipe-%s", igt_crtc_name(data.crtc))
> > +				run_test(&data, test_source_outp_inactive);
> >  		}
> >  	}
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel