Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
3025 discussions

Re: [Linaro-mm-sig] [PATCH] staging: android: ion: Fix dma direction for dma_sync_sg_for_cpu/device

by Laura Abbott

On 12/15/2017 12:59 PM, Sushmita Susheelendra wrote: > Use the direction argument passed into begin_cpu_access > and end_cpu_access when calling the dma_sync_sg_for_cpu/device. > The actual cache primitive called depends on the direction > passed in. > Acked-by: Laura Abbott <labbott(a)redhat.com> > Signed-off-by: Sushmita Susheelendra <ssusheel(a)codeaurora.org> > --- > drivers/staging/android/ion/ion.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index a7d9b0e..f480885 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -346,7 +346,7 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents, > - DMA_BIDIRECTIONAL); > + direction); > } > mutex_unlock(&buffer->lock); > > @@ -368,7 +368,7 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > mutex_lock(&buffer->lock); > list_for_each_entry(a, &buffer->attachments, list) { > dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents, > - DMA_BIDIRECTIONAL); > + direction); > } > mutex_unlock(&buffer->lock); > >

7 years, 8 months

Re: [Linaro-mm-sig] [PATCH 0/4] Backported amdgpu ttm deadlock fixes for 4.14

by Christian König

Am 01.12.2017 um 01:23 schrieb Lyude Paul: > I haven't gone to see where it started, but as of late a good number of > pretty nasty deadlock issues have appeared with the kernel. Easy > reproduction recipe on a laptop with i915/amdgpu prime with lockdep enabled: > > DRI_PRIME=1 glxinfo Acked-by: Christian König <christian.koenig(a)amd.com> Thanks for taking care of this, Christian. > > Additionally, some more race conditions exist that I've managed to > trigger with piglit and lockdep enabled after applying these patches: > > ============================= > WARNING: suspicious RCU usage > 4.14.3Lyude-Test+ #2 Not tainted > ----------------------------- > ./include/linux/reservation.h:216 suspicious rcu_dereference_protected() usage! > > other info that might help us debug this: > > rcu_scheduler_active = 2, debug_locks = 1 > 1 lock held by ext_image_dma_b/27451: > #0: (reservation_ww_class_mutex){+.+.}, at: [<ffffffffa034f2ff>] ttm_bo_unref+0x9f/0x3c0 [ttm] > > stack backtrace: > CPU: 0 PID: 27451 Comm: ext_image_dma_b Not tainted 4.14.3Lyude-Test+ #2 > Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.02 06/09/2017 > Call Trace: > dump_stack+0x8e/0xce > lockdep_rcu_suspicious+0xc5/0x100 > reservation_object_copy_fences+0x292/0x2b0 > ? ttm_bo_unref+0x9f/0x3c0 [ttm] > ttm_bo_unref+0xbd/0x3c0 [ttm] > amdgpu_bo_unref+0x2a/0x50 [amdgpu] > amdgpu_gem_object_free+0x4b/0x50 [amdgpu] > drm_gem_object_free+0x1f/0x40 [drm] > drm_gem_object_put_unlocked+0x40/0xb0 [drm] > drm_gem_object_handle_put_unlocked+0x6c/0xb0 [drm] > drm_gem_object_release_handle+0x51/0x90 [drm] > drm_gem_handle_delete+0x5e/0x90 [drm] > ? drm_gem_handle_create+0x40/0x40 [drm] > drm_gem_close_ioctl+0x20/0x30 [drm] > drm_ioctl_kernel+0x5d/0xb0 [drm] > drm_ioctl+0x2f7/0x3b0 [drm] > ? drm_gem_handle_create+0x40/0x40 [drm] > ? trace_hardirqs_on_caller+0xf4/0x190 > ? trace_hardirqs_on+0xd/0x10 > amdgpu_drm_ioctl+0x4f/0x90 [amdgpu] > do_vfs_ioctl+0x93/0x670 > ? __fget+0x108/0x1f0 > SyS_ioctl+0x79/0x90 > entry_SYSCALL_64_fastpath+0x23/0xc2 > > I've also added the relevant fixes for the issue mentioned above. > > Christian König (3): > drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more > dma-buf: make reservation_object_copy_fences rcu save > drm/amdgpu: reserve root PD while releasing it > > Michel Dänzer (1): > drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list > > drivers/dma-buf/reservation.c | 56 +++++++++++++++++++++++++--------- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++-- > drivers/gpu/drm/ttm/ttm_bo.c | 43 +++++++++++++------------- > 3 files changed, 74 insertions(+), 38 deletions(-) > > -- > 2.14.3 > > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

7 years, 9 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Fix ifnullfree.cocci warnings

by Sumit Semwal

Hello Vasyl, On 22 November 2017 at 20:52, Vasyl Gomonovych <gomonovych(a)gmail.com> wrote: > NULL check before some freeing functions is not needed. > drivers/dma-buf/dma-buf.c:1183:2-26: WARNING: NULL check before freeing debugfs_remove_recursive > Generated by: scripts/coccinelle/free/ifnullfree.cocci Thank you for your patch; applied to drm-misc-next. > > Signed-off-by: Vasyl Gomonovych <gomonovych(a)gmail.com> > --- > drivers/dma-buf/dma-buf.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 4a038dcf5361..048827b06c03 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1179,8 +1179,7 @@ static int dma_buf_init_debugfs(void) > > static void dma_buf_uninit_debugfs(void) > { > - if (dma_buf_debugfs_dir) > - debugfs_remove_recursive(dma_buf_debugfs_dir); > + debugfs_remove_recursive(dma_buf_debugfs_dir); > } > #else > static inline int dma_buf_init_debugfs(void) > -- > 1.9.1 > -- Thanks and regards, Sumit Semwal Linaro Mobile Group - Kernel Team Lead Linaro.org │ Open source software for ARM SoCs

7 years, 9 months

[PATCH] reservation: don't wait when timeout=0

by Rob Clark

If we are testing if a reservation object's fences have been signaled with timeout=0 (non-blocking), we need to pass 0 for timeout to dma_fence_wait_timeout(). Plus bonus spelling correction. Signed-off-by: Rob Clark <robdclark(a)gmail.com> --- drivers/dma-buf/reservation.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index dec3a815455d..71f51140a9ad 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); * * RETURNS * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or - * greater than zer on success. + * greater than zero on success. */ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, bool wait_all, bool intr, @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, goto retry; } - ret = dma_fence_wait_timeout(fence, intr, ret); + /* + * Note that dma_fence_wait_timeout() will return 1 if + * the fence is already signaled, so in the wait_all + * case when we go through the retry loop again, ret + * will be greater than 0 and we don't want this to + * cause _wait_timeout() to block + */ + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); dma_fence_put(fence); if (ret > 0 && wait_all && (i + 1 < shared_count)) goto retry; -- 2.13.6

7 years, 9 months

[PATCH 0/4] dma-buf: Silence dma_fence __rcu sparse warnings

by Ville Syrjala

From: Ville Syrjälä <ville.syrjala(a)linux.intel.com> When building drm+i915 I get around 150 lines of sparse noise from dma_fence __rcu warnings. This series eliminates all of that. The first two patches were already posted by Chris, but there wasn't any real reaction, so I figured I'd repost with a wider Cc list. As for the other two patches, I'm no expert on dma_fence and I didn't spend a lot of time looking at it so I can't be sure I annotated all the accesses correctly. But I figured someone will scream at me if I got it wrong ;) Cc: Dave Airlie <airlied(a)redhat.com> Cc: Jason Ekstrand <jason(a)jlekstrand.net> Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-media(a)vger.kernel.org Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Chris Wilson (2): drm/syncobj: Mark up the fence as an RCU protected pointer dma-buf/fence: Sparse wants __rcu on the object itself Ville Syrjälä (2): drm/syncobj: Use proper methods for accessing rcu protected pointers dma-buf: Use rcu_assign_pointer() to set rcu protected pointers drivers/dma-buf/reservation.c | 2 +- drivers/gpu/drm/drm_syncobj.c | 11 +++++++---- include/drm/drm_syncobj.h | 2 +- include/linux/dma-fence.h | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) -- 2.13.6

7 years, 10 months

Re: [Linaro-mm-sig] [PATCH] dma-fence: fix dma_fence_get_rcu_safe

by Chris Wilson

Quoting Christian König (2017-09-04 14:27:33) > From: Christian König <christian.koenig(a)amd.com> > > The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to > acquire a reference it doesn't necessary mean that there is no fence at all. > > It usually mean that the fence was replaced by a new one and in this situation > we certainly want to have the new one as result and *NOT* NULL. Which is not guaranteed by the code you wrote either. The point of the comment is that the mb is only inside the successful kref_atomic_inc_unless_zero, and that only after that mb do you know whether or not you have the current fence. You can argue that you want to replace the if (!dma_fence_get_rcu()) return NULL with if (!dma_fence_get_rcu() continue; but it would be incorrect to say that by simply ignoring the post-condition check that you do have the right fence. -Chris

7 years, 11 months

[PATCH] dma-fence: fix dma_fence_get_rcu_safe v2

by Christian König

From: Christian König <christian.koenig(a)amd.com> When dma_fence_get_rcu() fails to acquire a reference it doesn't necessary mean that there is no fence at all. It usually mean that the fence was replaced by a new one and in this situation we certainly want to have the new one as result and *NOT* NULL. v2: Keep extra check after dma_fence_get_rcu(). Signed-off-by: Christian König <christian.koenig(a)amd.com> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org --- include/linux/dma-fence.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 0a186c4..f4f23cb 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -248,9 +248,12 @@ dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep) struct dma_fence *fence; fence = rcu_dereference(*fencep); - if (!fence || !dma_fence_get_rcu(fence)) + if (!fence) return NULL; + if (!dma_fence_get_rcu(fence)) + continue; + /* The atomic_inc_not_zero() inside dma_fence_get_rcu() * provides a full memory barrier upon success (such as now). * This is paired with the write barrier from assigning -- 2.7.4

7 years, 11 months

[PATCH] dma-buf: remove redundant initialization of sg_table

by Colin King

From: Colin Ian King <colin.king(a)canonical.com> sg_table is being initialized and is never read before it is updated again later on, hence making the initialization redundant. Remove the initialization. Detected by clang scan-build: "warning: Value stored to 'sg_table' during its initialization is never read" Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- drivers/dma-buf/dma-buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4a038dcf5361..bc1cb284111c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -625,7 +625,7 @@ EXPORT_SYMBOL_GPL(dma_buf_detach); struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, enum dma_data_direction direction) { - struct sg_table *sg_table = ERR_PTR(-EINVAL); + struct sg_table *sg_table; might_sleep(); -- 2.14.1

7 years, 12 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: fix reservation_object_wait_timeout_rcu to wait correctly v2

by Deucher, Alexander

> -----Original Message----- > From: Christian König [mailto:deathsimple@vodafone.de] > Sent: Monday, July 31, 2017 10:13 AM > To: linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; linaro- > mm-sig(a)lists.linaro.org; Zhou, David(ChunMing); Deucher, Alexander > Subject: Re: [PATCH] dma-buf: fix reservation_object_wait_timeout_rcu to > wait correctly v2 > > Ping, what do you guys think of that? Seems reasonable to me. Reviewed-by: Alex Deucher <alexander.deucher(a)amd.com> > > Am 25.07.2017 um 15:35 schrieb Christian König: > > From: Christian König <christian.koenig(a)amd.com> > > > > With hardware resets in mind it is possible that all shared fences are > > signaled, but the exlusive isn't. Fix waiting for everything in this situation. > > > > v2: make sure we always wait for the exclusive fence > > > > Signed-off-by: Christian König <christian.koenig(a)amd.com> > > --- > > drivers/dma-buf/reservation.c | 33 +++++++++++++++------------------ > > 1 file changed, 15 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > > index 393817e..9d4316d 100644 > > --- a/drivers/dma-buf/reservation.c > > +++ b/drivers/dma-buf/reservation.c > > @@ -373,12 +373,25 @@ long reservation_object_wait_timeout_rcu(struct > reservation_object *obj, > > long ret = timeout ? timeout : 1; > > > > retry: > > - fence = NULL; > > shared_count = 0; > > seq = read_seqcount_begin(&obj->seq); > > rcu_read_lock(); > > > > - if (wait_all) { > > + fence = rcu_dereference(obj->fence_excl); > > + if (fence && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence- > >flags)) { > > + if (!dma_fence_get_rcu(fence)) > > + goto unlock_retry; > > + > > + if (dma_fence_is_signaled(fence)) { > > + dma_fence_put(fence); > > + fence = NULL; > > + } > > + > > + } else { > > + fence = NULL; > > + } > > + > > + if (!fence && wait_all) { > > struct reservation_object_list *fobj = > > rcu_dereference(obj- > >fence); > > > > @@ -405,22 +418,6 @@ long reservation_object_wait_timeout_rcu(struct > reservation_object *obj, > > } > > } > > > > - if (!shared_count) { > > - struct dma_fence *fence_excl = rcu_dereference(obj- > >fence_excl); > > - > > - if (fence_excl && > > - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > > - &fence_excl->flags)) { > > - if (!dma_fence_get_rcu(fence_excl)) > > - goto unlock_retry; > > - > > - if (dma_fence_is_signaled(fence_excl)) > > - dma_fence_put(fence_excl); > > - else > > - fence = fence_excl; > > - } > > - } > > - > > rcu_read_unlock(); > > if (fence) { > > if (read_seqcount_retry(&obj->seq, seq)) { >

8 years, 1 month

Re: [Linaro-mm-sig] [PATCH] dma-buf: fix reservation_object_wait_timeout_rcu to wait correctly

by Daniel Vetter

On Tue, Jul 25, 2017 at 11:11:35AM +0200, Christian König wrote: > Am 24.07.2017 um 13:57 schrieb Daniel Vetter: > > On Mon, Jul 24, 2017 at 11:51 AM, Christian König > > <deathsimple(a)vodafone.de> wrote: > > > Am 24.07.2017 um 10:33 schrieb Daniel Vetter: > > > > On Fri, Jul 21, 2017 at 06:20:01PM +0200, Christian König wrote: > > > > > From: Christian König <christian.koenig(a)amd.com> > > > > > > > > > > With hardware resets in mind it is possible that all shared fences are > > > > > signaled, but the exlusive isn't. Fix waiting for everything in this > > > > > situation. > > > > How did you end up with both shared and exclusive fences on the same > > > > reservation object? At least I thought the point of exclusive was that > > > > it's exclusive (and has an implicit barrier on all previous shared > > > > fences). Same for shared fences, they need to wait for the exclusive one > > > > (and replace it). > > > > > > > > Is this fallout from the amdgpu trickery where by default you do all > > > > shared fences? I thought we've aligned semantics a while back ... > > > > > > No, that is perfectly normal even for other drivers. Take a look at the > > > reservation code. > > > > > > The exclusive fence replaces all shared fences, but adding a shared fence > > > doesn't replace the exclusive fence. That actually makes sense, cause when > > > you want to add move shared fences those need to wait for the last exclusive > > > fence as well. > > Hm right. > > > > > Now normally I would agree that when you have shared fences it is sufficient > > > to wait for all of them cause those operations can't start before the > > > exclusive one finishes. But with GPU reset and/or the ability to abort > > > already submitted operations it is perfectly possible that you end up with > > > an exclusive fence which isn't signaled and a shared fence which is signaled > > > in the same reservation object. > > How does that work? The batch(es) with the shared fence are all > > supposed to wait for the exclusive fence before they start, which > > means even if you gpu reset and restart/cancel certain things, they > > shouldn't be able to complete out of order. > > Assume the following: > 1. The exclusive fence is some move operation by the kernel which executes > on a DMA engine. > 2. The shared fence is a 3D operation submitted by userspace which executes > on the 3D engine. > > Now we found the 3D engine to be hung and needs a reset, all currently > submitted jobs are aborted, marked with an error code and their fences put > into the signaled state. > > Since we only reset the 3D engine, the move operation (fortunately) isn't > affected by this. > > I think this applies to all drivers and isn't something amdgpu specific. Not i915 because: - At first we only had system wide gpu reset that killed everything, which means all requests will be completed, not just on a single engine. - Now we have per-engine reset, but combined with replaying them (the offending one gets a no-op batch to avoid re-hanging), to make sure the depency tree doesn't fall apart. Now I see that doing this isn't all that simple, and either way we still have the case where one driver resets but not the other (in multi-gpu), but I'm not exactly sure how to best handle this. What exactly is the downside of not dropping this assumption, i.e. why do you want this patch? What blows up? -Daniel > > Regards, > Christian. > > > > > If you outright cancel a fence then you're supposed to first call > > dma_fence_set_error(-EIO) and then complete it. Note that atm that > > part might be slightly overengineered and I'm not sure about how we > > expose stuff to userspace, e.g. dma_fence_set_error(-EAGAIN) is (or > > soon, has been) used by i915 for it's internal book-keeping, which > > might not be the best to leak to other consumers. But completing > > fences (at least exported ones, where userspace or other drivers can > > get at them) shouldn't be possible. > > -Daniel > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

8 years, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig