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

[Praveen Paneri]

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

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);

>
>
>> +		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(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.
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");