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

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

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
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.

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.


> +		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.


> +
> +			if (!prepare_test(data, mode,
> +					  igt_fb_tiling_to_mod(tilin
> g))) {
> +				igt_info("%s on pipe %s, connector
> %s: SKIPPED\n",
> +						igt_subtest_name(),
> +						kmstest_pipe_name(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.


> +				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