[igt-dev] [PATCH i-g-t] lib/i915: add mmap_offset support
Chris Wilson
chris at chris-wilson.co.uk
Fri Feb 22 22:29:40 UTC 2019
Quoting Chris Wilson (2019-02-22 22:13:18)
> Quoting Antonio Argenziano (2019-02-22 22:01:43)
> >
> >
> > On 22/02/19 13:59, Antonio Argenziano wrote:
> > > From: "Kalamarz, Lukasz" <lukasz.kalamarz at intel.com>
> > >
> > > With recently proposed changes, IGT need to start supporting new
> > > way of mmaping object, which will be used from now by default.
> > > This patch modify gem_mmap_wc and gem_mmap functions to be
> > > in sync with those changes.
> > >
> > > v2:
> > > - Fix IOCTL number. (Daniele)
> > > - Move wrappers to new file. (Chris)
> >
> > Not sure how much of the other stuff we might want to bring over so I've
> > moved only the new wrappers for now.
>
> It's a sensible chunk to review, so no worries, more can come later.
>
> > > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > > Cc: Janulgue Abdiel <abdiel.janulgue at intel.com>
> > > Cc: Matthew Auld <matthew.auld at intel.com>
> > > Cc: Michal Winiarski <michal.winiarski at intel.com>
> > > Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> > > Cc: Daniele Spurio Ceraolo <daniele.ceraolospurio at intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> > > ---
> > > lib/i915/gem_mman.c | 87 ++++++++++++++++++++++++++++++++++++++++++++
> > > lib/i915/gem_mman.h | 61 +++++++++++++++++++++++++++++++
> > > lib/ioctl_wrappers.c | 67 +++++++++++++++++++++++-----------
> > > lib/ioctl_wrappers.h | 1 +
> > > lib/meson.build | 1 +
> > > 5 files changed, 196 insertions(+), 21 deletions(-)
> > > create mode 100644 lib/i915/gem_mman.c
> > > create mode 100644 lib/i915/gem_mman.h
> > >
> > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > > new file mode 100644
> > > index 00000000..fb949f44
> > > --- /dev/null
> > > +++ b/lib/i915/gem_mman.c
> > > @@ -0,0 +1,87 @@
> > > +/*
> > > + * Copyright © 2007, 2011, 2013, 2014, 2019 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.
> > > + *
> > > + */
> > > +
> > > +#include <stdbool.h>
> > > +#include <sys/ioctl.h>
> > > +
> > > +#include "igt_core.h"
> > > +#include "ioctl_wrappers.h"
> > > +
> > > +#include "gem_mman.h"
> > > +
> > > +
> > > +bool has_mmap_offset(int fd)
> > > +{
> > > + static int has_mmap_offset = -1;
> > > +
> > > + if (has_mmap_offset == -1) {
> > > + struct drm_i915_getparam gp;
> > > +
> > > + has_mmap_offset = 0;
> > > +
> > > + memset(&gp, 0, sizeof(gp));
> > > + gp.param = 0x55; /* I915_PARAM_MMAP_OFFSET_VERSION */
> > > + gp.value = &has_mmap_offset;
> > > + ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > + }
> > > +
> > > + return has_mmap_offset > 0;
> > > +}
> > > +
> > > +/**
> > > + * __gem_mmap_offset:
> > > + * @fd: open i915 drm file descriptor
> > > + * @handle: gem buffer object handle
> > > + * @offset: offset in the gem buffer of the mmap arena
> > > + * @size: size of the mmap arena
> > > + * @prot: memory protection bits as used by mmap()
> > > + * @flags: flags used to determine caching
> > > + *
> > > + * Similar to __gem_mmap but use MMAP_OFFSET IOCTL.
> > > + *
> > > + * Returns: A pointer to the created memory mapping, NULL on failure.
> > > + */
> > > +void
> > > +*__gem_mmap_offset(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned int prot, uint64_t flags)
> > > +{
> > > + struct local_drm_i915_gem_mmap_offset arg;
> > > + void *ptr;
> > > +
> > > + memset(&arg, 0, sizeof(arg));
> > > + arg.handle = handle;
> > > + arg.offset = offset;
> > > + arg.flags = flags;
> > > +
> > > + if (igt_ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg))
> > > + return NULL;
> > > +
> > > + ptr = mmap64(0, size, prot, MAP_SHARED, fd, arg.offset);
> > > +
> > > + if (ptr == MAP_FAILED)
> > > + ptr = NULL;
> > > + else
> > > + errno = 0;
> > > +
> > > + return ptr;
> > > +}
> > > diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h
> > > new file mode 100644
> > > index 00000000..2f24ca99
> > > --- /dev/null
> > > +++ b/lib/i915/gem_mman.h
> > > @@ -0,0 +1,61 @@
> > > +/*
> > > + * Copyright © 2007, 2011, 2013, 2014, 2019 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.
> > > + *
> > > + */
> > > +
> > > +#ifndef GEM_MMAN_H
> > > +#define GEM_MMAN_H
> > > +
> > > +struct local_drm_i915_gem_mmap_offset {
> > > + /** Handle for the object being mapped. */
> > > + __u32 handle;
> > > + __u32 pad;
> > > + /**
> > > + * Fake offset to use for subsequent mmap call
> > > + *
> > > + * This is a fixed-size type for 32/64 compatibility.
> > > + */
> > > + __u64 offset;
> > > +
> > > + /**
> > > + * Flags for extended behaviour.
> > > + *
> > > + * It is mandatory that either one of the _WC/_WB flags
> > > + * should be passed here.
> > > + */
> > > + __u64 flags;
> > > +};
> > > +
> > > +#define LOCAL_DRM_I915_GEM_MMAP_OFFSET 0x24
> > > +#define LOCAL_I915_MMAP_OFFSET_WC (1 << 0)
> > > +#define LOCAL_I915_MMAP_OFFSET_WB (1 << 1)
> > > +#define LOCAL_I915_MMAP_OFFSET_UC (1 << 2)
> > > +
> > > +#define LOCAL_DRM_IOCTL_I915_GEM_MMAP_OFFSET \
> > > + DRM_IOWR(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_MMAP_OFFSET, struct local_drm_i915_gem_mmap_offset)
> > > +
> > > +bool has_mmap_offset(int fd);
> > > +
> > > +void *__gem_mmap_offset(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned int prot, uint64_t flags);
> > > +
> > > +#endif /* GEM_MMAN_H */
> > > +
> > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > index 404c2fbf..c11b0146 100644
> > > --- a/lib/ioctl_wrappers.c
> > > +++ b/lib/ioctl_wrappers.c
> > > @@ -707,35 +707,50 @@ bool gem_mmap__has_wc(int fd)
> > > static int has_wc = -1;
> > >
> > > if (has_wc == -1) {
> > > - struct drm_i915_getparam gp;
> > > - int mmap_version = -1;
> > > - int gtt_version = -1;
> > >
> > > has_wc = 0;
> > >
> > > - memset(&gp, 0, sizeof(gp));
> > > - gp.param = I915_PARAM_MMAP_GTT_VERSION;
> > > - gp.value = >t_version;
> > > - ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > -
> > > - memset(&gp, 0, sizeof(gp));
> > > - gp.param = I915_PARAM_MMAP_VERSION;
> > > - gp.value = &mmap_version;
> > > - ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > -
> > > - /* Do we have the new mmap_ioctl with DOMAIN_WC? */
> > > - if (mmap_version >= 1 && gtt_version >= 2) {
> > > - struct drm_i915_gem_mmap arg;
> > > + /* Do we have the new mmap_offset ioctl? */
> > > + if (has_mmap_offset(fd)) {
> > > + struct local_drm_i915_gem_mmap_offset arg;
> > >
> > > /* Does this device support wc-mmaps ? */
> > > memset(&arg, 0, sizeof(arg));
> > > arg.handle = gem_create(fd, 4096);
> > > arg.offset = 0;
> > > - arg.size = 4096;
> > > - arg.flags = I915_MMAP_WC;
> > > - has_wc = igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg) == 0;
> > > + arg.flags = LOCAL_I915_MMAP_OFFSET_WC;
> > > + has_wc = igt_ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg) == 0;
> > > gem_close(fd, arg.handle);
> > > + } else {
> > > + struct drm_i915_getparam gp;
> > > + int mmap_version = -1;
> > > + int gtt_version = -1;
> > > +
> > > + memset(&gp, 0, sizeof(gp));
> > > + gp.param = I915_PARAM_MMAP_GTT_VERSION;
> > > + gp.value = >t_version;
> > > + ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > +
> > > + memset(&gp, 0, sizeof(gp));
> > > + gp.param = I915_PARAM_MMAP_VERSION;
> > > + gp.value = &mmap_version;
> > > + ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > > +
> > > + /* Do we have the mmap_ioctl with DOMAIN_WC? */
> > > + if (mmap_version >= 1 && gtt_version >= 2) {
> > > + struct drm_i915_gem_mmap arg;
> > > +
> > > + /* Does this device support wc-mmaps ? */
> > > + memset(&arg, 0, sizeof(arg));
> > > + arg.handle = gem_create(fd, 4096);
> > > + arg.offset = 0;
> > > + arg.size = 4096;
> > > + arg.flags = I915_MMAP_WC;
> > > + has_wc = igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg) == 0;
> > > + gem_close(fd, arg.handle);
> > > + }
> > > }
> > > +
> > > errno = 0;
> > > }
> > >
> > > @@ -795,7 +810,10 @@ static void
> > > */
> > > void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> > > {
> > > - return __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);
> > > + if (has_mmap_offset(fd))
> > > + return __gem_mmap_offset(fd, handle, offset, size, prot, LOCAL_I915_MMAP_OFFSET_WC);
> > > + else
> > > + return __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);
>
> So this is basically,
>
> if (__gem_mmap_offset())
> return __gem_mmap();
>
> because statics are bad!
>
> For the purposes of API testing, we want to be able to still call
> __gem_mmap__wc without any magic, and similarly __gem_mmap__offset
> directly.
>
> I would suggest that we only put the magic fallback handling into
> gem_mmap__wc() and keep __gem_mmap__wc around for the explicit tests of
> that ioctl. Same for __gem_mmap__cpu.
>
> The only problem will be some callers of __gem_mmap__wc would like the
> nice automagic conversion. But hopefully they are few and far in between
> and can be converted to the igt_assert wielding variants instead -- or
> we find some other compromise.
One compromise would be to say that all ioctl testing is done is
gem_mmap_wc etc, and that they can have their own custom ioctl wrapper.
And in the library for everyone else, we put the magic wrapper as those
callers only care about obtaining WC access to the bo not how.
-Chris
More information about the igt-dev
mailing list