[PATCH] dma-buf,drm,ion: Propagate error code from dma_buf_start_cpu_access()

[PATCH] dma-buf,drm,ion: Propagate error code from dma_buf_start_cpu_access()

[Chris Wilson]

Drivers, especially i915.ko, can fail during the initial migration of a
dma-buf for CPU access. However, the error code from the driver was not
being propagated back to ioctl and so userspace was blissfully ignorant
of the failure. Rendering corruption ensues.

Whilst fixing the ioctl to return the error code from
dma_buf_start_cpu_access(), also do the same for
dma_buf_end_cpu_access().  For most drivers, dma_buf_end_cpu_access()
cannot fail. i915.ko however, as most drivers would, wants to avoid being
uninterruptible (as would be required to guarrantee no failure when
flushing the buffer to the device). As userspace already has to handle
errors from the SYNC_IOCTL, take advantage of this to be able to restart
the syscall across signals.

This fixes a coherency issue for i915.ko as well as reducing the
uninterruptible hold upon its BKL, the struct_mutex.

Fixes commit c11e391da2a8fe973c3c2398452000bed505851e
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Feb 11 20:04:51 2016 -0200

    dma-buf: Add ioctls to allow userspace to flush

Testcase: igt/gem_concurrent_blit/*dmabuf*interruptible
Testcase: igt/prime_mmap_coherency/ioctl-errors
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tiago Vignatti <tiago.vignatti@intel.com>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
CC: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: intel-gfx@lists.freedesktop.org
Cc: devel@driverdev.osuosl.org
---
 drivers/dma-buf/dma-buf.c                 | 17 +++++++++++------
 drivers/gpu/drm/i915/i915_gem_dmabuf.c    | 15 +++++----------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  5 +++--
 drivers/gpu/drm/udl/udl_fb.c              |  4 ++--
 drivers/staging/android/ion/ion.c         |  6 ++++--
 include/linux/dma-buf.h                   |  6 +++---
 6 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 9810d1df0691..774a60f4309a 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -259,6 +259,7 @@ static long dma_buf_ioctl(struct file *file,
 	struct dma_buf *dmabuf;
 	struct dma_buf_sync sync;
 	enum dma_data_direction direction;
+	int ret;
 
 	dmabuf = file->private_data;
 
@@ -285,11 +286,11 @@ static long dma_buf_ioctl(struct file *file,
 		}
 
 		if (sync.flags & DMA_BUF_SYNC_END)
-			dma_buf_end_cpu_access(dmabuf, direction);
+			ret = dma_buf_end_cpu_access(dmabuf, direction);
 		else
-			dma_buf_begin_cpu_access(dmabuf, direction);
+			ret = dma_buf_begin_cpu_access(dmabuf, direction);
 
-		return 0;
+		return ret;
 	default:
 		return -ENOTTY;
 	}
@@ -613,13 +614,17 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
  *
  * This call must always succeed.
  */
-void dma_buf_end_cpu_access(struct dma_buf *dmabuf,
-			    enum dma_data_direction direction)
+int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
+			   enum dma_data_direction direction)
 {
+	int ret = 0;
+
 	WARN_ON(!dmabuf);
 
 	if (dmabuf->ops->end_cpu_access)
-		dmabuf->ops->end_cpu_access(dmabuf, direction);
+		ret = dmabuf->ops->end_cpu_access(dmabuf, direction);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 1f3eef6fb345..0506016e18e0 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -228,25 +228,20 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
 	return ret;
 }
 
-static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
+static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
 	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	bool was_interruptible;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
-	was_interruptible = dev_priv->mm.interruptible;
-	dev_priv->mm.interruptible = false;
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, false);
-
-	dev_priv->mm.interruptible = was_interruptible;
 	mutex_unlock(&dev->struct_mutex);
 
-	if (unlikely(ret))
-		DRM_ERROR("unable to flush buffer following CPU access; rendering may be corrupt\n");
+	return ret;
 }
 
 static const struct dma_buf_ops i915_dmabuf_ops =  {
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 3cf8aab23a39..af267c35d813 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -97,11 +97,12 @@ static int omap_gem_dmabuf_begin_cpu_access(struct dma_buf *buffer,
 	return omap_gem_get_pages(obj, &pages, true);
 }
 
-static void omap_gem_dmabuf_end_cpu_access(struct dma_buf *buffer,
-		enum dma_data_direction dir)
+static int omap_gem_dmabuf_end_cpu_access(struct dma_buf *buffer,
+					  enum dma_data_direction dir)
 {
 	struct drm_gem_object *obj = buffer->priv;
 	omap_gem_put_pages(obj);
+	return 0;
 }
 
 
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index c427499133d6..33239a2b264a 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -423,8 +423,8 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	}
 
 	if (ufb->obj->base.import_attach) {
-		dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf,
-				       DMA_FROM_DEVICE);
+		ret = dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf,
+					     DMA_FROM_DEVICE);
 	}
 
  unlock:
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 0754a37c9674..49436b4510f4 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1075,14 +1075,16 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
 	return PTR_ERR_OR_ZERO(vaddr);
 }
 
-static void ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
-				       enum dma_data_direction direction)
+static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
+				      enum dma_data_direction direction)
 {
 	struct ion_buffer *buffer = dmabuf->priv;
 
 	mutex_lock(&buffer->lock);
 	ion_buffer_kmap_put(buffer);
 	mutex_unlock(&buffer->lock);
+
+	return 0;
 }
 
 static struct dma_buf_ops dma_buf_ops = {
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 532108ea0c1c..3fe90d494edb 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -94,7 +94,7 @@ struct dma_buf_ops {
 	void (*release)(struct dma_buf *);
 
 	int (*begin_cpu_access)(struct dma_buf *, enum dma_data_direction);
-	void (*end_cpu_access)(struct dma_buf *, enum dma_data_direction);
+	int (*end_cpu_access)(struct dma_buf *, enum dma_data_direction);
 	void *(*kmap_atomic)(struct dma_buf *, unsigned long);
 	void (*kunmap_atomic)(struct dma_buf *, unsigned long, void *);
 	void *(*kmap)(struct dma_buf *, unsigned long);
@@ -224,8 +224,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
 				enum dma_data_direction);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
 			     enum dma_data_direction dir);
-void dma_buf_end_cpu_access(struct dma_buf *dma_buf,
-			    enum dma_data_direction dir);
+int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
+			   enum dma_data_direction dir);
 void *dma_buf_kmap_atomic(struct dma_buf *, unsigned long);
 void dma_buf_kunmap_atomic(struct dma_buf *, unsigned long, void *);
 void *dma_buf_kmap(struct dma_buf *, unsigned long);
-- 
2.8.0.rc3

Re: [PATCH] dma-buf, drm, ion: Propagate error code from dma_buf_start_cpu_access()

[Daniel Vetter]

On Fri, Mar 18, 2016 at 08:02:39PM +0000, Chris Wilson wrote:
> Drivers, especially i915.ko, can fail during the initial migration of a
> dma-buf for CPU access. However, the error code from the driver was not
> being propagated back to ioctl and so userspace was blissfully ignorant
> of the failure. Rendering corruption ensues.
> 
> Whilst fixing the ioctl to return the error code from
> dma_buf_start_cpu_access(), also do the same for
> dma_buf_end_cpu_access().  For most drivers, dma_buf_end_cpu_access()
> cannot fail. i915.ko however, as most drivers would, wants to avoid being
> uninterruptible (as would be required to guarrantee no failure when
> flushing the buffer to the device). As userspace already has to handle
> errors from the SYNC_IOCTL, take advantage of this to be able to restart
> the syscall across signals.
> 
> This fixes a coherency issue for i915.ko as well as reducing the
> uninterruptible hold upon its BKL, the struct_mutex.
> 
> Fixes commit c11e391da2a8fe973c3c2398452000bed505851e
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Feb 11 20:04:51 2016 -0200
> 
>     dma-buf: Add ioctls to allow userspace to flush
> 
> Testcase: igt/gem_concurrent_blit/*dmabuf*interruptible
> Testcase: igt/prime_mmap_coherency/ioctl-errors
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org

Applied to drm-misc, I'll send a pull to Dave the next few days if no one
screams.
-Daniel

> ---
>  drivers/dma-buf/dma-buf.c                 | 17 +++++++++++------
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c    | 15 +++++----------
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  5 +++--
>  drivers/gpu/drm/udl/udl_fb.c              |  4 ++--
>  drivers/staging/android/ion/ion.c         |  6 ++++--
>  include/linux/dma-buf.h                   |  6 +++---
>  6 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 9810d1df0691..774a60f4309a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -259,6 +259,7 @@ static long dma_buf_ioctl(struct file *file,
>  	struct dma_buf *dmabuf;
>  	struct dma_buf_sync sync;
>  	enum dma_data_direction direction;
> +	int ret;
>  
>  	dmabuf = file->private_data;
>  
> @@ -285,11 +286,11 @@ static long dma_buf_ioctl(struct file *file,
>  		}
>  
>  		if (sync.flags & DMA_BUF_SYNC_END)
> -			dma_buf_end_cpu_access(dmabuf, direction);
> +			ret = dma_buf_end_cpu_access(dmabuf, direction);
>  		else
> -			dma_buf_begin_cpu_access(dmabuf, direction);
> +			ret = dma_buf_begin_cpu_access(dmabuf, direction);
>  
> -		return 0;
> +		return ret;
>  	default:
>  		return -ENOTTY;
>  	}
> @@ -613,13 +614,17 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   *
>   * This call must always succeed.
>   */
> -void dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> -			    enum dma_data_direction direction)
> +int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> +			   enum dma_data_direction direction)
>  {
> +	int ret = 0;
> +
>  	WARN_ON(!dmabuf);
>  
>  	if (dmabuf->ops->end_cpu_access)
> -		dmabuf->ops->end_cpu_access(dmabuf, direction);
> +		ret = dmabuf->ops->end_cpu_access(dmabuf, direction);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 1f3eef6fb345..0506016e18e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -228,25 +228,20 @@ static int i915_gem_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_dire
>  	return ret;
>  }
>  
> -static void i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
> +static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction direction)
>  {
>  	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>  	struct drm_device *dev = obj->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	bool was_interruptible;
>  	int ret;
>  
> -	mutex_lock(&dev->struct_mutex);
> -	was_interruptible = dev_priv->mm.interruptible;
> -	dev_priv->mm.interruptible = false;
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return ret;
>  
>  	ret = i915_gem_object_set_to_gtt_domain(obj, false);
> -
> -	dev_priv->mm.interruptible = was_interruptible;
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	if (unlikely(ret))
> -		DRM_ERROR("unable to flush buffer following CPU access; rendering may be corrupt\n");
> +	return ret;
>  }
>  
>  static const struct dma_buf_ops i915_dmabuf_ops =  {
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> index 3cf8aab23a39..af267c35d813 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> @@ -97,11 +97,12 @@ static int omap_gem_dmabuf_begin_cpu_access(struct dma_buf *buffer,
>  	return omap_gem_get_pages(obj, &pages, true);
>  }
>  
> -static void omap_gem_dmabuf_end_cpu_access(struct dma_buf *buffer,
> -		enum dma_data_direction dir)
> +static int omap_gem_dmabuf_end_cpu_access(struct dma_buf *buffer,
> +					  enum dma_data_direction dir)
>  {
>  	struct drm_gem_object *obj = buffer->priv;
>  	omap_gem_put_pages(obj);
> +	return 0;
>  }
>  
>  
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index c427499133d6..33239a2b264a 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -423,8 +423,8 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  	}
>  
>  	if (ufb->obj->base.import_attach) {
> -		dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf,
> -				       DMA_FROM_DEVICE);
> +		ret = dma_buf_end_cpu_access(ufb->obj->base.import_attach->dmabuf,
> +					     DMA_FROM_DEVICE);
>  	}
>  
>   unlock:
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 0754a37c9674..49436b4510f4 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -1075,14 +1075,16 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
>  	return PTR_ERR_OR_ZERO(vaddr);
>  }
>  
> -static void ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> -				       enum dma_data_direction direction)
> +static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
> +				      enum dma_data_direction direction)
>  {
>  	struct ion_buffer *buffer = dmabuf->priv;
>  
>  	mutex_lock(&buffer->lock);
>  	ion_buffer_kmap_put(buffer);
>  	mutex_unlock(&buffer->lock);
> +
> +	return 0;
>  }
>  
>  static struct dma_buf_ops dma_buf_ops = {
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 532108ea0c1c..3fe90d494edb 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -94,7 +94,7 @@ struct dma_buf_ops {
>  	void (*release)(struct dma_buf *);
>  
>  	int (*begin_cpu_access)(struct dma_buf *, enum dma_data_direction);
> -	void (*end_cpu_access)(struct dma_buf *, enum dma_data_direction);
> +	int (*end_cpu_access)(struct dma_buf *, enum dma_data_direction);
>  	void *(*kmap_atomic)(struct dma_buf *, unsigned long);
>  	void (*kunmap_atomic)(struct dma_buf *, unsigned long, void *);
>  	void *(*kmap)(struct dma_buf *, unsigned long);
> @@ -224,8 +224,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>  				enum dma_data_direction);
>  int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>  			     enum dma_data_direction dir);
> -void dma_buf_end_cpu_access(struct dma_buf *dma_buf,
> -			    enum dma_data_direction dir);
> +int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
> +			   enum dma_data_direction dir);
>  void *dma_buf_kmap_atomic(struct dma_buf *, unsigned long);
>  void dma_buf_kunmap_atomic(struct dma_buf *, unsigned long, void *);
>  void *dma_buf_kmap(struct dma_buf *, unsigned long);
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] dma-buf, drm, ion: Propagate error code from dma_buf_start_cpu_access()

[Sumit Semwal]

On 19 March 2016 at 15:39, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 18, 2016 at 08:02:39PM +0000, Chris Wilson wrote:
>> Drivers, especially i915.ko, can fail during the initial migration of a
>> dma-buf for CPU access. However, the error code from the driver was not
>> being propagated back to ioctl and so userspace was blissfully ignorant
>> of the failure. Rendering corruption ensues.
>>
>> Whilst fixing the ioctl to return the error code from
>> dma_buf_start_cpu_access(), also do the same for
>> dma_buf_end_cpu_access().  For most drivers, dma_buf_end_cpu_access()
>> cannot fail. i915.ko however, as most drivers would, wants to avoid being
>> uninterruptible (as would be required to guarrantee no failure when
>> flushing the buffer to the device). As userspace already has to handle
>> errors from the SYNC_IOCTL, take advantage of this to be able to restart
>> the syscall across signals.
>>
>> This fixes a coherency issue for i915.ko as well as reducing the
>> uninterruptible hold upon its BKL, the struct_mutex.
>>
>> Fixes commit c11e391da2a8fe973c3c2398452000bed505851e
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Date:   Thu Feb 11 20:04:51 2016 -0200
>>
>>     dma-buf: Add ioctls to allow userspace to flush
>>
>> Testcase: igt/gem_concurrent_blit/*dmabuf*interruptible
>> Testcase: igt/prime_mmap_coherency/ioctl-errors
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> CC: linux-media@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: devel@driverdev.osuosl.org
>
> Applied to drm-misc, I'll send a pull to Dave the next few days if no one
> screams.
> -Daniel
Thanks for pulling it via drm-misc, Daniel.
Chris, I feel since this is an API change, it also needs an update to
the Documentation file.
With that, and a minor nit below, please feel free to add
Acked-by: Sumit Semwal <sumit.semwal@linaro.org>

>
>> ---
>>  drivers/dma-buf/dma-buf.c                 | 17 +++++++++++------
>>  drivers/gpu/drm/i915/i915_gem_dmabuf.c    | 15 +++++----------
>>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  5 +++--
>>  drivers/gpu/drm/udl/udl_fb.c              |  4 ++--
>>  drivers/staging/android/ion/ion.c         |  6 ++++--
>>  include/linux/dma-buf.h                   |  6 +++---
>>  6 files changed, 28 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 9810d1df0691..774a60f4309a 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -259,6 +259,7 @@ static long dma_buf_ioctl(struct file *file,
>>       struct dma_buf *dmabuf;
>>       struct dma_buf_sync sync;
>>       enum dma_data_direction direction;
>> +     int ret;
>>
>>       dmabuf = file->private_data;
>>
>> @@ -285,11 +286,11 @@ static long dma_buf_ioctl(struct file *file,
>>               }
>>
>>               if (sync.flags & DMA_BUF_SYNC_END)
>> -                     dma_buf_end_cpu_access(dmabuf, direction);
>> +                     ret = dma_buf_end_cpu_access(dmabuf, direction);
>>               else
>> -                     dma_buf_begin_cpu_access(dmabuf, direction);
>> +                     ret = dma_buf_begin_cpu_access(dmabuf, direction);
>>
>> -             return 0;
>> +             return ret;
>>       default:
>>               return -ENOTTY;
>>       }
>> @@ -613,13 +614,17 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>>   *
>>   * This call must always succeed.
>>   */
Perhaps update the above comment to reflect the change as well?

>> -void dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>> -                         enum dma_data_direction direction)
>> +int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>> +                        enum dma_data_direction direction)
>>  {
>> +     int ret = 0;
>> +
>>       WARN_ON(!dmabuf);
>>
>>       if (dmabuf->ops->end_cpu_access)
>> -             dmabuf->ops->end_cpu_access(dmabuf, direction);
>> +             ret = dmabuf->ops->end_cpu_access(dmabuf, direction);
>> +
>> +     return ret;
>>  }
<< snip>>

Best regards,
Sumit.

[PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

Just a bit of wording polish plus mentioning that it can fail and must
be restarted.

Requested by Sumit.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tiago Vignatti <tiago.vignatti@intel.com>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
CC: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: intel-gfx@lists.freedesktop.org
Cc: devel@driverdev.osuosl.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/dma-buf-sharing.txt | 11 ++++++-----
 drivers/dma-buf/dma-buf.c         |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
index 32ac32e773e1..5c4e3e586ec8 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
 
    No special interfaces, userspace simply calls mmap on the dma-buf fd, making
    sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
-   used when the access happens. This is discussed next paragraphs.
+   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
+   -EGAIN or -EINTR, in which case it must be restarted.
 
    Some systems might need some sort of cache coherency management e.g. when
    CPU and GPU domains are being accessed through dma-buf at the same time. To
@@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
        want (with the new data being consumed by the GPU or say scanout device)
      - munmap once you don't need the buffer any more
 
-    Therefore, for correctness and optimal performance, systems with the memory
-    cache shared by the GPU and CPU i.e. the "coherent" and also the
-    "incoherent" are always required to use SYNC_START and SYNC_END before and
-    after, respectively, when accessing the mapped address.
+    For correctness and optimal performance, it is always required to use
+    SYNC_START and SYNC_END before and after, respectively, when accessing the
+    mapped address. Userspace cannot on coherent access, even when there are
+    systems where it just works without calling these ioctls.
 
 2. Supporting existing mmap interfaces in importers
 
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 774a60f4309a..4a2c07ee6677 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
  * @dmabuf:	[in]	buffer to complete cpu access for.
  * @direction:	[in]	length of range for cpu access.
  *
- * This call must always succeed.
+ * Can return negative error values, returns 0 on success.
  */
 int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
 			   enum dma_data_direction direction)
-- 
2.8.0.rc3

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Sumit Semwal]

On 21 March 2016 at 13:00, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Sumit Semwal <sumit.semwal@linaro.org>

> ---
>  Documentation/dma-buf-sharing.txt | 11 ++++++-----
>  drivers/dma-buf/dma-buf.c         |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..5c4e3e586ec8 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>
>     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EGAIN or -EINTR, in which case it must be restarted.
>
>     Some systems might need some sort of cache coherency management e.g. when
>     CPU and GPU domains are being accessed through dma-buf at the same time. To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>         want (with the new data being consumed by the GPU or say scanout device)
>       - munmap once you don't need the buffer any more
>
> -    Therefore, for correctness and optimal performance, systems with the memory
> -    cache shared by the GPU and CPU i.e. the "coherent" and also the
> -    "incoherent" are always required to use SYNC_START and SYNC_END before and
> -    after, respectively, when accessing the mapped address.
> +    For correctness and optimal performance, it is always required to use
> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
> +    mapped address. Userspace cannot on coherent access, even when there are
> +    systems where it just works without calling these ioctls.
>
>  2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:    [in]    buffer to complete cpu access for.
>   * @direction: [in]    length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>                            enum dma_data_direction direction)
> --
> 2.8.0.rc3
>

Best regards,
Sumit.

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Hans Verkuil]

Hi Daniel,

Two small comments:

On 03/21/2016 08:30 AM, Daniel Vetter wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
> 
> Requested by Sumit.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++++++-----
>  drivers/dma-buf/dma-buf.c         |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..5c4e3e586ec8 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>  
>     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EGAIN or -EINTR, in which case it must be restarted.

EGAIN -> EAGAIN

>  
>     Some systems might need some sort of cache coherency management e.g. when
>     CPU and GPU domains are being accessed through dma-buf at the same time. To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>         want (with the new data being consumed by the GPU or say scanout device)
>       - munmap once you don't need the buffer any more
>  
> -    Therefore, for correctness and optimal performance, systems with the memory
> -    cache shared by the GPU and CPU i.e. the "coherent" and also the
> -    "incoherent" are always required to use SYNC_START and SYNC_END before and
> -    after, respectively, when accessing the mapped address.
> +    For correctness and optimal performance, it is always required to use
> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
> +    mapped address. Userspace cannot on coherent access, even when there are

"Userspace cannot on coherent access"? Do you mean "cannot do"? Sorry, the
meaning isn't clear to me.

Regards,

	Hans

> +    systems where it just works without calling these ioctls.
>  
>  2. Supporting existing mmap interfaces in importers
>  
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:	[in]	buffer to complete cpu access for.
>   * @direction:	[in]	length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>  			   enum dma_data_direction direction)
> 

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

On Mon, Mar 21, 2016 at 8:40 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> +    For correctness and optimal performance, it is always required to use
>> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
>> +    mapped address. Userspace cannot on coherent access, even when there are
>
> "Userspace cannot on coherent access"? Do you mean "cannot do"? Sorry, the
> meaning isn't clear to me.

"cannot rely on". I'll send out v2 asap (and let's hope the coffee
works this time around).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

Just a bit of wording polish plus mentioning that it can fail and must
be restarted.

Requested by Sumit.

v2: Fix them typos (Hans).

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tiago Vignatti <tiago.vignatti@intel.com>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>
CC: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: intel-gfx@lists.freedesktop.org
Cc: devel@driverdev.osuosl.org
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/dma-buf-sharing.txt | 11 ++++++-----
 drivers/dma-buf/dma-buf.c         |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
index 32ac32e773e1..ca44c5820585 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
 
    No special interfaces, userspace simply calls mmap on the dma-buf fd, making
    sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
-   used when the access happens. This is discussed next paragraphs.
+   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
+   -EAGAIN or -EINTR, in which case it must be restarted.
 
    Some systems might need some sort of cache coherency management e.g. when
    CPU and GPU domains are being accessed through dma-buf at the same time. To
@@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
        want (with the new data being consumed by the GPU or say scanout device)
      - munmap once you don't need the buffer any more
 
-    Therefore, for correctness and optimal performance, systems with the memory
-    cache shared by the GPU and CPU i.e. the "coherent" and also the
-    "incoherent" are always required to use SYNC_START and SYNC_END before and
-    after, respectively, when accessing the mapped address.
+    For correctness and optimal performance, it is always required to use
+    SYNC_START and SYNC_END before and after, respectively, when accessing the
+    mapped address. Userspace cannot rely on coherent access, even when there
+    are systems where it just works without calling these ioctls.
 
 2. Supporting existing mmap interfaces in importers
 
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 774a60f4309a..4a2c07ee6677 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
  * @dmabuf:	[in]	buffer to complete cpu access for.
  * @direction:	[in]	length of range for cpu access.
  *
- * This call must always succeed.
+ * Can return negative error values, returns 0 on success.
  */
 int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
 			   enum dma_data_direction direction)
-- 
2.8.0.rc3

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Hans Verkuil]

On 03/21/2016 08:51 AM, Daniel Vetter wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
> 
> Requested by Sumit.
> 
> v2: Fix them typos (Hans).
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++++++-----
>  drivers/dma-buf/dma-buf.c         |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>  
>     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.
>  
>     Some systems might need some sort of cache coherency management e.g. when
>     CPU and GPU domains are being accessed through dma-buf at the same time. To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>         want (with the new data being consumed by the GPU or say scanout device)
>       - munmap once you don't need the buffer any more
>  
> -    Therefore, for correctness and optimal performance, systems with the memory
> -    cache shared by the GPU and CPU i.e. the "coherent" and also the
> -    "incoherent" are always required to use SYNC_START and SYNC_END before and
> -    after, respectively, when accessing the mapped address.
> +    For correctness and optimal performance, it is always required to use
> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
> +    mapped address. Userspace cannot rely on coherent access, even when there
> +    are systems where it just works without calling these ioctls.
>  
>  2. Supporting existing mmap interfaces in importers
>  
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:	[in]	buffer to complete cpu access for.
>   * @direction:	[in]	length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>  			   enum dma_data_direction direction)
> 

Much better :-)

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> v2: Fix them typos (Hans).
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/dma-buf-sharing.txt | 11 ++++++-----
>  drivers/dma-buf/dma-buf.c         |  2 +-
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>
>     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.

What is "restart on EAGAIN" supposed to mean? Or more generally, what
does EAGAIN tell the caller?

Thanks
David

>     Some systems might need some sort of cache coherency management e.g. when
>     CPU and GPU domains are being accessed through dma-buf at the same time. To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>         want (with the new data being consumed by the GPU or say scanout device)
>       - munmap once you don't need the buffer any more
>
> -    Therefore, for correctness and optimal performance, systems with the memory
> -    cache shared by the GPU and CPU i.e. the "coherent" and also the
> -    "incoherent" are always required to use SYNC_START and SYNC_END before and
> -    after, respectively, when accessing the mapped address.
> +    For correctness and optimal performance, it is always required to use
> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
> +    mapped address. Userspace cannot rely on coherent access, even when there
> +    are systems where it just works without calling these ioctls.
>
>  2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>   * @dmabuf:    [in]    buffer to complete cpu access for.
>   * @direction: [in]    length of range for cpu access.
>   *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>   */
>  int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>                            enum dma_data_direction direction)
> --
> 2.8.0.rc3
>

Re: [PATCH] dma-buf,drm,ion: Propagate error code from dma_buf_start_cpu_access()

[Tiago Vignatti]

On 03/18/2016 05:02 PM, Chris Wilson wrote:
> Drivers, especially i915.ko, can fail during the initial migration of a
> dma-buf for CPU access. However, the error code from the driver was not
> being propagated back to ioctl and so userspace was blissfully ignorant
> of the failure. Rendering corruption ensues.
>
> Whilst fixing the ioctl to return the error code from
> dma_buf_start_cpu_access(), also do the same for
> dma_buf_end_cpu_access().  For most drivers, dma_buf_end_cpu_access()
> cannot fail. i915.ko however, as most drivers would, wants to avoid being
> uninterruptible (as would be required to guarrantee no failure when
> flushing the buffer to the device). As userspace already has to handle
> errors from the SYNC_IOCTL, take advantage of this to be able to restart
> the syscall across signals.
>
> This fixes a coherency issue for i915.ko as well as reducing the
> uninterruptible hold upon its BKL, the struct_mutex.
>
> Fixes commit c11e391da2a8fe973c3c2398452000bed505851e
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Feb 11 20:04:51 2016 -0200
>
>      dma-buf: Add ioctls to allow userspace to flush
>
> Testcase: igt/gem_concurrent_blit/*dmabuf*interruptible
> Testcase: igt/prime_mmap_coherency/ioctl-errors
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org

Reviewed-by: Tiago Vignatti <tiago.vignatti@intel.com>

Best regards,

Tiago

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/21/2016 04:51 AM, Daniel Vetter wrote:
> Just a bit of wording polish plus mentioning that it can fail and must
> be restarted.
>
> Requested by Sumit.
>
> v2: Fix them typos (Hans).
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> CC: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: devel@driverdev.osuosl.org
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Tiago Vignatti <tiago.vignatti@intel.com>

Best regards,

Tiago


> ---
>   Documentation/dma-buf-sharing.txt | 11 ++++++-----
>   drivers/dma-buf/dma-buf.c         |  2 +-
>   2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> index 32ac32e773e1..ca44c5820585 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>
>      No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>      sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> -   used when the access happens. This is discussed next paragraphs.
> +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> +   -EAGAIN or -EINTR, in which case it must be restarted.
>
>      Some systems might need some sort of cache coherency management e.g. when
>      CPU and GPU domains are being accessed through dma-buf at the same time. To
> @@ -366,10 +367,10 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>          want (with the new data being consumed by the GPU or say scanout device)
>        - munmap once you don't need the buffer any more
>
> -    Therefore, for correctness and optimal performance, systems with the memory
> -    cache shared by the GPU and CPU i.e. the "coherent" and also the
> -    "incoherent" are always required to use SYNC_START and SYNC_END before and
> -    after, respectively, when accessing the mapped address.
> +    For correctness and optimal performance, it is always required to use
> +    SYNC_START and SYNC_END before and after, respectively, when accessing the
> +    mapped address. Userspace cannot rely on coherent access, even when there
> +    are systems where it just works without calling these ioctls.
>
>   2. Supporting existing mmap interfaces in importers
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 774a60f4309a..4a2c07ee6677 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -612,7 +612,7 @@ EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access);
>    * @dmabuf:	[in]	buffer to complete cpu access for.
>    * @direction:	[in]	length of range for cpu access.
>    *
> - * This call must always succeed.
> + * Can return negative error values, returns 0 on success.
>    */
>   int dma_buf_end_cpu_access(struct dma_buf *dmabuf,
>   			   enum dma_data_direction direction)
>

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Daniel Vetter]

On Mon, Mar 21, 2016 at 01:26:58PM +0100, David Herrmann wrote:
> Hi
> 
> On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Just a bit of wording polish plus mentioning that it can fail and must
> > be restarted.
> >
> > Requested by Sumit.
> >
> > v2: Fix them typos (Hans).
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tiago Vignatti <tiago.vignatti@intel.com>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > CC: linux-media@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linaro-mm-sig@lists.linaro.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: devel@driverdev.osuosl.org
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/dma-buf-sharing.txt | 11 ++++++-----
> >  drivers/dma-buf/dma-buf.c         |  2 +-
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
> > index 32ac32e773e1..ca44c5820585 100644
> > --- a/Documentation/dma-buf-sharing.txt
> > +++ b/Documentation/dma-buf-sharing.txt
> > @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
> >
> >     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
> >     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
> > -   used when the access happens. This is discussed next paragraphs.
> > +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
> > +   -EAGAIN or -EINTR, in which case it must be restarted.
> 
> What is "restart on EAGAIN" supposed to mean? Or more generally, what
> does EAGAIN tell the caller?

