[i-g-t,v3,3/4] lib/ioctl_wrapper: Implement __gem_mmap

Submitted by Lukasz Kalamarz on Jan. 11, 2019, 4:15 p.m.

Details

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

Commit Message

Lukasz Kalamarz Jan. 11, 2019, 4:15 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.
v2: Reordered and splited this patch into two separete patches
v3: Dropped unnecessary check

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>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Patch hide | download patch | download mbox

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index f71f0e32..e9225da0 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -736,6 +736,42 @@  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()
+ * @flags: flags used to determine caching
+ *
+ * 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.
+ */
+static 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;
+
+	memset(&arg, 0, sizeof(arg));
+	arg.handle = handle;
+	arg.offset = offset;
+	arg.size = size;
+	arg.flags = 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);
+}
+
 /**
  * __gem_mmap__wc:
  * @fd: open i915 drm file descriptor

Comments

Daniele Ceraolo Spurio Jan. 11, 2019, 5:39 p.m.
On 01/11/2019 08:15 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.
> v2: Reordered and splited this patch into two separete patches

I think it make more sense for this to be a single patch together with 
the next one. Easier to review as well.

> v3: Dropped unnecessary check
> 
> 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>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index f71f0e32..e9225da0 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -736,6 +736,42 @@ 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()
> + * @flags: flags used to determine caching
> + *
> + * This functions wraps up procedure to establish a memory mapping through
> + * direct cpu access, bypassing the gpu (valid for wc == false). For wc == true

This is a bit unclear IMO, as it sounds like the bypassing the GPU is 
only valid for wc == false, which isn't true since WC also bypasses the 
gpu caches.

Also, instead of using wc == true/false I would say "wc mmappings" and 
"!wc mmappings" or something like that.

Daniele

> + * 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.
> + */
> +static 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;
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.handle = handle;
> +	arg.offset = offset;
> +	arg.size = size;
> +	arg.flags = 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);
> +}
> +
>   /**
>    * __gem_mmap__wc:
>    * @fd: open i915 drm file descriptor
>
Lukasz Kalamarz Jan. 14, 2019, 3:58 p.m.
On Fri, 2019-01-11 at 09:39 -0800, Daniele Ceraolo Spurio wrote:
> 

> On 01/11/2019 08:15 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.

> > v2: Reordered and splited this patch into two separete patches

> 

> I think it make more sense for this to be a single patch together

> with 

> the next one. Easier to review as well.


Will merge those tomorrow then :)

> 

> > v3: Dropped unnecessary check

> > 

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

> > Cc: Chris Wilson <chris@chris-wilson.co.uk>

> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

> > ---

> >   lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++

> >   1 file changed, 36 insertions(+)

> > 

> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c

> > index f71f0e32..e9225da0 100644

> > --- a/lib/ioctl_wrappers.c

> > +++ b/lib/ioctl_wrappers.c

> > @@ -736,6 +736,42 @@ 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()

> > + * @flags: flags used to determine caching

> > + *

> > + * This functions wraps up procedure to establish a memory mapping

> > through

> > + * direct cpu access, bypassing the gpu (valid for wc == false).

> > For wc == true

> 

> This is a bit unclear IMO, as it sounds like the bypassing the GPU

> is 

> only valid for wc == false, which isn't true since WC also bypasses

> the 

> gpu caches.


Will change wording in description. My thought was that word 'also' in
second sentence will connect those two for a case, when wc == true.
----
Lukasz

> 

> Also, instead of using wc == true/false I would say "wc mmappings"

> and 

> "!wc mmappings" or something like that.

> 

> Daniele

> 

> > + * 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.

> > + */

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

> > +

> > +	memset(&arg, 0, sizeof(arg));

> > +	arg.handle = handle;

> > +	arg.offset = offset;

> > +	arg.size = size;

> > +	arg.flags = 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);

> > +}

> > +

> >   /**

> >    * __gem_mmap__wc:

> >    * @fd: open i915 drm file descriptor

> >