[igt-dev] [PATCH i-g-t 5/8] tests/i915/kms_flip_tiling: Generalize away copy-pasta

[Ville Syrjälä]

On Mon, Oct 18, 2021 at 10:28:30AM +0300, Ville Syrjälä wrote:
> On Mon, Oct 18, 2021 at 12:32:18PM +0530, Karthik B S wrote:
> > On 10/14/2021 3:47 AM, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Replace the huge swaths of copypasta by just looping over the
> > > set of modifiers reported by the plane, and testing each against
> > > the others (and itself).
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >   tests/i915/kms_flip_tiling.c | 209 +++++------------------------------
> > >   1 file changed, 30 insertions(+), 179 deletions(-)
> > >
> > > diff --git a/tests/i915/kms_flip_tiling.c b/tests/i915/kms_flip_tiling.c
> > > index 63e0b8b9648f..e2e6619baeca 100644
> > > --- a/tests/i915/kms_flip_tiling.c
> > > +++ b/tests/i915/kms_flip_tiling.c
> > > @@ -161,195 +161,46 @@ igt_main
> > >   		igt_display_require(&data.display, data.drm_fd);
> > >   	}
> > >   
> > > -	/*
> > > -	 * Test that a page flip from a tiled buffer to a linear one works
> > > -	 * correctly. First, it sets the crtc with the linear buffer and
> > > -	 * generates a reference crc for the pipe. Then, the crtc is set with
> > > -	 * the tiled one and page flip to the linear one issued. A new crc is
> > > -	 * generated and compared to the reference one.
> > > -	 */
> > > -
> > > -	igt_describe("Check pageflip from tiled buffer to linear one works correctly with x tiling");
> > > -	igt_subtest_with_dynamic("flip-changes-tiling") {
> > > -		uint64_t modifier[2] = { I915_FORMAT_MOD_X_TILED,
> > > -					 DRM_FORMAT_MOD_LINEAR };
> > > +	igt_describe("Check pageflip between modifiers");
> > > +	igt_subtest_with_dynamic("flip-change-tiling") {
> > >   		enum pipe pipe;
> > >   
> > > -		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > > -			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > > -
> > >   		for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > > -			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > > -				test_flip_tiling(&data, pipe, output, modifier);
> > > -			test_cleanup(&data, pipe, output);
> > > -		}
> > > -	}
> > > -
> > > -	igt_describe("Check pageflip from tiled buffer to linear one works correctly with y tiling");
> > > -	igt_subtest_with_dynamic("flip-changes-tiling-Y") {
> > > -		uint64_t modifier[2] = { I915_FORMAT_MOD_Y_TILED,
> > > -					 DRM_FORMAT_MOD_LINEAR };
> > > -		enum pipe pipe;
> > > +			igt_plane_t *plane;
> > >   
> > > -		igt_require_fb_modifiers(data.drm_fd);
> > > +			igt_output_set_pipe(output, pipe);
> > >   
> > > -		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > > -			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > > +			plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> > >   
> > > -		igt_require(data.gen >= 9);
> > > +			for (int i = 0; i < plane->format_mod_count; i++) {
> > > +				if (plane->formats[i] != data.testformat)
> > > +					continue;
> > >   
> > > -		for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > > -			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > > -				test_flip_tiling(&data, pipe, output, modifier);
> > > -			test_cleanup(&data, pipe, output);
> > > -		}
> > > -	}
> > > -
> > > -	igt_describe("Check pageflip from tiled buffer to linear one works correctly with yf tiling");
> > > -	igt_subtest_with_dynamic("flip-changes-tiling-Yf") {
> > > -		uint64_t modifier[2] = { I915_FORMAT_MOD_Yf_TILED,
> > > -					 DRM_FORMAT_MOD_LINEAR };
> > > -		enum pipe pipe;
> > > +				for (int j = 0; j < plane->format_mod_count; j++) {
> > > +					uint64_t modifier[2] = {
> > > +						plane->modifiers[i],
> > > +						plane->modifiers[j],
> > > +					};
> > >   
> > > -		igt_require_fb_modifiers(data.drm_fd);
> > > +					if (plane->formats[j] != data.testformat)
> > > +						continue;
> > Move this check before assigning modifier[]?
> 
> Then I'd have to split the declaration and assignment.

Hmm. Or I guess I could fully embrace c99 and declare anywhere.
But I don't *think* we're doing that anywhere else in igt so far.

-- 
Ville Syrjälä
Intel