[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*)>t_size_total,
> + (size_t*)>t_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