Re: [PATCH] dma-buf,drm,ion: Propagate error code from dma_buf_start_cpu_access()
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 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 Cc: Tiago Vignatti Cc: Stéphane Marchesin Cc: David Herrmann Cc: Sumit Semwal Cc: Daniel Vetter CC: linux-me...@vger.kernel.org Cc: dri-de...@lists.freedesktop.org Cc: linaro-mm-...@lists.linaro.org Cc: intel-...@lists.freedesktop.org Cc: de...@driverdev.osuosl.org Reviewed-by: Tiago Vignatti Best regards, Tiago ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] dma-buf, drm, ion: Propagate error code from dma_buf_start_cpu_access()
On 19 March 2016 at 15:39, Daniel Vetter wrote: > On Fri, Mar 18, 2016 at 08:02:39PM +, 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 >> 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 >> Cc: Tiago Vignatti >> Cc: Stéphane Marchesin >> Cc: David Herrmann >> Cc: Sumit Semwal >> Cc: Daniel Vetter >> CC: linux-me...@vger.kernel.org >> Cc: dri-de...@lists.freedesktop.org >> Cc: linaro-mm-...@lists.linaro.org >> Cc: intel-...@lists.freedesktop.org >> Cc: de...@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 > >> --- >> 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] dma-buf, drm, ion: Propagate error code from dma_buf_start_cpu_access()
On Fri, Mar 18, 2016 at 08:02:39PM +, 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 > 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 > Cc: Tiago Vignatti > Cc: Stéphane Marchesin > Cc: David Herrmann > Cc: Sumit Semwal > Cc: Daniel Vetter > CC: linux-me...@vger.kernel.org > Cc: dri-de...@lists.freedesktop.org > Cc: linaro-mm-...@lists.linaro.org > Cc: intel-...@lists.freedesktop.org > Cc: de...@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_E