[Intel-gfx] [PATCH 1/3] igt/gem_stolen: Verifying extended gem_create ioctl

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Fri Jul 3 02:05:05 PDT 2015


On 07/01/2015 10:26 AM, ankitprasad.r.sharma at intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
>
> This patch adds the testcases for verifying the new extended
> gem_create ioctl. By means of this extended ioctl, memory
> placement of the GEM object can be specified, i.e. either
> shmem or stolen memory.
> These testcases include functional tests and interface tests for
> testing the gem_create ioctl call for stolen memory placement
>
> v2: Testing pread/pwrite functionality for stolen backed objects,
> added local struct for extended gem_create and gem_get_aperture,
> until headers catch up (Chris)
>
> v3: Removed get_aperture related functions, extended gem_pread
> to compare speeds for user pages with and without page faults,
> unexposed local_gem_create struct, changed gem_create_stolen
> usage (Chris)
>
> v4: Splitting patch to remove changes from gem_pread/gem_pwrite
> to another patch (Ankit)
>
> v5: Rebased to the latest (Ankit)
>      Added IGT_TEST_DESCRIPTION (Thomas Wood)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> ---
>   lib/ioctl_wrappers.c   |  59 ++++++++
>   lib/ioctl_wrappers.h   |  12 ++
>   tests/Makefile.sources |   1 +
>   tests/gem_stolen.c     | 363 +++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 435 insertions(+)
>   create mode 100644 tests/gem_stolen.c
>
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index a269d0f..0e868eb 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -389,6 +389,65 @@ void gem_sync(int fd, uint32_t handle)
>   		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
>   }
>
> +bool gem_create__has_stolen_support(int fd)
> +{
> +	static int has_stolen_support = -1;
> +	struct drm_i915_getparam gp;
> +	int val = -1;
> +
> +	if (has_stolen_support < 0) {
> +		memset(&gp, 0, sizeof(gp));
> +		gp.param = 36; /* CREATE_VERSION */
> +		gp.value = &val;
> +
> +		/* Do we have the extended gem_create_ioctl? */
> +		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +		has_stolen_support = val >= 1;

If ioctl fails it will declare stolen support. (val remains -1)

I would also suggest "has_stolen_support = val > 1" as clearer.

> +	}
> +
> +	return has_stolen_support;
> +}
> +
> +struct local_i915_gem_create_v2 {
> +	uint64_t size;
> +	uint32_t handle;
> +	uint32_t pad;
> +#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
> +	uint32_t flags;
> +};
> +
> +#define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
> +/**
> + * gem_create_stolen:
> + * @fd: open i915 drm file descriptor
> + * @size: desired size of the buffer
> + * @flags: desired placement i.e. stolen or shmem
> + *
> + * This wraps the new GEM_CREATE ioctl, which allocates a
> + * new gem buffer object of @size and placement based on @flags.
> + *
> + * Returns: The file-private handle of the created buffer object
> + */
> +
> +uint32_t gem_create_stolen(int fd, int size)
> +{

Why int for size?

> +	struct local_i915_gem_create_v2 create;
> +	int ret;
> +
> +	memset(&create, 0, sizeof(create));
> +	create.handle = 0;
> +	create.size = size;
> +	create.flags = I915_CREATE_PLACEMENT_STOLEN;
> +	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> +
> +	if (ret < 0)
> +		return 0;
> +
> +	errno = 0;
> +	return create.handle;
> +}
> +
> +

Interestingly gem_create_stolen copies implementation from __gem_create 
and differs from normal gem_create in handling of ioctl failure. But it 
removes the double underscore from the name. I think this creates 
unnecessary inconsistency. It should either have double underscore 
prefix or copy implementation from gem_create.

>   uint32_t __gem_create(int fd, int size)
>   {
>   	struct drm_i915_gem_create create;
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index bc5d4bd..bff5135 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -56,6 +56,8 @@ void gem_read(int fd, uint32_t handle, uint64_t offset, void *buf, uint64_t leng
>   void gem_set_domain(int fd, uint32_t handle,
>   		    uint32_t read_domains, uint32_t write_domain);
>   void gem_sync(int fd, uint32_t handle);
> +bool gem_create__has_stolen_support(int fd);
> +uint32_t gem_create_stolen(int fd, int size);
>   uint32_t __gem_create(int fd, int size);
>   uint32_t gem_create(int fd, uint64_t size);
>   void gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf);
> @@ -67,6 +69,16 @@ bool gem_mmap__has_wc(int fd);
>   void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
>
>   /**
> + * gem_require_stolen_support:
> + * @fd: open i915 drm file descriptor
> + *
> + * Test macro to query whether support for allocating objects from stolen
> + * memory is available. Automatically skips through igt_require() if not.
> + */
> +#define gem_require_stolen_support(fd) \
> +			igt_require(gem_create__has_stolen_support(fd))
> +
> +/**
>    * gem_require_mmap_wc:
>    * @fd: open i915 drm file descriptor
>    *
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index b9479cc..324cbb5 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -55,6 +55,7 @@ TESTS_progs_M = \
>   	gem_reset_stats \
>   	gem_ringfill \
>   	gem_set_tiling_vs_blt \
> +	gem_stolen \
>   	gem_storedw_batches_loop \
>   	gem_streaming_writes \
>   	gem_tiled_blits \
> diff --git a/tests/gem_stolen.c b/tests/gem_stolen.c
> new file mode 100644
> index 0000000..9caaefd
> --- /dev/null
> +++ b/tests/gem_stolen.c
> @@ -0,0 +1,363 @@
> +/*
> + * Copyright © 2011 Intel Corporation

Is the year correct?

> + *
> + * 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:
> + *    Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> + *
> + */
> +
> +/** @file gem_create_stolen.c
> + *
> + * This is a test for the extended gem_create ioctl, that includes allocation
> + * of object from stolen memory

Missing full stop.

> + *
> + * The goal is to simply ensure the basics work, and invalid input combinations
> + * are rejected.
> + */
> +
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include <getopt.h>
> +
> +#include <drm.h>
> +
> +#include "ioctl_wrappers.h"
> +#include "intel_bufmgr.h"
> +#include "intel_batchbuffer.h"
> +#include "intel_io.h"
> +#include "intel_chipset.h"
> +#include "igt_aux.h"
> +#include "drmtest.h"
> +#include "drm.h"
> +#include "i915_drm.h"
> +
> +IGT_TEST_DESCRIPTION("This test verifies the exetended gem_create ioctl,"
> +		     " that includes allocation of obj from stolen region");
> +#define CLEAR(s) memset(&s, 0, sizeof(s))
> +#define SIZE 1024*1024
> +
> +static struct drm_intel_bufmgr *bufmgr;
> +static struct intel_batchbuffer *batch;
> +
> +static void verify_copy_op(drm_intel_bo *src, drm_intel_bo *dest)
> +{
> +	uint32_t *virt, i, ret;
> +	/* Fill the src BO with dwords */
> +	ret = drm_intel_gem_bo_map_gtt(src);
> +	igt_assert(!ret);
> +
> +	virt = src->virtual;
> +	for (i = 0; i < SIZE/4; i++)

There is a lot of these hardcoded fours around. Maybe it would be nicer 
to use sizeof(*virt) or #define SIZE_DWORDS .. or something.

> +		virt[i] = i;
> +
> +	intel_copy_bo(batch, dest, src, SIZE);
> +
> +	ret = drm_intel_gem_bo_map_gtt(dest);
> +	igt_assert(!ret);
> +
> +	virt = dest->virtual;
> +	/* verify */
> +	for (i = 0; i < SIZE/4; i++) {
> +		if (virt[i] != i) {
> +			fprintf(stderr, "Expected 0x%08x, found 0x%08x "
> +					"at offset 0x%08x\n",
> +					i, virt[i], i * 4);

Doesn't %x add '0x' prefix already?

But probably could just use igt_assert_eq here without the extra 
fprintf. Also in other identical places.

> +			igt_assert(virt[i] == i);
> +		}
> +	}
> +
> +	drm_intel_bo_unmap(src);
> +	drm_intel_bo_unmap(dest);
> +}
> +
> +static void stolen_pwrite(int fd)
> +{
> +	drm_intel_bo *bo;
> +	uint32_t buf[SIZE/4];
> +	uint32_t handle = 0;
> +	uint32_t *virt;
> +	int i, ret = 0;
> +
> +	for (i = 0; i < SIZE/4; i ++)

Stray space.

> +		buf[i] = 0xdead;
> +
> +	gem_require_stolen_support(fd);
> +
> +	handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(handle != 0);
> +
> +	gem_write(fd, handle, 0, buf, SIZE);
> +	bo = gem_handle_to_libdrm_bo(bufmgr, fd, "bo", handle);
> +
> +	ret = drm_intel_gem_bo_map_gtt(bo);
> +	igt_assert(!ret);
> +
> +	virt = bo->virtual;
> +
> +	for (i = 0; i < SIZE/4; i ++)
> +		igt_assert(virt[i] == 0xdead);
> +
> +	drm_intel_bo_unmap(bo);
> +	drm_intel_bo_unreference(bo);
> +	gem_close(fd, handle);
> +}
> +
> +static void stolen_pread(int fd)
> +{
> +	drm_intel_bo *bo;
> +	uint32_t buf[SIZE/4];
> +	uint32_t handle = 0;
> +	uint32_t *virt;
> +	int i, ret = 0;
> +
> +	CLEAR(buf);
> +
> +	gem_require_stolen_support(fd);
> +
> +	handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(handle != 0);
> +
> +	bo = gem_handle_to_libdrm_bo(bufmgr, fd, "bo", handle);
> +
> +	ret = drm_intel_gem_bo_map_gtt(bo);
> +	igt_assert(!ret);
> +
> +	virt = bo->virtual;
> +
> +	for (i = 0; i < SIZE/4; i ++)
> +		virt[i] = 0xdead;
> +
> +	drm_intel_bo_unmap(bo);
> +	drm_intel_bo_unreference(bo);
> +
> +	gem_read(fd, handle, 0, buf, SIZE);
> +
> +	for (i = 0; i < SIZE/4; i ++)
> +		igt_assert(buf[i] == 0xdead);
> +
> +	gem_close(fd, handle);
> +}
> +
> +static void copy_test(int fd)
> +{
> +	drm_intel_bo *src, *dest;
> +	uint32_t src_handle = 0, dest_handle = 0;
> +	int ret = 0;
> +
> +	gem_require_stolen_support(fd);
> +
> +	src_handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(src_handle != 0);
> +
> +	dest_handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(dest_handle != 0);
> +
> +	src = gem_handle_to_libdrm_bo(bufmgr, fd, "src_bo", src_handle);
> +	dest = gem_handle_to_libdrm_bo(bufmgr, fd, "dst_bo", dest_handle);
> +
> +	igt_assert(src != NULL);
> +	igt_assert(dest != NULL);
> +
> +	verify_copy_op(src, dest);
> +
> +	drm_intel_bo_unreference(src);
> +	drm_intel_bo_unreference(dest);
> +	gem_close(fd, src_handle);
> +	gem_close(fd, dest_handle);
> +}
> +
> +static void verify_object_clear(int fd)
> +{
> +	drm_intel_bo *bo;
> +	uint32_t handle = 0;
> +	uint32_t *virt;
> +	int i, ret = 0;
> +
> +	gem_require_stolen_support(fd);
> +
> +	handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(handle != 0);
> +
> +	bo = gem_handle_to_libdrm_bo(bufmgr, fd, "verify_bo", handle);
> +	igt_assert(bo != NULL);
> +
> +	ret = drm_intel_gem_bo_map_gtt(bo);
> +	igt_assert(ret == 0);

There are igt_assert_eq and igt_assert_neq but in uses such as here 
(NULL vs non-NULL) I am not too bothered.

> +	/* Verify if the BO is zeroed */
> +	virt = bo->virtual;
> +	for (i = 0; i < SIZE / 4; i++)
> +		igt_assert(virt[i] == 0);
> +
> +	drm_intel_bo_unmap(bo);
> +	drm_intel_bo_unreference(bo);
> +	gem_close(fd, handle);
> +}
> +
> +static void stolen_fill_purge_test(int fd)
> +{
> +	drm_intel_bo *bo;
> +	int mret = 0, ret = 0, obj_count = 0, i = 0;
> +	int _ret = 0, j = 0;
> +	uint32_t handle[100] = {0};
> +	uint32_t new_handle = 0;

These two don't need to be initialized, plus, I don't see any protection 
for overflowing the handle array below?

> +	uint32_t *virt;
> +	int retained = 0;
> +
> +	gem_require_stolen_support(fd);
> +
> +	/* Exhaust Stolen space */
> +	while(ret == 0) {
> +		ret = -1;
> +		handle[i] = gem_create_stolen(fd, SIZE);
> +		if (handle[i] != 0) {
> +			ret = 0;
> +			bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +						     "verify_bo", handle[i]);
> +			igt_assert(bo != NULL);
> +
> +			_ret = drm_intel_gem_bo_map_gtt(bo);
> +			igt_assert(_ret == 0);
> +
> +			virt = bo->virtual;
> +			for (j = 0; j < SIZE / 4; j++)
> +				virt[j] = 0xab;
> +
> +			drm_intel_bo_unmap(bo);
> +			drm_intel_bo_unreference(bo);
> +
> +			obj_count++;
> +		}

Above loop and various "ret" flavours looks like could be simplified.

I think if you convert to do-while you can avoid the "ret = -1; ... ret 
= 0;" dance.

do {

	handle[i] = gem_create_stolen()
	if (handle[i]) {
		...
		<the main part of your loop>
		...
	}

	i++;
} while (handle[i-1]);

> +		i++;
> +	}
> +
> +	igt_assert(obj_count > 0);
> +
> +	/* Mark all stolen objects purgeable */
> +	for (i = 0; i < obj_count; i++)
> +		retained = gem_madvise(fd, handle[i], I915_MADV_DONTNEED);
> +
> +	/* Try to allocate one more object */
> +	new_handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(new_handle != 0);
> +
> +	/* Check if the retained object's memory contents are intact */
> +	for (i = 0; i < obj_count; i++) {
> +		retained = gem_madvise(fd, handle[i], I915_MADV_WILLNEED);
> +		if (retained) {
> +			bo = gem_handle_to_libdrm_bo(bufmgr, fd,
> +						     "verify_bo", handle[i]);
> +			igt_assert(bo != NULL);
> +
> +			_ret = drm_intel_gem_bo_map_gtt(bo);
> +			igt_assert(_ret == 0);
> +
> +			virt = bo->virtual;
> +			for (j = 0; j < SIZE / 4; j++)
> +				igt_assert(virt[j] == 0xab);
> +
> +			drm_intel_bo_unmap(bo);
> +			drm_intel_bo_unreference(bo);
> +		}
> +	}
> +
> +	gem_close(fd, new_handle);
> +	for (i = 0; i < obj_count; i++)
> +		gem_close(fd, handle[i]);
> +}
> +
> +static void
> +stolen_no_mmap(int fd)
> +{
> +	void *addr;
> +	uint32_t handle = 0;
> +
> +	gem_require_stolen_support(fd);
> +
> +	handle = gem_create_stolen(fd, SIZE);
> +	igt_assert(handle != 0);
> +
> +	addr = gem_mmap__cpu(fd, handle, 0, SIZE, PROT_READ | PROT_WRITE);
> +	igt_assert(addr == NULL);
> +
> +	gem_close(fd, handle);
> +}
> +
> +igt_main
> +{
> +	int i, fd, gtt_size_total, gtt_size_mappable;
> +	uint32_t devid;
> +
> +	igt_skip_on_simulation();
> +
> +	igt_fixture {
> +		fd = drm_open_any();
> +		devid = intel_get_drm_devid(fd);
> +
> +		drm_intel_get_aperture_sizes(fd, (size_t*)&gtt_size_total,
> +				(size_t*)&gtt_size_mappable);
> +		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
> +		batch = intel_batchbuffer_alloc(bufmgr, devid);
> +	}
> +
> +	igt_subtest("stolen-clear")
> +		verify_object_clear(fd);
> +
> +	/*
> +	 * stolen mem special cases - checking for non cpu mappable
> +	 */
> +	igt_subtest("stolen-no-mmap")
> +		stolen_no_mmap(fd);
> +
> +	/* checking for pread/pwrite interfaces */
> +	igt_subtest("stolen-pwrite")
> +		stolen_pwrite(fd);
> +
> +	igt_subtest("stolen-pread")
> +		stolen_pread(fd);
> +
> +	/* Functional Test - blt copy */
> +	igt_subtest("stolen-copy")
> +		copy_test(fd);
> +
> +	/* Filling stolen completely and marking all the objects
> +	 * purgeable. Then trying to add one more object, to verify
> +	 * the purging logic.
> +	 * Again marking all objects WILLNEED and verifying the
> +	 * contents of the retained objects.
> +	 */
> +	igt_subtest("stolen-fill-purge")
> +		stolen_fill_purge_test(fd);
> +
> +	igt_fixture {
> +		intel_batchbuffer_free(batch);
> +		drm_intel_bufmgr_destroy(bufmgr);
> +	}
> +}

Could also test creation of huge objects which can never be allocated 
from stolen, esp if size overflow 32-bits.

Regards,

Tvrtko


More information about the Intel-gfx mailing list