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);
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 flushTestcase: 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);
dma_buf_begin_cpu_access(dmabuf, direction);
ret = dma_buf_begin_cpu_access(dmabuf, direction);
return 0;
return ret;@@ -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
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 flushTestcase: 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.
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)
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.
- 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.
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.
- 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)
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
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)
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.
- 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
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.
- 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
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
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
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
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
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
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
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
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
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.
- 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.
*/ int dma_buf_end_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction)
- Can return negative error values, returns 0 on success.
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 flushTestcase: 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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
David Herrmann -
Hans Verkuil -
Sumit Semwal -
Tiago Vignatti