Do what drmIoctl does essentially.

while (ret == -1 && (errno == EAGAIN || errno == EINTR)
	ret = ioctl();

Typed from memery, too lazy to look it up in the source ;-) I'm trying to
sell the idea of a real dma-buf manpage to Sumit, we should clarify this
in detail there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hey

On Mon, Mar 21, 2016 at 6:14 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Mar 21, 2016 at 01:26:58PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Mon, Mar 21, 2016 at 8:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > Just a bit of wording polish plus mentioning that it can fail and must
>> > be restarted.
>> >
>> > Requested by Sumit.
>> >
>> > v2: Fix them typos (Hans).
>> >
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Tiago Vignatti <tiago.vignatti@intel.com>
>> > Cc: Stéphane Marchesin <marcheu@chromium.org>
>> > Cc: David Herrmann <dh.herrmann@gmail.com>
>> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> > Cc: Daniel Vetter <daniel.vetter@intel.com>
>> > CC: linux-media@vger.kernel.org
>> > Cc: dri-devel@lists.freedesktop.org
>> > Cc: linaro-mm-sig@lists.linaro.org
>> > Cc: intel-gfx@lists.freedesktop.org
>> > Cc: devel@driverdev.osuosl.org
>> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
>> > Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
>> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> > ---
>> >  Documentation/dma-buf-sharing.txt | 11 ++++++-----
>> >  drivers/dma-buf/dma-buf.c         |  2 +-
>> >  2 files changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/Documentation/dma-buf-sharing.txt b/Documentation/dma-buf-sharing.txt
>> > index 32ac32e773e1..ca44c5820585 100644
>> > --- a/Documentation/dma-buf-sharing.txt
>> > +++ b/Documentation/dma-buf-sharing.txt
>> > @@ -352,7 +352,8 @@ Being able to mmap an export dma-buf buffer object has 2 main use-cases:
>> >
>> >     No special interfaces, userspace simply calls mmap on the dma-buf fd, making
>> >     sure that the cache synchronization ioctl (DMA_BUF_IOCTL_SYNC) is *always*
>> > -   used when the access happens. This is discussed next paragraphs.
>> > +   used when the access happens. Note that DMA_BUF_IOCTL_SYNC can fail with
>> > +   -EAGAIN or -EINTR, in which case it must be restarted.
>>
>> What is "restart on EAGAIN" supposed to mean? Or more generally, what
>> does EAGAIN tell the caller?
>
> Do what drmIoctl does essentially.
>
> while (ret == -1 && (errno == EAGAIN || errno == EINTR)
>         ret = ioctl();
>
> Typed from memery, too lazy to look it up in the source ;-) I'm trying to
> sell the idea of a real dma-buf manpage to Sumit, we should clarify this
> in detail there.

My question was rather about why we do this? Semantics for EINTR are
well defined, and with SA_RESTART (default on linux) user-space can
ignore it. However, looping on EAGAIN is very uncommon, and it is not
at all clear why it is needed?

Returning an error to user-space makes sense if user-space has a
reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
operation helps user-space at all? As someone without insight into the
driver implementation, it is hard to tell why.. Any hints?

Thanks
David

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Chris Wilson]

On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
> My question was rather about why we do this? Semantics for EINTR are
> well defined, and with SA_RESTART (default on linux) user-space can
> ignore it. However, looping on EAGAIN is very uncommon, and it is not
> at all clear why it is needed?
> 
> Returning an error to user-space makes sense if user-space has a
> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
> operation helps user-space at all? As someone without insight into the
> driver implementation, it is hard to tell why.. Any hints?

The reason we return EAGAIN is to workaround a deadlock we face when
blocking on the GPU holding the struct_mutex (inside the client's
process), but the GPU is dead. As our locking is very, very coarse we
cannot restart the GPU without acquiring the struct_mutex being held by
the client so we wake the client up and tell them the resource they are
waiting on (the flush of the object from the GPU into the CPU domain) is
temporarily unavailable. If they try to immediately wait upon the ioctl
again, they are blocked waiting for the reset to occur before they may
complete their flush. There are a few other possible deadlocks that are
also avoided with EAGAIN (again, the issue is more or less the lack of
fine grained locking).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
>> My question was rather about why we do this? Semantics for EINTR are
>> well defined, and with SA_RESTART (default on linux) user-space can
>> ignore it. However, looping on EAGAIN is very uncommon, and it is not
>> at all clear why it is needed?
>>
>> Returning an error to user-space makes sense if user-space has a
>> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
>> operation helps user-space at all? As someone without insight into the
>> driver implementation, it is hard to tell why.. Any hints?
>
> The reason we return EAGAIN is to workaround a deadlock we face when
> blocking on the GPU holding the struct_mutex (inside the client's
> process), but the GPU is dead. As our locking is very, very coarse we
> cannot restart the GPU without acquiring the struct_mutex being held by
> the client so we wake the client up and tell them the resource they are
> waiting on (the flush of the object from the GPU into the CPU domain) is
> temporarily unavailable. If they try to immediately wait upon the ioctl
> again, they are blocked waiting for the reset to occur before they may
> complete their flush. There are a few other possible deadlocks that are
> also avoided with EAGAIN (again, the issue is more or less the lack of
> fine grained locking).

...so you hijacked EAGAIN for all DRM ioctls just for a driver
workaround? EAGAIN is universally used to signal the caller about a
blocking resource. It is very much linked to O_NONBLOCK. Why not use
EBUSY, ECANCELED, ECOMM, EDEADLOCK, EIO, EL3RST, ...

Anyhow, I guess that ship has sailed. But just mentioning EAGAIN in a
kernel-doc is way to vague for user-space to figure out they should
loop on it.

Thanks
David

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Chris Wilson]

On Wed, Mar 23, 2016 at 04:32:59PM +0100, David Herrmann wrote:
> Hi
> 
> On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
> >> My question was rather about why we do this? Semantics for EINTR are
> >> well defined, and with SA_RESTART (default on linux) user-space can
> >> ignore it. However, looping on EAGAIN is very uncommon, and it is not
> >> at all clear why it is needed?
> >>
> >> Returning an error to user-space makes sense if user-space has a
> >> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
> >> operation helps user-space at all? As someone without insight into the
> >> driver implementation, it is hard to tell why.. Any hints?
> >
> > The reason we return EAGAIN is to workaround a deadlock we face when
> > blocking on the GPU holding the struct_mutex (inside the client's
> > process), but the GPU is dead. As our locking is very, very coarse we
> > cannot restart the GPU without acquiring the struct_mutex being held by
> > the client so we wake the client up and tell them the resource they are
> > waiting on (the flush of the object from the GPU into the CPU domain) is
> > temporarily unavailable. If they try to immediately wait upon the ioctl
> > again, they are blocked waiting for the reset to occur before they may
> > complete their flush. There are a few other possible deadlocks that are
> > also avoided with EAGAIN (again, the issue is more or less the lack of
> > fine grained locking).
> 
> ...so you hijacked EAGAIN for all DRM ioctls just for a driver
> workaround?

No, we utilized the fact that EAGAIN was already enshrined by libdrm as
the defacto mechanism for repeating the ioctl in order to repeat the
ioctl for a driver workaround.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/23/2016 12:42 PM, Chris Wilson wrote:
> On Wed, Mar 23, 2016 at 04:32:59PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Wed, Mar 23, 2016 at 12:56 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>> On Wed, Mar 23, 2016 at 12:30:42PM +0100, David Herrmann wrote:
>>>> My question was rather about why we do this? Semantics for EINTR are
>>>> well defined, and with SA_RESTART (default on linux) user-space can
>>>> ignore it. However, looping on EAGAIN is very uncommon, and it is not
>>>> at all clear why it is needed?
>>>>
>>>> Returning an error to user-space makes sense if user-space has a
>>>> reason to react to it. I fail to see how EAGAIN on a cache-flush/sync
>>>> operation helps user-space at all? As someone without insight into the
>>>> driver implementation, it is hard to tell why.. Any hints?
>>>
>>> The reason we return EAGAIN is to workaround a deadlock we face when
>>> blocking on the GPU holding the struct_mutex (inside the client's
>>> process), but the GPU is dead. As our locking is very, very coarse we
>>> cannot restart the GPU without acquiring the struct_mutex being held by
>>> the client so we wake the client up and tell them the resource they are
>>> waiting on (the flush of the object from the GPU into the CPU domain) is
>>> temporarily unavailable. If they try to immediately wait upon the ioctl
>>> again, they are blocked waiting for the reset to occur before they may
>>> complete their flush. There are a few other possible deadlocks that are
>>> also avoided with EAGAIN (again, the issue is more or less the lack of
>>> fine grained locking).
>>
>> ...so you hijacked EAGAIN for all DRM ioctls just for a driver
>> workaround?
>
> No, we utilized the fact that EAGAIN was already enshrined by libdrm as
> the defacto mechanism for repeating the ioctl in order to repeat the
> ioctl for a driver workaround.

Do we have an agreement here after all? David? I need to know whether 
this fixup is okay to go cause I'll need to submit to Chrome OS then.

Best Regards,

Tiago

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[David Herrmann]

Hi

On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
<tiago.vignatti@intel.com> wrote:
> Do we have an agreement here after all? David? I need to know whether this
> fixup is okay to go cause I'll need to submit to Chrome OS then.

Sure it is fine. The code is already there, we cannot change it.

Thanks
David

Re: [PATCH] dma-buf: Update docs for SYNC ioctl

[Tiago Vignatti]

On 03/29/2016 06:47 AM, David Herrmann wrote:
> Hi
>
> On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti
> <tiago.vignatti@intel.com> wrote:
>> Do we have an agreement here after all? David? I need to know whether this
>> fixup is okay to go cause I'll need to submit to Chrome OS then.
>
> Sure it is fine. The code is already there, we cannot change it.

ah true. Only now that I've noticed it's already in Linus tree. Thanks 
anyway!

Tiago