[PATCH i-g-t] igt/kms_rotation_crc: exercise invalid rotations

[PATCH i-g-t] igt/kms_rotation_crc: exercise invalid rotations

[Matthew Auld]

Add expect-to-fail tests for invalid rotations on each of the plane types.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 tests/kms_rotation_crc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index f94f8f1..a0a5a0f 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -479,6 +479,68 @@ err_commit:
 	igt_assert(ret == 0);
 }
 
+static void test_plane_rotation_invalid(data_t *data, enum igt_plane plane_type)
+{
+	igt_display_t *display = &data->display;
+	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
+	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;
+	igt_output_t *output = &display->outputs[0];
+	igt_plane_t *plane;
+	drmModeModeInfo *mode;
+	unsigned int stride, size, w, h;
+	uint32_t gem_handle;
+	int ret;
+
+	igt_require(output != NULL && output->valid == true);
+
+	plane = igt_output_get_plane(output, plane_type);
+	igt_require(igt_plane_supports_rotation(plane));
+
+	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
+		igt_require(data->display.has_universal_planes);
+		commit = COMMIT_UNIVERSAL;
+	}
+
+	mode = igt_output_get_mode(output);
+	w = mode->hdisplay;
+	h = mode->vdisplay;
+
+	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;
+
+	igt_plane_set_fb(plane, NULL);
+	igt_display_commit(display);
+
+	igt_plane_set_rotation(plane, data->rotation);
+	igt_plane_set_fb(plane, &data->fb);
+
+	drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
+				 DRM_MODE_OBJECT_PLANE,
+				 plane->rotation_property,
+				 plane->rotation);
+	ret = igt_display_try_commit2(display, commit);
+
+	kmstest_restore_vt_mode();
+	igt_remove_fb(fd, &data->fb);
+	igt_assert_eq(ret, -EINVAL);
+}
+
 igt_main
 {
 	data_t data = {};
@@ -579,6 +641,40 @@ igt_main
 		test_plane_rotation_exhaust_fences(&data, IGT_PLANE_PRIMARY);
 	}
 
+	igt_subtest_f("primary-rotation-90-invalid") {
+		igt_require(gen < 9);
+		data.rotation = IGT_ROTATION_90;
+		test_plane_rotation_invalid(&data, IGT_PLANE_PRIMARY);
+	}
+
+	igt_subtest_f("primary-rotation-270-invalid") {
+		igt_require(gen < 9);
+		data.rotation = IGT_ROTATION_270;
+		test_plane_rotation_invalid(&data, IGT_PLANE_PRIMARY);
+	}
+
+	igt_subtest_f("sprite-rotation-90-invalid") {
+		igt_require(gen < 9);
+		data.rotation = IGT_ROTATION_90;
+		test_plane_rotation_invalid(&data, IGT_PLANE_2);
+	}
+
+	igt_subtest_f("sprite-rotation-270-invalid") {
+		igt_require(gen < 9);
+		data.rotation = IGT_ROTATION_270;
+		test_plane_rotation_invalid(&data, IGT_PLANE_2);
+	}
+
+	igt_subtest_f("cursor-rotation-90-invalid") {
+		data.rotation = IGT_ROTATION_90;
+		test_plane_rotation_invalid(&data, IGT_PLANE_CURSOR);
+	}
+
+	igt_subtest_f("cursor-rotation-270-invalid") {
+		data.rotation = IGT_ROTATION_270;
+		test_plane_rotation_invalid(&data, IGT_PLANE_CURSOR);
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 	}
-- 
2.4.3

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

Re: [PATCH i-g-t] igt/kms_rotation_crc: exercise invalid rotations

[Ville Syrjälä]

On Wed, Mar 09, 2016 at 03:05:24PM +0000, Matthew Auld wrote:
> Add expect-to-fail tests for invalid rotations on each of the plane types.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  tests/kms_rotation_crc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index f94f8f1..a0a5a0f 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -479,6 +479,68 @@ err_commit:
>  	igt_assert(ret == 0);
>  }
>  
> +static void test_plane_rotation_invalid(data_t *data, enum igt_plane plane_type)
> +{
> +	igt_display_t *display = &data->display;
> +	uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
> +	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;
> +	igt_output_t *output = &display->outputs[0];
> +	igt_plane_t *plane;
> +	drmModeModeInfo *mode;
> +	unsigned int stride, size, w, h;
> +	uint32_t gem_handle;
> +	int ret;
> +
> +	igt_require(output != NULL && output->valid == true);
> +
> +	plane = igt_output_get_plane(output, plane_type);
> +	igt_require(igt_plane_supports_rotation(plane));
> +
> +	if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
> +		igt_require(data->display.has_universal_planes);
> +		commit = COMMIT_UNIVERSAL;
> +	}
> +
> +	mode = igt_output_get_mode(output);
> +	w = mode->hdisplay;
> +	h = mode->vdisplay;
> +
> +	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);

