[RFC,i-g-t,v3] tests/gem_exec_pad_to_size: Test object padding at execbuf

Submitted by Tvrtko Ursulin on April 1, 2015, 3:14 p.m.

Details

Message ID 1427901292-2128-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Tvrtko Ursulin April 1, 2015, 3:14 p.m.
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag.

Similar to some other tests, it uses knowledge of the DRM
allocation policy in order to get two objects mapped adjacent
to each other. It is then possible to verify that the pad to
size flag will move them apart.

v2: Correct commit message. (Chris Wilson)
v3: Changes after code review by Chris Wilson.
     * No need for gem_sync after execbuf.
     * Drop caches before running.
     * Allocate one additional bo per iteration.
     * Don't explicitly set unused execbuf fields to zero.
     * One improved comment.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/Makefile.sources       |   1 +
 tests/gem_exec_pad_to_size.c | 252 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 253 insertions(+)
 create mode 100644 tests/gem_exec_pad_to_size.c

Patch hide | download patch | download mbox

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 0a974a6..5f21728 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -34,6 +34,7 @@  TESTS_progs_M = \
 	gem_exec_bad_domains \
 	gem_exec_faulting_reloc \
 	gem_exec_nop \
+	gem_exec_pad_to_size \
 	gem_exec_params \
 	gem_exec_parse \
 	gem_fenced_exec_thrash \
diff --git a/tests/gem_exec_pad_to_size.c b/tests/gem_exec_pad_to_size.c
new file mode 100644
index 0000000..0017586
--- /dev/null
+++ b/tests/gem_exec_pad_to_size.c
@@ -0,0 +1,252 @@ 
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Tvrtko Ursulin <tvrtko.ursulin@intel.com>
+ *
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "ioctl_wrappers.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "intel_reg.h"
+#include "igt_debugfs.h"
+
+#ifndef PAGE_SIZE
+#define PAGE_SIZE 4096
+#endif
+
+struct local_drm_i915_gem_exec_object2 {
+	/**
+	 * User's handle for a buffer to be bound into the GTT for this
+	 * operation.
+	 */
+	__u32 handle;
+
+	/** Number of relocations to be performed on this buffer */
+	__u32 relocation_count;
+	/**
+	 * Pointer to array of struct drm_i915_gem_relocation_entry containing
+	 * the relocations to be performed in this buffer.
+	 */
+	__u64 relocs_ptr;
+
+	/** Required alignment in graphics aperture */
+	__u64 alignment;
+
+	/**
+	 * Returned value of the updated offset of the object, for future
+	 * presumed_offset writes.
+	 */
+	__u64 offset;
+
+#define LOCAL_EXEC_OBJECT_NEEDS_FENCE (1<<0)
+#define LOCAL_EXEC_OBJECT_NEEDS_GTT	(1<<1)
+#define LOCAL_EXEC_OBJECT_WRITE	(1<<2)
+#define LOCAL_EXEC_OBJECT_PAD_TO_SIZE (1<<3)
+#define __LOCAL_EXEC_OBJECT_UNKNOWN_FLAGS -(LOCAL_EXEC_OBJECT_PAD_TO_SIZE<<1)
+	__u64 flags;
+
+	__u64 pad_to_size;
+	__u64 rsvd2;
+};
+
+static int
+exec(int fd, uint32_t handles[3], uint32_t pad_to_size[2], uint64_t *offsets)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct local_drm_i915_gem_exec_object2 gem_exec[3];
+	int ret = 0;
+
+	memset(gem_exec, 0, sizeof(gem_exec));
+
+	gem_exec[0].handle = handles[1];
+	gem_exec[0].flags = pad_to_size[0] ?
+			    LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0;
+	gem_exec[0].pad_to_size = pad_to_size[0];
+
+	gem_exec[1].handle = handles[2];
+	gem_exec[1].flags = pad_to_size[1] ?
+			    LOCAL_EXEC_OBJECT_PAD_TO_SIZE : 0;
+	gem_exec[1].pad_to_size = pad_to_size[1];
+
+	gem_exec[2].handle = handles[0];
+
+	memset(&execbuf, 0, sizeof(execbuf));
+
+	execbuf.buffers_ptr = (uintptr_t)gem_exec;
+	execbuf.buffer_count = 3;
+	execbuf.batch_len = 8;
+
+	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf))
+			ret = -errno;
+
+	if (ret == 0 && offsets) {
+		offsets[0] = gem_exec[0].offset;
+		offsets[1] = gem_exec[1].offset;
+	}
+
+	return ret;
+}
+
+static void
+require_pad_to_size(int fd, uint32_t handles[3])
+{
+	igt_require(exec(fd, handles, (uint32_t[2]){ PAGE_SIZE, PAGE_SIZE },
+			 NULL) == 0);
+}
+
+static void
+not_aligned(int fd, uint32_t handles[3])
+{
+	require_pad_to_size(fd, handles);
+
+	igt_require(exec(fd, handles, (uint32_t[2]){1 ,1},
+			 NULL) == -EINVAL);
+}
+
+static void
+padding(int fd, uint32_t handles[3])
+{
+	uint32_t pad_to_size[2] = {0, 0};
+	uint64_t offsets[2];
+	uint32_t distance;
+	bool neighbours = false;
+	unsigned int try, i;
+	const unsigned int max_tries = 4096; /* Finger in the air. */
+	uint32_t *loc_handles;
+	uint32_t eb_handles[3];
+
+	require_pad_to_size(fd, handles);
+
+	igt_drop_caches_set(DROP_ALL);
+
+	loc_handles = calloc(max_tries * 2, sizeof(uint32_t));
+	igt_assert(loc_handles);
+
+	/* Try with passed in handles first. */
+	loc_handles[0] = handles[1];
+	loc_handles[1] = handles[2];
+
+	/* Try to get two buffer object next to each other in GTT space.
+	 * We presume that sequential reservations are likely to be aligned and
+	 * try until we find a pair that is.
+	 */
+	for (try = 0; try < max_tries;) {
+		eb_handles[0] = handles[0];
+		eb_handles[1] = loc_handles[try];
+		eb_handles[2] = loc_handles[try + 1];
+
+		igt_assert(exec(fd, eb_handles, (uint32_t[2]){0, 0},
+				offsets) == 0);
+
+		if (offsets[1] > offsets[0]) {
+			distance = offsets[1] - offsets[0];
+			if (distance == PAGE_SIZE)
+				neighbours = true;
+			pad_to_size[0] = ALIGN(distance + PAGE_SIZE, PAGE_SIZE);
+		} else {
+			distance = offsets[0] - offsets[1];
+			if (distance == PAGE_SIZE)
+				neighbours = true;
+			pad_to_size[1] = ALIGN(distance + PAGE_SIZE, PAGE_SIZE);
+		}
+
+		if (neighbours)
+			break;
+
+		try++;
+
+		loc_handles[try + 1] = gem_create(fd, PAGE_SIZE);
+		igt_assert(loc_handles[try + 1]);
+	}
+
+	/* Test can't confidently run. */
+	if (!neighbours)
+		goto cleanup;
+
+	/* Re-exec with padding set. */
+	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);
+
+	if (offsets[1] > offsets[0])
+		distance = offsets[1] - offsets[0];
+	else
+		distance = offsets[0] - offsets[1];
+
+	/* Check that object are now away from each other. */
+	igt_assert(distance >= 2 * PAGE_SIZE);
+
+cleanup:
+	/* Cleanup our bos. */
+	for (i = 0;  i < try; i++)
+		gem_close(fd, loc_handles[2 + i]);
+
+	free(loc_handles);
+
+	igt_require(neighbours);
+}
+
+uint32_t batch[2] = {MI_BATCH_BUFFER_END};
+uint32_t handles[3];
+int fd;
+
+igt_main
+{
+	igt_fixture {
+		fd = drm_open_any();
+
+		handles[0] = gem_create(fd, PAGE_SIZE);
+		gem_write(fd, handles[0], 0, batch, sizeof(batch));
+
+		handles[1] = gem_create(fd, PAGE_SIZE);
+		handles[2] = gem_create(fd, PAGE_SIZE);
+	}
+
+	igt_subtest("pad_to_size")
+		require_pad_to_size(fd, handles);
+
+	igt_subtest("not_aligned")
+		not_aligned(fd, handles);
+
+	igt_subtest("padding")
+		padding(fd, handles);
+
+	igt_fixture {
+		gem_close(fd, handles[0]);
+		gem_close(fd, handles[1]);
+		gem_close(fd, handles[2]);
+
+		close(fd);
+	}
+}

