[i-g-t,2/6] lib/ioctl_wrapper: Implement __gem_mmap

Submitted by Lukasz Kalamarz on Jan. 9, 2019, 5:40 p.m.

Details

Message ID 20190109174057.16835-2-lukasz.kalamarz@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Lukasz Kalamarz Jan. 9, 2019, 5:40 p.m.
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(-)

Patch hide | download patch | download mbox

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)
+{
+	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;
 
-	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);
 }
 
 /**
@@ -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);
 	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
 {
 	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);
 

Comments

Daniele Ceraolo Spurio Jan. 9, 2019, 6:04 p.m.
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);
>   
>
Chris Wilson Jan. 9, 2019, 8:40 p.m.
Quoting Lukasz Kalamarz (2019-01-09 17:40:53)
> +void *__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot, bool wc)
> +{
> +       struct drm_i915_gem_mmap arg;
> +
> +       if (wc & !gem_mmap__has_wc(fd)) {

Let's just pass it to the kernel once rather than twice. It only really
made sense if there was an assert later on and we wanted to
differentiate between expected failure and user error.

> +               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;

Then take _flags_.

> +       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);
> +}