[3/3] drm/i915: Use CPU mapping for userspace dma-buf mmap()
diff mbox

Message ID 1438812791-5857-4-git-send-email-tiago.vignatti@intel.com
State New
Headers show

Commit Message

Tiago Vignatti Aug. 5, 2015, 10:13 p.m. UTC
Userspace is the one in charge of flush CPU by wrapping mmap with
begin{,end}_cpu_access.

v2: Remove LLC check cause we have dma-buf sync providers now. Also, fix return
before transferring ownership when mmap fails.

Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Chris Wilson Aug. 6, 2015, 9:09 a.m. UTC | #1
On Wed, Aug 05, 2015 at 07:13:11PM -0300, Tiago Vignatti wrote:
> Userspace is the one in charge of flush CPU by wrapping mmap with
> begin{,end}_cpu_access.
> 
> v2: Remove LLC check cause we have dma-buf sync providers now. Also, fix return
> before transferring ownership when mmap fails.

i915 doesn't implement end_cpu_access() as required for your sync ioctl.
 
> Signed-off-by: Tiago Vignatti <tiago.vignatti@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index e9c2bfd..b608f67 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -193,7 +193,23 @@ static void i915_gem_dmabuf_kunmap(struct dma_buf *dma_buf, unsigned long page_n
>  
>  static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
>  {
> -	return -EINVAL;
> +	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
> +	int ret;
> +
> +	if (obj->base.size < vma->vm_end - vma->vm_start)
> +		return -EINVAL;
> +
> +	if (WARN_ON(!obj->base.filp))
> +		return -EINVAL;

So long as Linus is not watching, ENODEV - since we could support it,
but we just have not implemented it yet.

> +	ret = obj->base.filp->f_op->mmap(obj->base.filp, vma);
> +	if (IS_ERR_VALUE(ret))
> +		return -EINVAL;

The return is 0 on success or error otherwise. Always propagate the
original error unless you have good reason not to (in which case you
document it). So

if (ret)
  return ret;

> +
> +	fput(vma->vm_file);
> +	vma->vm_file = get_file(obj->base.filp);
> +
> +	return ret;

return 0;

>  }
>  
>  static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size_t length, enum dma_data_direction direction)
> -- 
> 2.1.0
>
Chris Wilson Aug. 6, 2015, 9:13 a.m. UTC | #2
On Thu, Aug 06, 2015 at 10:09:37AM +0100, Chris Wilson wrote:
> On Wed, Aug 05, 2015 at 07:13:11PM -0300, Tiago Vignatti wrote:
> > Userspace is the one in charge of flush CPU by wrapping mmap with
> > begin{,end}_cpu_access.
> > 
> > v2: Remove LLC check cause we have dma-buf sync providers now. Also, fix return
> > before transferring ownership when mmap fails.
> 
> i915 doesn't implement end_cpu_access() as required for your sync ioctl.

An important point here is that performance on !llc (and for certain
objects with llc) will be absymal, utterly absymal. So what's the
rationale for this compared to doing the opposite and mapping the client
memory (a normal mmap) into the GPU with llc/snooping.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index e9c2bfd..b608f67 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -193,7 +193,23 @@  static void i915_gem_dmabuf_kunmap(struct dma_buf *dma_buf, unsigned long page_n
 
 static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma)
 {
-	return -EINVAL;
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
+	int ret;
+
+	if (obj->base.size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	if (WARN_ON(!obj->base.filp))
+		return -EINVAL;
+
+	ret = obj->base.filp->f_op->mmap(obj->base.filp, vma);
+	if (IS_ERR_VALUE(ret))
+		return -EINVAL;
+
+	fput(vma->vm_file);
+	vma->vm_file = get_file(obj->base.filp);
+
+	return ret;
 }
 
 static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, size_t start, size_t length, enum dma_data_direction direction)