Re: [igt-dev] [PATCH i-g-t v3 3/4] lib/ioctl_wrapper: Implement __gem_mmap

["Kalamarz, Lukasz" <lukasz.kalamarz@intel.com>]

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
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev