[igt-dev] [PATCH i-g-t v3 1/4] lib/i915/gem_mman: add mmap_offset support
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Thu Nov 21 07:25:20 UTC 2019
On Wed, Nov 20, 2019 at 07:41:45PM +0000, Chris Wilson wrote:
> > +#define LOCAL_I915_PARAM_MMAP_OFFSET_VERSION 54
> > +
> > +static bool gem_has_mmap_offset(int fd)
> > +{
> > + int has_mmap_offset = 0;
> > + struct drm_i915_getparam gp;
> > +
> > + memset(&gp, 0, sizeof(gp));
> > + gp.param = LOCAL_I915_PARAM_MMAP_OFFSET_VERSION;
> > + gp.value = &has_mmap_offset;
> > + ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>
> * Blinks.
>
> Oh no it isn't.
>
> It's just I915_PARAM_MMAP_GTT_VERSION >= 4
I've double checked:
i915_getparam:
case I915_PARAM_MMAP_GTT_VERSION:
-> i915_gem_mmap_gtt_version()
-> return 3
case I915_PARAM_MMAP_OFFSET_VERSION:
-> return 1
Where's the magic 4 in GTT_VERSION? I don't see it.
Is some non merged patch containing it?
>
> > +
> > + return has_mmap_offset > 0;
> > +}
> > +
> > +void gem_require_mmap_offset(int i915)
> > +{
> > + igt_require(gem_has_mmap_offset(i915));
> > +}
> > +
> > +static int gem_mmap_gtt_version(int fd)
> > +{
> > + struct drm_i915_getparam gp;
> > + 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);
> > +
> > + return gtt_version;
> > +}
> > +
> > /**
> > * __gem_mmap__gtt:
> > * @fd: open i915 drm file descriptor
> > @@ -101,46 +134,63 @@ int gem_munmap(void *ptr, uint64_t size)
> > return ret;
> > }
> >
> > -bool gem_mmap__has_wc(int fd)
> > +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;
> > -
> > - /* 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;
> > + int has_wc = 0;
> > +
> > + struct drm_i915_getparam gp;
> > + int mmap_version = -1;
> > +
> > + 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 && gem_mmap_gtt_version(fd) >= 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;
>
> Ok.
>
> >
> > return has_wc > 0;
> > }
> >
> > +bool __gem_mmap_offset__has_wc(int fd)
> > +{
> > + int has_wc = 0;
> > +
> > + if (gem_has_mmap_offset(fd)) {
>
> Don't need the double ioctl... Hmm, oh wait. That's what you get for not
> checking arg.pad. Gah.
You suggest we can skip gem_has_mmap_offset() call by trial by fire
I915_GEM_MMAP_OFFSET ioctl()?
>
> > + struct local_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.flags = LOCAL_I915_MMAP_OFFSET_WC;
> > + has_wc = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_MMAP_OFFSET,
> > + &arg) == 0;
> > + gem_close(fd, arg.handle);
> > + }
>
> Ok.
>
> > +
> > + errno = 0;
> > +
> > + return has_wc > 0;
> > +}
> > +
> > +bool gem_mmap__has_wc(int fd)
> > +{
> > + return __gem_mmap_offset__has_wc(fd) || __gem_mmap__has_wc(fd);
> > +}
> > +
> > /**
> > * __gem_mmap:
> > * @fd: open i915 drm file descriptor
> > @@ -157,8 +207,8 @@ bool gem_mmap__has_wc(int fd)
> > *
> > * Returns: A pointer to the created memory mapping, NULL on failure.
> > */
> > -static void
> > -*__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned int prot, uint64_t flags)
> > +void *__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size,
> > + unsigned int prot, uint64_t flags)
> > {
> > struct drm_i915_gem_mmap arg;
> >
> > @@ -177,6 +227,43 @@ static void
> > return from_user_pointer(arg.addr_ptr);
> > }
> >
> > +/**
> > + * __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_i915_gem_mmap_offset arg;
> > + void *ptr;
> > +
> > + memset(&arg, 0, sizeof(arg));
> > + arg.handle = handle;
> > + arg.offset = offset;
>
> That's an interesting extension :-p
>
> Yeah, being able to mmap a slice of the object is one of the extensions
> I had in mind.
Doesn't it work as I expect? Isn't memory range verified in do_mmap()
and after syscall our implementation does the real job then?
>
> > + arg.flags = flags;
> > +
> > + if (igt_ioctl(fd, LOCAL_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;
> > +}
> > +
> > /**
> > * __gem_mmap__wc:
> > * @fd: open i915 drm file descriptor
> > @@ -194,7 +281,12 @@ 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);
> > + void *ptr = __gem_mmap_offset(fd, handle, offset, size, prot,
> > + LOCAL_I915_MMAP_OFFSET_WC);
> > + if (!ptr)
> > + ptr = __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);
>
> Ok.
>
> > +
> > + return ptr;
> > }
> >
> > /**
> > @@ -205,14 +297,62 @@ void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, un
> > * @size: size of the mmap arena
> > * @prot: memory protection bits as used by mmap()
> > *
> > - * Like __gem_mmap__wc() except we assert on failure.
> > + * Try to __gem_mmap_wc(). Assert on failure.
>
> It was better when it was __wc, at least then it matches the function
> name ;)
Ok, I'll fix that.
>
> > *
> > * Returns: A pointer to the created memory mapping
> > */
> > void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> > {
> > void *ptr = __gem_mmap__wc(fd, handle, offset, size, prot);
> > +
> > + igt_assert(ptr);
> > + return ptr;
> > +}
> > +
> > +/**
> > + * __gem_mmap__device_coherent:
> > + * @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()
> > + *
> > + * Try to __gem_mmap_offset, then __gem_mmap__wc(), then __gem_mmap__gtt().
>
> Don't repeat the code, we can read that! You need to explain the
> semantics.
>
> Returns a pointer to a block of linear device memory mapped into the
> process with WC semantics.
>
> That's a slight lie if we fallback to __gtt as it may end up being
> tiled, so note the exception.
Ok, I'll fix that.
>
> > + *
> > + * Returns: A pointer to the created memory mapping or NULL.
> > + */
> > +void *__gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
> > + uint64_t size, unsigned prot)
> > +{
> > + void *ptr = __gem_mmap_offset(fd, handle, offset, size, prot,
> > + LOCAL_I915_MMAP_OFFSET_WC);
> > + if (!ptr)
> > + ptr = __gem_mmap__wc(fd, handle, offset, size, prot);
> > +
> > + if (!ptr)
> > + ptr = __gem_mmap__gtt(fd, handle, size, prot);
> > +
> > + return ptr;
> > +}
> > +
> > +/**
> > + * gem_mmap__device_coherent:
> > + * @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()
> > + *
> > + * Call __gem_mmap__device__coherent(), asserts on fail.
> > + *
> > + * Returns: A pointer to the created memory mapping.
> > + */
> > +void *gem_mmap__device_coherent(int fd, uint32_t handle, uint64_t offset,
> > + uint64_t size, unsigned prot)
> > +{
> > + void *ptr = gem_mmap__device_coherent(fd, handle, offset, size, prot);
> > igt_assert(ptr);
>
> Might as well abide by kernel coding style, blank lines after variables.
>
> And if we are writing a library function, lets be nice and include debug
> info in the assert (handle, size, protection etc).
>
> You never know, we might start a trend of being helpful.
Ok.
>
> > +
> > return ptr;
> > }
> >
> > @@ -248,7 +388,11 @@ void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, u
> > */
> > void *gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
> > {
> > - void *ptr = __gem_mmap__cpu(fd, handle, offset, size, prot);
> > + void *ptr = __gem_mmap_offset(fd, handle, offset, size, prot,
> > + LOCAL_I915_MMAP_OFFSET_WB);
> > + if (!ptr)
> > + ptr = __gem_mmap__cpu(fd, handle, offset, size, prot);
>
> Ok.
>
> > +
> > igt_assert(ptr);
> > return ptr;
> > }
> > diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h
> > index 096ff592..e774d5c8 100644
> > --- a/lib/i915/gem_mman.h
> > +++ b/lib/i915/gem_mman.h
> > @@ -25,12 +25,46 @@
> > #ifndef GEM_MMAN_H
> > #define GEM_MMAN_H
> >
> > +#define LOCAL_I915_GEM_MMAP_OFFSET DRM_I915_GEM_MMAP_GTT
> > +#define LOCAL_IOCTL_I915_GEM_MMAP_OFFSET DRM_IOWR(DRM_COMMAND_BASE + \
> > + LOCAL_I915_GEM_MMAP_OFFSET, struct local_i915_gem_mmap_offset)
> > +
> > +struct local_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_I915_MMAP_OFFSET_WC (1 << 0)
> > +#define LOCAL_I915_MMAP_OFFSET_WB (1 << 1)
> > +#define LOCAL_I915_MMAP_OFFSET_UC (1 << 2)
> > +#define LOCAL_I915_MMAP_OFFSET_FLAGS \
> > + (LOCAL_I915_MMAP_OFFSET_WC | LOCAL_I915_MMAP_OFFSET_WB | LOCAL_I915_MMAP_OFFSET_UC)
> > +};
>
> I suggest just having a dummy commit to update i915_drm.h until the real
> commit lands.
> -Chris
Ok. I'll move structures and definitions there.
Zbigniew
More information about the igt-dev
mailing list