Re: [PATCH v2 7/7] igt/kms_fbc_crc.c : Add Y-tile tests

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Sex, 2017-07-14 às 19:25 +0530, Praveen Paneri escreveu:
> Hi Paulo,
> 
> On Thursday 13 July 2017 02:31 AM, Paulo Zanoni wrote:
> > Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu:
> > > Now that we have support for Y-tiled buffers, add another
> > > iteration of tests for Y-tiled buffers.
> > 
> > Have you tested this on platforms that don't support Y-tiled
> > buffers? I
> 
> Unfortunately I haven't...
> > don't see a check for that, so I wonder if we'll just fail some
> > assertion or correctly hit some igt_skip() call I couldn't find.
> 
> ...but I will add the check as you have mentioned
> > 
> > More below.
> > 
> > > 
> > > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com>
> > > ---
> > >  tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++--------
> > > ----
> > > ------------
> > >  1 file changed, 39 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> > > index 7964e05..cdf04c1 100644
> > > --- a/tests/kms_fbc_crc.c
> > > +++ b/tests/kms_fbc_crc.c
> > > @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data)
> > >  	igt_output_set_pipe(output, data->pipe);
> > >  }
> > > 
> > > -static void create_fbs(data_t *data, bool tiled, struct igt_fb
> > > *fbs)
> > > +static void create_fbs(data_t *data, uint64_t tiling, struct
> > > igt_fb
> > > *fbs)
> > >  {
> > >  	int rc;
> > >  	drmModeModeInfo *mode = igt_output_get_mode(data-
> > > >output);
> > > -	uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED
> > > :
> > > -				  LOCAL_DRM_FORMAT_MOD_NONE;
> > > 
> > >  	rc = igt_create_color_fb(data->drm_fd, mode->hdisplay,
> > > mode-
> > > > vdisplay,
> > > 
> > >  				 DRM_FORMAT_XRGB8888, tiling,
> > > @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data)
> > >  	struct igt_fb fbs[4];
> > >  	int i;
> > > 
> > > -	create_fbs(data, false, &fbs[0]);
> > > -	create_fbs(data, false, &fbs[2]);
> > > +	create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]);
> > > +	create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]);
> > > 
> > >  	fill_mmap_gtt(data, fbs[2].gem_handle, 0xff);
> > >  	fill_mmap_gtt(data, fbs[3].gem_handle, 0xff);
> > > @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data)
> > >  		igt_remove_fb(data->drm_fd, &fbs[i]);
> > >  }
> > > 
> > > -static bool prepare_test(data_t *data, enum test_mode test_mode)
> > > +static bool prepare_test(data_t *data, enum test_mode test_mode,
> > > uint64_t tiling)
> > >  {
> > >  	igt_display_t *display = &data->display;
> > >  	igt_output_t *output = data->output;
> > > @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum
> > > test_mode test_mode)
> > > 
> > >  	data->primary = igt_output_get_plane_type(data->output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > 
> > > -	create_fbs(data, true, data->fb);
> > > +	create_fbs(data, tiling, data->fb);
> > > 
> > >  	igt_pipe_crc_free(data->pipe_crc);
> > >  	data->pipe_crc = NULL;
> > > @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum
> > > test_mode mode)
> > > 
> > >  	reset_display(data);
> > > 
> > > -	for_each_pipe_with_valid_output(display, data->pipe,
> > > data-
> > > > output) {
> > > 
> > > -		prepare_crtc(data);
> > > -
> > > -		igt_info("Beginning %s on pipe %s, connector
> > > %s\n",
> > > -			  igt_subtest_name(),
> > > -			  kmstest_pipe_name(data->pipe),
> > > -			  igt_output_name(data->output));
> > > -
> > > -		if (!prepare_test(data, mode)) {
> > > -			igt_info("%s on pipe %s, connector %s:
> > > SKIPPED\n",
> > > -				  igt_subtest_name(),
> > > -				  kmstest_pipe_name(data->pipe),
> > > -				  igt_output_name(data-
> > > >output));
> > > -			continue;
> > > +	for (int tiling = I915_TILING_X;
> > > +	     tiling <= I915_TILING_Y; tiling++) {
> > 
> > What I don't understand is why this part of the code chooses to go
> > with
> > the tiling constants (I915_TILING_) only to later convert them to
> > modifiers with igt_fb_tiling_to_mod(). If this loop iterated over
> > the
> > modifiers directly we wouldn't need that. The rest of the code only
> > cares about the modifiers.
> 
> I chose to loop over tiling constants as they are in a simple
> arithmetic 
> order. anyhow I will just change that.

Just put the two local_format stuff in an array and iterate over it.

> 
> Also as mentioned above can I just add a check to skip Y-tiling
> tests 
> for older platforms?
> 
> igt_skip_on(intel_gen(intel_get_drm_devid(drm_fd)) < 9 &&
>                                 tiling == I915_TILING_Y);

You can't do this here because the same subtest tests X tiling.

Perhaps we could make Y tiling be a separate subtest? I'm not a huge
fan of single tests that do tons of stuff.

> 
> > 
> > 
> > > +		for_each_pipe_with_valid_output(display,
> > > +						data->pipe,
> > > data-
> > > > output) {
> > > 
> > > +			prepare_crtc(data);
> > > +
> > > +			igt_info("Beginning %s on pipe %s,
> > > connector
> > > "
> > > +					"%s, %s-tiled\n",
> > > +					igt_subtest_name(),
> > > +					kmstest_pipe_name(data-
> > > > pipe),
> > > 
> > > +					igt_output_name(data-
> > > > output),
> > > 
> > > +					(tiling ==
> > > I915_TILING_X) ?
> > > "x":"y" );
> > 
> > This change is not keeping the indentation style, things should be
> > aligned with the parens (although I see they're actually aligned
> > with
> > the quote, which is also weird). The same can be said for the other
> > two
> > igt_info() calls in this patch.
> 
> Will fix it
> > 
> > 
> > > +
> > > +			if (!prepare_test(data, mode,
> > > +					  igt_fb_tiling_to_mod(t
> > > ilin
> > > g))) {
> > > +				igt_info("%s on pipe %s,
> > > connector
> > > %s: SKIPPED\n",
> > > +						igt_subtest_name
> > > (),
> > > +						kmstest_pipe_nam
> > > e(da
> > > ta->pipe),
> > > +						igt_output_name(
> > > data
> > > ->output));
> > 
> > This one is missing the "%s-tiled" part that was added in the other
> > two
> > messages.
> > 
> > And we can probably create a "const char *tiling_name" variable to
> > store the %s part in order to avoid the same ternary operator in
> > the 3
> > if statements.
> 
> make sense, will add.
> > 
> > 
> > > +				continue;
> > > +			}
> > > +
> > > +			valid_tests++;
> > > +
> > > +			test_crc(data, mode);
> > > +
> > > +			igt_info("%s on pipe %s, connector %s"
> > > +					"%s-tiled: PASSED\n",
> > > +					igt_subtest_name(),
> > > +					kmstest_pipe_name(data-
> > > > pipe),
> > > 
> > > +					igt_output_name(data-
> > > > output),
> > > 
> > > +					(tiling ==
> > > I915_TILING_X) ?
> > > "x":"y" );
> > > +
> > > +			finish_crtc(data, mode);
> > >  		}
> > > -
> > > -		valid_tests++;
> > > -
> > > -		test_crc(data, mode);
> > > -
> > > -		igt_info("%s on pipe %s, connector %s:
> > > PASSED\n",
> > > -			  igt_subtest_name(),
> > > -			  kmstest_pipe_name(data->pipe),
> > > -			  igt_output_name(data->output));
> > > -
> > > -		finish_crtc(data, mode);
> > >  	}
> > > 
> > >  	igt_require_f(valid_tests, "no valid crtc/connector
> > > combinations found\n");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx