diff mbox

igt/kms_rotation_crc: Add a subtest to validate Y-tiled obj + Y fb modifier (v5)

Message ID 1446169473-20563-1-git-send-email-vivek.kasireddy@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kasireddy, Vivek Oct. 30, 2015, 1:44 a.m. UTC
The main goal of this subtest is to trigger the following warning in
the function i915_gem_object_get_fence():
	if (WARN_ON(!obj->map_and_fenceable))

To trigger this warning, the subtest first creates a Y-tiled object and
an associated framebuffer with the Y-fb modifier. Furthermore, to
prevent the map_and_fenceable from being set, we make sure that
the object does not have a normal VMA by refraining from rendering to the
object and by setting the rotation property upfront before calling commit.

v2: Do not call paint_squares and just use one output.

v3: Convert an if condition to igt_require and move the plane rotation
requirement further up before the fb allocation.

v4: After setting rotation to 90 and committing, change the rotation to
0 and commit once more. This is to test if the i915 driver hits any
warnings while pinning and unpinning an object that has both normal
and rotated views.

v5:
- Add another subtest to toggle the order of rotation
- Exhaustively test the i915 driver's pinning and unpinning code paths
  for any fence leaks by iterating until MAX available fences.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 tests/kms_rotation_crc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

Comments

Tvrtko Ursulin Oct. 30, 2015, 10:22 a.m. UTC | #1
On 30/10/15 01:44, Vivek Kasireddy wrote:
> The main goal of this subtest is to trigger the following warning in
> the function i915_gem_object_get_fence():
> 	if (WARN_ON(!obj->map_and_fenceable))
>
> To trigger this warning, the subtest first creates a Y-tiled object and
> an associated framebuffer with the Y-fb modifier. Furthermore, to
> prevent the map_and_fenceable from being set, we make sure that
> the object does not have a normal VMA by refraining from rendering to the
> object and by setting the rotation property upfront before calling commit.
>
> v2: Do not call paint_squares and just use one output.
>
> v3: Convert an if condition to igt_require and move the plane rotation
> requirement further up before the fb allocation.
>
> v4: After setting rotation to 90 and committing, change the rotation to
> 0 and commit once more. This is to test if the i915 driver hits any
> warnings while pinning and unpinning an object that has both normal
> and rotated views.
>
> v5:
> - Add another subtest to toggle the order of rotation
> - Exhaustively test the i915 driver's pinning and unpinning code paths
>    for any fence leaks by iterating until MAX available fences.
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   tests/kms_rotation_crc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 84 insertions(+)
>
> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> index cc9847e..34f8150 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -264,6 +264,80 @@ 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,
> +					   int toggle)
> +{
> +	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;
> +	igt_output_t *output = &display->outputs[0];
> +	igt_plane_t *plane;
> +	drmModeModeInfo *mode;
> +	unsigned int stride, size, w, h;
> +	uint32_t gem_handle;
> +	int num_fences = gem_available_fences(fd);
> +	int i, 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_fb(plane, &data->fb);
> +
> +	for (i = 0; i < num_fences + 1; i++) {
> +		igt_plane_set_rotation(plane, toggle ? IGT_ROTATION_0 : IGT_ROTATION_90);
> +		drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
> +					 DRM_MODE_OBJECT_PLANE,
> +					 plane->rotation_property,
> +					 plane->rotation);
> +		ret = igt_display_try_commit2(display, commit);
> +		igt_assert(ret == 0);
> +
> +		igt_plane_set_rotation(plane, toggle ? IGT_ROTATION_90 : IGT_ROTATION_0);
> +		drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
> +					 DRM_MODE_OBJECT_PLANE,
> +					 plane->rotation_property,
> +					 plane->rotation);
> +		ret = igt_display_try_commit2(display, commit);
> +		igt_assert(ret == 0);
> +	}

It manages to exhaust the fences with only one object? I was expecting 
that multiple objects will be required since if it is only one how come 
it allocates more than one fence register?

Regards,

Tvrtko
Kasireddy, Vivek Oct. 31, 2015, 1:45 a.m. UTC | #2
Hi Tvrtko,

On Fri, 30 Oct 2015 10:22:08 +0000
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> 
> On 30/10/15 01:44, Vivek Kasireddy wrote:
> > The main goal of this subtest is to trigger the following warning in
> > the function i915_gem_object_get_fence():
> > 	if (WARN_ON(!obj->map_and_fenceable))
> >
> > To trigger this warning, the subtest first creates a Y-tiled object
> > and an associated framebuffer with the Y-fb modifier. Furthermore,
> > to prevent the map_and_fenceable from being set, we make sure that
> > the object does not have a normal VMA by refraining from rendering
> > to the object and by setting the rotation property upfront before
> > calling commit.
> >
> > v2: Do not call paint_squares and just use one output.
> >
> > v3: Convert an if condition to igt_require and move the plane
> > rotation requirement further up before the fb allocation.
> >
> > v4: After setting rotation to 90 and committing, change the
> > rotation to 0 and commit once more. This is to test if the i915
> > driver hits any warnings while pinning and unpinning an object that
> > has both normal and rotated views.
> >
> > v5:
> > - Add another subtest to toggle the order of rotation
> > - Exhaustively test the i915 driver's pinning and unpinning code
> > paths for any fence leaks by iterating until MAX available fences.
> >
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >   tests/kms_rotation_crc.c | 84
> > ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84
> > insertions(+)
> >
> > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > index cc9847e..34f8150 100644
> > --- a/tests/kms_rotation_crc.c
> > +++ b/tests/kms_rotation_crc.c
> > @@ -264,6 +264,80 @@ 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,
> > +					   int toggle)
> > +{
> > +	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;
> > +	igt_output_t *output = &display->outputs[0];
> > +	igt_plane_t *plane;
> > +	drmModeModeInfo *mode;
> > +	unsigned int stride, size, w, h;
> > +	uint32_t gem_handle;
> > +	int num_fences = gem_available_fences(fd);
> > +	int i, 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_fb(plane, &data->fb);
> > +
> > +	for (i = 0; i < num_fences + 1; i++) {
> > +		igt_plane_set_rotation(plane, toggle ?
> > IGT_ROTATION_0 : IGT_ROTATION_90);
> > +		drmModeObjectSetProperty(fd,
> > plane->drm_plane->plane_id,
> > +					 DRM_MODE_OBJECT_PLANE,
> > +					 plane->rotation_property,
> > +					 plane->rotation);
> > +		ret = igt_display_try_commit2(display, commit);
> > +		igt_assert(ret == 0);
> > +
> > +		igt_plane_set_rotation(plane, toggle ?
> > IGT_ROTATION_90 : IGT_ROTATION_0);
> > +		drmModeObjectSetProperty(fd,
> > plane->drm_plane->plane_id,
> > +					 DRM_MODE_OBJECT_PLANE,
> > +					 plane->rotation_property,
> > +					 plane->rotation);
> > +		ret = igt_display_try_commit2(display, commit);
> > +		igt_assert(ret == 0);
> > +	}
> 
> It manages to exhaust the fences with only one object? I was
> expecting that multiple objects will be required since if it is only
> one how come it allocates more than one fence register?

Before I sent out this version, I did try with two objects to see if it
triggers any WARNs but it didn't. I am not sure if I did it right
though, I'll take a look at it again. However, when I tried this
version, that is, single-object-multiple-views, it did trigger the
following WARNS:

WARN_ON(!ggtt_vma || dev_priv->fence_regs[obj->fence_reg].pin_count >
ggtt_vma->pin_count)

if (WARN_ON(fence->pin_count)) 

I tested these with v3 of my i915 driver patch. Of-course, with v4,
I don't seen any WARNs. I'll take a closer look at triggering
any WARNs with 2 or more objects. However, one thing that was not clear
to me was when you said unpin both (in your previous comment), I was
wondering how I can do that from the test. Did you mean something like
this:

igt_plane_set_fb(plane, NULL);
igt_display_commit(display);

Thanks and Regards,
Vivek


> 
> Regards,
> 
> Tvrtko
Tvrtko Ursulin Nov. 2, 2015, 9:41 a.m. UTC | #3
On 31/10/15 01:45, Vivek Kasireddy wrote:
> Hi Tvrtko,
>
> On Fri, 30 Oct 2015 10:22:08 +0000
> Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>
>>
>> On 30/10/15 01:44, Vivek Kasireddy wrote:
>>> The main goal of this subtest is to trigger the following warning in
>>> the function i915_gem_object_get_fence():
>>> 	if (WARN_ON(!obj->map_and_fenceable))
>>>
>>> To trigger this warning, the subtest first creates a Y-tiled object
>>> and an associated framebuffer with the Y-fb modifier. Furthermore,
>>> to prevent the map_and_fenceable from being set, we make sure that
>>> the object does not have a normal VMA by refraining from rendering
>>> to the object and by setting the rotation property upfront before
>>> calling commit.
>>>
>>> v2: Do not call paint_squares and just use one output.
>>>
>>> v3: Convert an if condition to igt_require and move the plane
>>> rotation requirement further up before the fb allocation.
>>>
>>> v4: After setting rotation to 90 and committing, change the
>>> rotation to 0 and commit once more. This is to test if the i915
>>> driver hits any warnings while pinning and unpinning an object that
>>> has both normal and rotated views.
>>>
>>> v5:
>>> - Add another subtest to toggle the order of rotation
>>> - Exhaustively test the i915 driver's pinning and unpinning code
>>> paths for any fence leaks by iterating until MAX available fences.
>>>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>    tests/kms_rotation_crc.c | 84
>>> ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84
>>> insertions(+)
>>>
>>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>>> index cc9847e..34f8150 100644
>>> --- a/tests/kms_rotation_crc.c
>>> +++ b/tests/kms_rotation_crc.c
>>> @@ -264,6 +264,80 @@ 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,
>>> +					   int toggle)
>>> +{
>>> +	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;
>>> +	igt_output_t *output = &display->outputs[0];
>>> +	igt_plane_t *plane;
>>> +	drmModeModeInfo *mode;
>>> +	unsigned int stride, size, w, h;
>>> +	uint32_t gem_handle;
>>> +	int num_fences = gem_available_fences(fd);
>>> +	int i, 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_fb(plane, &data->fb);
>>> +
>>> +	for (i = 0; i < num_fences + 1; i++) {
>>> +		igt_plane_set_rotation(plane, toggle ?
>>> IGT_ROTATION_0 : IGT_ROTATION_90);
>>> +		drmModeObjectSetProperty(fd,
>>> plane->drm_plane->plane_id,
>>> +					 DRM_MODE_OBJECT_PLANE,
>>> +					 plane->rotation_property,
>>> +					 plane->rotation);
>>> +		ret = igt_display_try_commit2(display, commit);
>>> +		igt_assert(ret == 0);
>>> +
>>> +		igt_plane_set_rotation(plane, toggle ?
>>> IGT_ROTATION_90 : IGT_ROTATION_0);
>>> +		drmModeObjectSetProperty(fd,
>>> plane->drm_plane->plane_id,
>>> +					 DRM_MODE_OBJECT_PLANE,
>>> +					 plane->rotation_property,
>>> +					 plane->rotation);
>>> +		ret = igt_display_try_commit2(display, commit);
>>> +		igt_assert(ret == 0);
>>> +	}
>>
>> It manages to exhaust the fences with only one object? I was
>> expecting that multiple objects will be required since if it is only
>> one how come it allocates more than one fence register?
>
> Before I sent out this version, I did try with two objects to see if it
> triggers any WARNs but it didn't. I am not sure if I did it right
> though, I'll take a look at it again. However, when I tried this
> version, that is, single-object-multiple-views, it did trigger the
> following WARNS:
>
> WARN_ON(!ggtt_vma || dev_priv->fence_regs[obj->fence_reg].pin_count >
> ggtt_vma->pin_count)
>
> if (WARN_ON(fence->pin_count))
>
> I tested these with v3 of my i915 driver patch. Of-course, with v4,
> I don't seen any WARNs. I'll take a closer look at triggering
> any WARNs with 2 or more objects. However, one thing that was not clear

I thought you would need number of framebuffers / objects > num_fences 
and display all of them to trigger the running out of fences ref leak.

> to me was when you said unpin both (in your previous comment), I was
> wondering how I can do that from the test. Did you mean something like
> this:
>
> igt_plane_set_fb(plane, NULL);
> igt_display_commit(display);

Couldn't find in this thread where I said "unpin both" so don't know.

Regards,

Tvrtko
diff mbox

Patch

diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index cc9847e..34f8150 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -264,6 +264,80 @@  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,
+					   int toggle)
+{
+	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;
+	igt_output_t *output = &display->outputs[0];
+	igt_plane_t *plane;
+	drmModeModeInfo *mode;
+	unsigned int stride, size, w, h;
+	uint32_t gem_handle;
+	int num_fences = gem_available_fences(fd);
+	int i, 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_fb(plane, &data->fb);
+
+	for (i = 0; i < num_fences + 1; i++) {
+		igt_plane_set_rotation(plane, toggle ? IGT_ROTATION_0 : IGT_ROTATION_90);
+		drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
+					 DRM_MODE_OBJECT_PLANE,
+					 plane->rotation_property,
+					 plane->rotation);
+		ret = igt_display_try_commit2(display, commit);
+		igt_assert(ret == 0);
+
+		igt_plane_set_rotation(plane, toggle ? IGT_ROTATION_90 : IGT_ROTATION_0);
+		drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
+					 DRM_MODE_OBJECT_PLANE,
+					 plane->rotation_property,
+					 plane->rotation);
+		ret = igt_display_try_commit2(display, commit);
+		igt_assert(ret == 0);
+	}
+
+	kmstest_restore_vt_mode();
+	igt_remove_fb(fd, &data->fb);
+}
+
 igt_main
 {
 	data_t data = {};
@@ -345,6 +419,16 @@  igt_main
 		test_plane_rotation(&data, IGT_PLANE_PRIMARY);
 	}
 
+	igt_subtest_f("primary-rotation-90-to-0-Y-tiled") {
+		igt_require(gen >= 9);
+		test_plane_rotation_ytiled_obj(&data, IGT_PLANE_PRIMARY, 0);
+	}
+
+	igt_subtest_f("primary-rotation-0-to-90-Y-tiled") {
+		igt_require(gen >= 9);
+		test_plane_rotation_ytiled_obj(&data, IGT_PLANE_PRIMARY, 1);
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 	}