Comments

On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This tests the new EXEC_OBJECT_PAD_TO_SIZE exec_object2 flag.
> 
> Similar to some other tests, it uses knowledge of the DRM
> allocation policy in order to get two objects mapped adjacent
> to each other. It is then possible to verify that the pad to
> size flag will move them apart.
> 
> v2: Correct commit message. (Chris Wilson)
> v3: Changes after code review by Chris Wilson.
>      * No need for gem_sync after execbuf.
>      * Drop caches before running.
>      * Allocate one additional bo per iteration.
>      * Don't explicitly set unused execbuf fields to zero.
>      * One improved comment.

Ah, sorry to be a nuisance, I'm getting to the actual test finally!

> +	/* Re-exec with padding set. */
> +	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);

The crux of the test is that we generate two objects such that

B_offset = A_offset + A_size

and then tell the kernel that A is actually 2*size (A_pad_to_size)

> +	if (offsets[1] > offsets[0])
> +		distance = offsets[1] - offsets[0];
> +	else
> +		distance = offsets[0] - offsets[1];

The assertion I feel should only be that

B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size

i.e. that they are now disjoint.

Your test is valid nevertheless, it is the ordering of the objects that
is confusing.

Hmm, can you loop until B_offset == A_offset + A_size such that we don't
have the confusion with order? And even assert that A_offset is
unchanged (though that smells like a little to much internal knowledge
leaking through, it is a desirable property of the allocator though - no
unnecessarily eviction) afterwards.

Do you agree that losing the handling of negative distances will make
the test simpler to understand (at the expense of doing more work in the
setup)?
-Chris
On 04/01/2015 04:42 PM, Chris Wilson wrote:
> On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote:
>> +	/* Re-exec with padding set. */
>> +	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);
>
> The crux of the test is that we generate two objects such that
>
> B_offset = A_offset + A_size
>
> and then tell the kernel that A is actually 2*size (A_pad_to_size)
>
>> +	if (offsets[1] > offsets[0])
>> +		distance = offsets[1] - offsets[0];
>> +	else
>> +		distance = offsets[0] - offsets[1];
>
> The assertion I feel should only be that
>
> B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size

I don't get this. B starts after A + padding, but B ends before A?

> i.e. that they are now disjoint.
>
> Your test is valid nevertheless, it is the ordering of the objects that
> is confusing.
>
> Hmm, can you loop until B_offset == A_offset + A_size such that we don't
> have the confusion with order? And even assert that A_offset is
> unchanged (though that smells like a little to much internal knowledge
> leaking through, it is a desirable property of the allocator though - no
> unnecessarily eviction) afterwards.
>
> Do you agree that losing the handling of negative distances will make
> the test simpler to understand (at the expense of doing more work in the
> setup)?

I thought my test logic is pretty straightforward:

1. Find two objects next to each other.
2. Add padding on the "lower" (addressed) object.
3. Ensure objects are now apart at least what the padding is.

Regards,

Tvrtko
On Wed, Apr 01, 2015 at 05:07:25PM +0100, Tvrtko Ursulin wrote:
> 
> On 04/01/2015 04:42 PM, Chris Wilson wrote:
> >On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote:
> >>+	/* Re-exec with padding set. */
> >>+	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);
> >
> >The crux of the test is that we generate two objects such that
> >
> >B_offset = A_offset + A_size
> >
> >and then tell the kernel that A is actually 2*size (A_pad_to_size)
> >
> >>+	if (offsets[1] > offsets[0])
> >>+		distance = offsets[1] - offsets[0];
> >>+	else
> >>+		distance = offsets[0] - offsets[1];
> >
> >The assertion I feel should only be that
> >
> >B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size
> 
> I don't get this. B starts after A + padding, but B ends before A?

s/&&/||/ 

> >i.e. that they are now disjoint.
> >
> >Your test is valid nevertheless, it is the ordering of the objects that
> >is confusing.
> >
> >Hmm, can you loop until B_offset == A_offset + A_size such that we don't
> >have the confusion with order? And even assert that A_offset is
> >unchanged (though that smells like a little to much internal knowledge
> >leaking through, it is a desirable property of the allocator though - no
> >unnecessarily eviction) afterwards.
> >
> >Do you agree that losing the handling of negative distances will make
> >the test simpler to understand (at the expense of doing more work in the
> >setup)?
> 
> I thought my test logic is pretty straightforward:
> 
> 1. Find two objects next to each other.
> 2. Add padding on the "lower" (addressed) object.
> 3. Ensure objects are now apart at least what the padding is.

What should happen is:

Initial layout:

|    ||aa||bb|
batch A   B

(B_offset == A_offset + PAGE)

We then submit an execbuf with A_pad_to_size = 2*PAGE, the kernel will
unbind A, keep B, then bind A (given an empty GTT):

|    |xxxx|bb||aaaaaa|
batch      B  A

but the distance here is just 1 PAGE. Then if B_pad_to_size is set to
2*PAGE:

|    |xxxxxxxx|aaaaaa||bbbbbb|
batch         A       B

If both are initially set to pad_to_size = 2*PAGE, we should see something
like:

|    ||aaaaaa||bbbbbb|
batch A       B

Given that the assertion should just be that the new objects do not
overlap assuming their size is now pad_to_size.
-Chris
On Wed, Apr 01, 2015 at 05:31:16PM +0100, Chris Wilson wrote:
> On Wed, Apr 01, 2015 at 05:07:25PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 04/01/2015 04:42 PM, Chris Wilson wrote:
> > >On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote:
> > >>+	/* Re-exec with padding set. */
> > >>+	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);
> > >
> > >The crux of the test is that we generate two objects such that
> > >
> > >B_offset = A_offset + A_size
> > >
> > >and then tell the kernel that A is actually 2*size (A_pad_to_size)
> > >
> > >>+	if (offsets[1] > offsets[0])
> > >>+		distance = offsets[1] - offsets[0];
> > >>+	else
> > >>+		distance = offsets[0] - offsets[1];
> > >
> > >The assertion I feel should only be that
> > >
> > >B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size
> > 
> > I don't get this. B starts after A + padding, but B ends before A?
> 
> s/&&/||/ 

Sorry &&. Gah, must be time for a coffee break.

The assertion is that the objects do not overlap based on the pad_to_size we
expect the kernel to apply, rather than their natural size. If the
kernel doesn't move the objects, they would it would fail.
-Chris
On 04/01/2015 05:39 PM, Chris Wilson wrote:
> On Wed, Apr 01, 2015 at 05:31:16PM +0100, Chris Wilson wrote:
>> On Wed, Apr 01, 2015 at 05:07:25PM +0100, Tvrtko Ursulin wrote:
>>>
>>> On 04/01/2015 04:42 PM, Chris Wilson wrote:
>>>> On Wed, Apr 01, 2015 at 04:14:52PM +0100, Tvrtko Ursulin wrote:
>>>>> +	/* Re-exec with padding set. */
>>>>> +	igt_assert(exec(fd, eb_handles, pad_to_size, offsets) == 0);
>>>>
>>>> The crux of the test is that we generate two objects such that
>>>>
>>>> B_offset = A_offset + A_size
>>>>
>>>> and then tell the kernel that A is actually 2*size (A_pad_to_size)
>>>>
>>>>> +	if (offsets[1] > offsets[0])
>>>>> +		distance = offsets[1] - offsets[0];
>>>>> +	else
>>>>> +		distance = offsets[0] - offsets[1];
>>>>
>>>> The assertion I feel should only be that
>>>>
>>>> B_offset + B_size <= A_offset && B_offset >= A_offset + A_pad_to_size
>>>
>>> I don't get this. B starts after A + padding, but B ends before A?
>>
>> s/&&/||/
>
> Sorry &&. Gah, must be time for a coffee break.
>
> The assertion is that the objects do not overlap based on the pad_to_size we
> expect the kernel to apply, rather than their natural size. If the
> kernel doesn't move the objects, they would it would fail.

You know all this time I didn't realize you were saying me check was 
broken, just that it was confusing. :)

Regards,

Tvrtko