Re: [igt-dev] [PATCH i-g-t 2/6] lib/ioctl_wrapper: Implement __gem_mmap

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 01/09/2019 09:40 AM, Lukasz Kalamarz wrote:
> Previous implementation of __gem_mmap__cpu and __gem_mmap_wc  only
> differ with setting proper flag for caching. This patch implement __gem_mmap
> which merge those two functions into one wrapper.
> Small documentation modification are add to be in par with new implementation.
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   lib/ioctl_wrappers.c | 96 +++++++++++++++++++++++++++++++-------------
>   lib/ioctl_wrappers.h |  1 +
>   2 files changed, 68 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index f71f0e32..87744edc 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -736,6 +736,45 @@ bool gem_mmap__has_wc(int fd)
>   	return has_wc > 0;
>   }
>   
> +/**
> + * __gem_mmap:
> + * @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()
> + * @wc: flag marking if we want to set up MMAP_WC flag
> + *
> + * This functions wraps up procedure to establish a memory mapping through
> + * direct cpu access, bypassing the gpu (valid for wc == false). For wc == true
> + * it also bypass cpu caches completely and GTT system agent (i.e. there is no
> + * automatic tiling of the mmapping through the fence registers).
> + *
> + * Returns: A pointer to the created memory mapping, NULL on failure.
> + */
> +void *__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot, bool wc)

This shouldn't be called from outside this file, so you can just make it 
static.

> +{
> +	struct drm_i915_gem_mmap arg;
> +
> +	if (wc & !gem_mmap__has_wc(fd)) {
> +		errno = ENOSYS;
> +		return NULL;
> +	}
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.handle = handle;
> +	arg.offset = offset;
> +	arg.size = size;
> +	arg.flags = wc ? I915_MMAP_WC : 0;
> +	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
> +		return NULL;
> +
> +	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr), arg.size));
> +
> +	errno = 0;
> +	return from_user_pointer(arg.addr_ptr);
> +}
> +
>   /**
>    * __gem_mmap__wc:
>    * @fd: open i915 drm file descriptor
> @@ -753,25 +792,20 @@ bool gem_mmap__has_wc(int fd)
>    */
>   void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
>   {
> -	struct drm_i915_gem_mmap arg;
> +       struct drm_i915_gem_mmap mmap_arg;
>   

Any reason not to just do:

void *__gem_mmap__wc(...)
{
	return __gem_mmap(..., true);
}

?

> -	if (!gem_mmap__has_wc(fd)) {
> -		errno = ENOSYS;
> -		return NULL;
> -	}
> +       memset(&mmap_arg, 0, sizeof(mmap_arg));
> +       mmap_arg.handle = handle;
> +       mmap_arg.offset = offset;
> +       mmap_arg.size = size;
> +       mmap_arg.flags = I915_MMAP_WC;
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &mmap_arg))
> +               return NULL;
>   
> -	memset(&arg, 0, sizeof(arg));
> -	arg.handle = handle;
> -	arg.offset = offset;
> -	arg.size = size;
> -	arg.flags = I915_MMAP_WC;
> -	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
> -		return NULL;
> +       VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(mmap_arg.addr_ptr), mmap_arg.size));
>   
> -	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr), arg.size));
> -
> -	errno = 0;
> -	return from_user_pointer(arg.addr_ptr);
> +       errno = 0;
> +       return from_user_pointer(mmap_arg.addr_ptr);

Looks like you've changed all tabs to spaces here. Can just remove it 
all if you follow the above suggestion.

>   }
>   
>   /**
> @@ -782,13 +816,16 @@ 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.
> + * This functions wraps up procedure to establish a memory mapping through
> + * direct cpu access, bypassing the gpu and cpu caches completely and also
> + * bypassing the GTT system agent (i.e. there is no automatic tiling of
> + * the mmapping through the fence registers).We assert on failure.
>    *
>    * 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);
> +	void *ptr = __gem_mmap(fd, handle, offset, size, prot, true);

If you follow the above suggestion for __gem_mmap__wc then there'll be 
no need to update here.

>   	igt_assert(ptr);
>   	return ptr;
>   }
> @@ -810,17 +847,17 @@ void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, u

The same suggestion I had for the __wc path apply for __cpu


Daniele

>   {
>   	struct drm_i915_gem_mmap mmap_arg;
>   
> -	memset(&mmap_arg, 0, sizeof(mmap_arg));
> -	mmap_arg.handle = handle;
> -	mmap_arg.offset = offset;
> -	mmap_arg.size = size;
> -	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &mmap_arg))
> -		return NULL;
> +       memset(&mmap_arg, 0, sizeof(mmap_arg));
> +       mmap_arg.handle = handle;
> +       mmap_arg.offset = offset;
> +       mmap_arg.size = size;
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &mmap_arg))
> +               return NULL;
>   
> -	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(mmap_arg.addr_ptr), mmap_arg.size));
> +       VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(mmap_arg.addr_ptr), mmap_arg.size));
>   
> -	errno = 0;
> -	return from_user_pointer(mmap_arg.addr_ptr);
> +       errno = 0;
> +       return from_user_pointer(mmap_arg.addr_ptr);
>   }
>   
>   /**
> @@ -831,13 +868,14 @@ void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, u
>    * @size: size of the mmap arena
>    * @prot: memory protection bits as used by mmap()
>    *
> - * Like __gem_mmap__cpu() except we assert on failure.
> + * This functions wraps up procedure to establish a memory mapping through
> + * direct cpu access, bypassing the gpu completely and we assert on failure.
>    *
>    * Returns: A pointer to the created memory mapping
>    */
>   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(fd, handle, offset, size, prot, false);
>   	igt_assert(ptr);
>   	return ptr;
>   }
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b22b36b0..51e66c08 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -97,6 +97,7 @@ void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsi
>   void *__gem_mmap__gtt(int fd, uint32_t handle, uint64_t size, unsigned prot);
>   void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
>   void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
> +void *__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot, bool wc);
>   
>   int gem_munmap(void *ptr, uint64_t size);
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev