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

Praveen Paneri praveen.paneri at intel.com
Mon Jul 17 13:33:12 UTC 2017



On Friday 14 July 2017 07:55 PM, Paulo Zanoni wrote:
> 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 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.
>
> 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.
Okay! what if I just skip the loop for Y tiling with some message?
>
> Perhaps we could make Y tiling be a separate subtest? I'm not a huge
> fan of single tests that do tons of stuff.
Will it be possible in just one subtest. I thought we will have to 
duplicate all the subtests for Y-tile case? Plz suggest

Thanks,
Praveen
>
>>
>>>
>>>
>>>> +		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");


More information about the Intel-gfx mailing list