Re: [PATCH] igt/kms_rotation_crc: Add a subtest to validate Y-tiled obj + Y fb modifier (v2)

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 22/10/15 02:24, Vivek Kasireddy wrote:
> The main goal of this subtest is to verify whether flipping a

Need to change to commit message since there is no flipping involved.

> framebuffer with a Y fb modifier (90/270 degree rotation) and
> with an associated Y-tiled object works or not.

And also explain in more detail the requirement to not have a normal VMA 
before the rotated one is displayed.

> v2: Do not call paint_squares and just use one output.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   tests/kms_rotation_crc.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
>
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index cc9847e..63f27f8 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -264,6 +264,73 @@ static void test_plane_rotation(data_t *data, enum igt_plane plane_type)
>   	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
>   }
>
> +static void test_plane_rotation_ytiled_obj(data_t *data, enum igt_plane plane_type)
> +{
> +	igt_display_t *display = &data->display;
> +	uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
> +	uint32_t format = DRM_FORMAT_XRGB8888;
> +	int bpp = igt_drm_format_to_bpp(format);
> +	enum igt_commit_style commit = COMMIT_LEGACY;
> +	int fd = data->gfx_fd;
> +	int valid_tests = 0;
> +	igt_output_t *output = &display->outputs[0];
> +	int ret;
> +
> +	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
> +		igt_require(data->display.has_universal_planes);
> +		commit = COMMIT_UNIVERSAL;
> +	}
> +
> +	if (output && output->valid) {

How about getting rid of this indentation level and valid_tests by just 
doing igt_require on the condition above?

> +		igt_plane_t *plane;
> +		drmModeModeInfo *mode = igt_output_get_mode(output);
> +		unsigned int w = mode->hdisplay;
> +		unsigned int h = mode->vdisplay;
> +		unsigned int stride, size;
> +		uint32_t gem_handle;
> +
> +		for (stride = 512; stride < (w * bpp / 8); stride *= 2)
> +			;
> +		for (size = 1024*1024; size < stride * h; size *= 2)
> +			;
> +
> +		gem_handle = gem_create(fd, size);
> +		ret = __gem_set_tiling(fd, gem_handle, I915_TILING_Y, stride);
> +		igt_assert(ret == 0);
> +
> +		do_or_die(__kms_addfb(fd, gem_handle, w, h, stride,
> +			  format, tiling, LOCAL_DRM_MODE_FB_MODIFIERS,
> +			  &data->fb.fb_id));
> +		data->fb.width = w;
> +		data->fb.height = h;
> +		data->fb.gem_handle = gem_handle;
> +
> +		plane = igt_output_get_plane(output, plane_type);
> +		igt_require(igt_plane_supports_rotation(plane));

This would maybe be cleaner above the fb allocation, to group all the 
asserts together.

Otherwise looks fine to me.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx