Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
2944 discussions

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, 7 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, 7 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, 7 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, 8 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, 9 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, 9 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, 10 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)) { >

7 years, 11 months

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

7 years, 11 months

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

by 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. 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 +41 (0) 79 365 57 48 - http://blog.ffwll.ch

7 years, 11 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig