Re: [PATCH 1/3] igt/gem_stolen: Verifying extended gem_create ioctl

Tvrtko Ursulin
2015-07-22T16:00:14Z

Hi,

On 07/22/2015 02:45 PM, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@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: Fixed Rebase conflicts (Ankit)
> Added IGT_TEST_DESCRIPTION (Thomas Wood)
>
> v6: Added __gem_create_stolen for allowing error handling by User,
> updated gem_create_stolen to align with gem_create function, corrected
> fill_purge test (out of bound access), added testcase to validate
> allocating of more than 32 bit sized buffers (Tvrtko)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> ---
> lib/ioctl_wrappers.c | 74 ++++++++++
> lib/ioctl_wrappers.h | 13 ++
> tests/Makefile.sources | 1 +
> tests/gem_stolen.c | 363 +++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 451 insertions(+)
> create mode 100644 tests/gem_stolen.c
>
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index a269d0f..da65de5 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -389,6 +389,80 @@ 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 >= 2;
> + }
> +
> + 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)
> +uint32_t __gem_create_stolen(int fd, uint64_t 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;
> +}
> +
> +/**
> + * gem_create_stolen:
> + * @fd: open i915 drm file descriptor
> + * @size: desired size of the buffer
> + * @flags: desired placement i.e. stolen or shmem

There are no flags in the function prototype. :)

> + *
> + * 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, uint64_t 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;
> + do_ioctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> + igt_assert(create.handle);
> +
> + return create.handle;
> +}
> +
> +
> 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..ff1584a 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -56,6 +56,9 @@ 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, uint64_t size);
> +uint32_t gem_create_stolen(int fd, uint64_t 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 +70,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..d59e143
> --- /dev/null
> +++ b/tests/gem_stolen.c
> @@ -0,0 +1,363 @@
> +/*
> + * 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:
> + * 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.
> + *
> + * 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
> +#define DWORD_SIZE 4
> +#define DATA 0xdead
> +#define LARGE_SIZE 8*1024*1024*1024
> +#define MAX_OBJECTS 100
> +
> +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/DWORD_SIZE; i++)
> + 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/DWORD_SIZE; i++)
> + igt_assert(virt[i] == i);

You did not like suggestion to use igt_assert_eq?

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

Stray space remains. :)

> + buf[i] = DATA;
> +
> + gem_require_stolen_support(fd);
> +
> + handle = gem_create_stolen(fd, SIZE);
> +
> + 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/DWORD_SIZE; i ++)
> + igt_assert(virt[i] == DATA);
> +
> + 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/DWORD_SIZE];
> + uint32_t handle = 0;
> + uint32_t *virt;
> + int i, ret = 0;
> +
> + CLEAR(buf);
> +
> + gem_require_stolen_support(fd);
> +
> + handle = gem_create_stolen(fd, 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/DWORD_SIZE; i ++)
> + virt[i] = DATA;
> +
> + drm_intel_bo_unmap(bo);
> + drm_intel_bo_unreference(bo);
> +
> + gem_read(fd, handle, 0, buf, SIZE);
> +
> + for (i = 0; i < SIZE/DWORD_SIZE; i ++)
> + igt_assert(buf[i] == DATA);
> +
> + 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);
> + dest_handle = gem_create_stolen(fd, SIZE);
> +
> + 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);
> +
> + 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);
> +
> + /* Verify if the BO is zeroed */
> + virt = bo->virtual;
> + for (i = 0; i < SIZE / DWORD_SIZE; i++)
> + igt_assert(virt[i] == 0);
> +
> + drm_intel_bo_unmap(bo);
> + drm_intel_bo_unreference(bo);
> + gem_close(fd, handle);
> +}
> +
> +static void stolen_large_obj_alloc(int fd)
> +{
> + uint32_t handle = 0;
> +
> + gem_require_stolen_support(fd);
> + handle = __gem_create_stolen(fd, LARGE_SIZE);
> + igt_assert(handle == 0);
> +}

I think we need one more subtest, which will overflow the 32-bit size by
say one page. This would expose the bug in the kernel patch "[PATCH 2/4]
drm/i915: Support for creating Stolen memory backed objects".

> +static void stolen_fill_purge_test(int fd)
> +{
> + drm_intel_bo *bo;
> + int mret = 0, ret = 0, obj_count = 0, i = 0;

mret and ret unused?

Regards,

Tvrtko