[Intel-gfx] [PATCH] igt/gem_create_stolen: Verifying extended gem_create ioctl
Chris Wilson
chris at chris-wilson.co.uk
Wed Apr 29 03:49:41 PDT 2015
On Wed, Apr 29, 2015 at 03:02:20PM +0530, 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)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> ---
> lib/ioctl_wrappers.c | 88 +++++++++++
> lib/ioctl_wrappers.h | 28 ++++
> tests/Makefile.sources | 1 +
> tests/gem_create_stolen.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/gem_pread.c | 43 +++++
> tests/gem_pwrite.c | 42 +++++
> 6 files changed, 594 insertions(+)
> create mode 100644 tests/gem_create_stolen.c
>
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index ff78ef1..5980067 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -379,6 +379,57 @@ void gem_sync(int fd, uint32_t handle)
> gem_set_domain(fd, 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 = 35; /* 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;
> + }
> +
> + return has_stolen_support;
> +}
> +
> +#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, uint32_t flags)
> +{
> + struct local_i915_gem_create_v2 create;
> + int ret;
With a name like gem_create_stolen() I expect it to only create a stolen
obj i.e. always set create.flags = I915_GEM_CREATE_STOLEN.
> + memset(&create, 0, sizeof(create));
> + create.handle = 0;
> + create.size = size;
> + create.flags = flags;
> + ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
The default behaviour is for gem_create() to die if it fails (and so
cause a FAIL, we do explicit checks to skip the test if we don't have
kernel support beforehand). That saves all the various igt_assert()
noise you have in your test cases.
> +
> + if (ret < 0)
> + return 0;
> +
> + errno = 0;
> + return create.handle;
> +}
> uint32_t __gem_create(int fd, int size)
> {
> struct drm_i915_gem_create create;
> @@ -1016,6 +1067,43 @@ uint64_t gem_mappable_aperture_size(void)
> return pci_dev->regions[bar].size;
> }
>
> +#define LOCAL_IOCTL_I915_GEM_GET_APERTURE DRM_IOR (DRM_COMMAND_BASE + DRM_I915_GEM_GET_APERTURE, struct local_i915_gem_get_aperture_v2)
> +/**
> + * gem_stolen_size:
> + *
> + * Feature test macro to query the total size of the stolen region
> + *
> + * Returns: The total size of the stolen region
> + */
> +uint64_t gem_stolen_size(fd)
> +{
> + struct local_i915_gem_get_aperture_v2 aperture;
> +
> + memset(&aperture, 0, sizeof(aperture));
> + aperture.aper_size = 256*1024*1024;
Ah, there's no point initializing aperture.aper_size since we don't
return it in case of ioctl failure anyway.
> + do_ioctl(fd, LOCAL_IOCTL_I915_GEM_GET_APERTURE, &aperture);
> +
> + return aperture.stolen_size;
> +}
> +
> +/**
> + * gem_available_stolen_size:
> + *
> + * Feature test macro to query the available size in the stolen region
> + *
> + * Returns: The available size in the stolen region
> + */
> +uint64_t gem_available_stolen_size(fd)
> +{
> + struct local_i915_gem_get_aperture_v2 aperture;
> +
> + memset(&aperture, 0, sizeof(aperture));
> + aperture.aper_size = 256*1024*1024;
Ditto.
> + do_ioctl(fd, LOCAL_IOCTL_I915_GEM_GET_APERTURE, &aperture);
> +
> + return aperture.stolen_available_size;
> +}
> +
> /**
> * gem_require_caching:
> * @fd: open i915 drm file descriptor
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index ced7ef3..af76af0 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -56,6 +56,16 @@ void gem_read(int fd, uint32_t handle, uint32_t offset, void *buf, uint32_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);
> +
> +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;
So without exposing flags to gem_create_stolen() we don't need to expose
the extended create struct or the extended aperture struct. That should
make life easier if we ever feel the desire to update the required min
version of libdrm and switch over to the kernel headers.
> diff --git a/tests/gem_create_stolen.c b/tests/gem_create_stolen.c
> new file mode 100644
> index 0000000..0224fc0
> +static void
> +stolen_no_mmap(int fd)
> +{
> + drm_intel_bo *bo;
> + int ret = 0;
> + uint32_t handle = 0;
> +
> + gem_require_stolen_support(fd);
> +
> + handle = gem_create_stolen(fd, SIZE, I915_CREATE_PLACEMENT_STOLEN);
> + igt_assert(handle != 0);
> +
> + bo = gem_handle_to_libdrm_bo(bufmgr, fd, "mmap_bo", handle);
> + igt_assert(bo != NULL);
> +
> + ret = drm_intel_bo_map(bo, 1);
> + igt_assert(ret != 0);
So this is weird. libdrm doesn't know the limitations (can't even tell
if the drm_intel_bo we generate is a full object or not - similar we can
expect such problems across flink/dma-buf, eek, I had better write a few
more tests to ensure that I have appropriate levels of fallback handling
in place). I would just use gem_mmap__cpu here to avoid the confusion of
using libdrm and the digression I just went on :)
> +
> + drm_intel_bo_unreference(bo);
> + gem_close(fd, handle);
> +}
> +
> +static void invalid_flag_test(int fd)
> +{
> + uint32_t handle = 0;
> +
> + gem_require_stolen_support(fd);
> +
> + handle = gem_create_stolen(fd, SIZE, ~I915_CREATE_PLACEMENT_STOLEN);
So here, I would just create a copy of the local_create_v2 and do
explicit ioctls. There is no point in complicating the more general
library API just to ease a single test. Or create a
__gem_create_with_flags() if you don't want to copy the struct around.
> +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_info("Total Stolen Size %u bytes\n",
> + gem_stolen_size(fd));
> + igt_info("Available Stolen Size %u bytes\n",
> + gem_available_stolen_size(fd));
> +
> + igt_subtest("stolen-invalid-flag")
> + invalid_flag_test(fd);
> +
> + 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);
Looks like a nice set of tests. Otoh, the only other thing that
concerned me was checking that stolen speed is resonable - but I see you
have that covered below.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx
mailing list