Y tiled fb will be rejected for gen < 9, should use X/linear for those.
Actually using X/linear for gen >= 9 would also be interesting to make
sure we reject 90/270 for those even when they are otherwise listead as
supported rotations.

Also why hand roll the fb creation instead of using the igt_fb stuff?

> +	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;
> +
> +	igt_plane_set_fb(plane, NULL);
> +	igt_display_commit(display);
> +
> +	igt_plane_set_rotation(plane, data->rotation);
> +	igt_plane_set_fb(plane, &data->fb);
> +
> +	drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
> +				 DRM_MODE_OBJECT_PLANE,
> +				 plane->rotation_property,
> +				 plane->rotation);

What's this doing here?

> +	ret = igt_display_try_commit2(display, commit);
> +
> +	kmstest_restore_vt_mode();
> +	igt_remove_fb(fd, &data->fb);
> +	igt_assert_eq(ret, -EINVAL);
> +}
> +
>  igt_main
>  {
>  	data_t data = {};
> @@ -579,6 +641,40 @@ igt_main
>  		test_plane_rotation_exhaust_fences(&data, IGT_PLANE_PRIMARY);
>  	}
>  
> +	igt_subtest_f("primary-rotation-90-invalid") {
> +		igt_require(gen < 9);
> +		data.rotation = IGT_ROTATION_90;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_PRIMARY);
> +	}
> +
> +	igt_subtest_f("primary-rotation-270-invalid") {
> +		igt_require(gen < 9);
> +		data.rotation = IGT_ROTATION_270;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_PRIMARY);
> +	}
> +
> +	igt_subtest_f("sprite-rotation-90-invalid") {
> +		igt_require(gen < 9);
> +		data.rotation = IGT_ROTATION_90;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_2);
> +	}
> +
> +	igt_subtest_f("sprite-rotation-270-invalid") {
> +		igt_require(gen < 9);
> +		data.rotation = IGT_ROTATION_270;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_2);
> +	}
> +
> +	igt_subtest_f("cursor-rotation-90-invalid") {
> +		data.rotation = IGT_ROTATION_90;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_CURSOR);
> +	}
> +
> +	igt_subtest_f("cursor-rotation-270-invalid") {
> +		data.rotation = IGT_ROTATION_270;
> +		test_plane_rotation_invalid(&data, IGT_PLANE_CURSOR);
> +	}

These seem rather needlessly hardcody. I would suggest looking at the
supported rotations for each plane and picking some invalid ones based
on that.

> +
>  	igt_fixture {
>  		igt_display_fini(&data.display);
>  	}
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] igt/kms_rotation_crc: exercise invalid rotations

[Matthew Auld]

> Y tiled fb will be rejected for gen < 9, should use X/linear for those.

Are you referring to the __gem_set_tiling call? Since this didn't fail
on BDW and I don't see any logic in i915_gem_set_tiling which
implements this.

> Actually using X/linear for gen >= 9 would also be interesting to
make sure we reject 90/270

Yup, good point

> I would suggest looking at the supported rotations for each plane
and picking some invalid ones based on that.

Sorry, I must be missing something, isn't that what it already does?
I've just picked the invalid rotations for the primary, cursor and sprite
planes.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] igt/kms_rotation_crc: exercise invalid rotations

[Ville Syrjälä]

On Thu, Mar 10, 2016 at 09:03:37AM +0000, Matthew Auld wrote:
> > Y tiled fb will be rejected for gen < 9, should use X/linear for those.
> 
> Are you referring to the __gem_set_tiling call? Since this didn't fail
> on BDW and I don't see any logic in i915_gem_set_tiling which
> implements this.

No, I mean the addfb. Note that the current logic in addfb is sort of
silly, and I tried to fix it in
https://lists.freedesktop.org/archives/intel-gfx/2016-February/087670.html
but no review so far.

> 
> > Actually using X/linear for gen >= 9 would also be interesting to
> make sure we reject 90/270
> 
> Yup, good point
> 
> > I would suggest looking at the supported rotations for each plane
> and picking some invalid ones based on that.
> 
> Sorry, I must be missing something, isn't that what it already does?
> I've just picked the invalid rotations for the primary, cursor and sprite
> planes.

I mean that instead of hardcoding the assumption which rotation are
unsupported, you could actually look at what the driver reports as
supported vs. not.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx