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: Update docs for SYNC ioctl
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 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 Cc: Hans Verkuil Acked-by: Sumit Semwal Signed-off-by: Daniel Vetter Reviewed-by: Tiago Vignatti 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) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] dma-buf: Update docs for SYNC ioctl
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 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] dma-buf: Update docs for SYNC ioctl
On 03/29/2016 06:47 AM, David Herrmann wrote: Hi On Mon, Mar 28, 2016 at 9:42 PM, Tiago Vignatti 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
