- Linaro-mm-sig - lists.linaro.org

[PATCH 07/18] drm/atomic-helper: Add dma-fence annotations

by Daniel Vetter

This is a bit disappointing since we need to split the annotations over all the different parts. I was considering just leaking the critical section into the ->atomic_commit_tail callback of each driver. But that would mean we need to pass the fence_cookie into each driver (there's a total of 13 implementations of this hook right now), so bad flag day. And also a bit leaky abstraction. Hence just do it function-by-function. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 7cd7fe0d57b4..bfcc7857a9a1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1549,6 +1549,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done); void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; + bool fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_commit_modeset_disables(dev, old_state); @@ -1560,6 +1561,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state) drm_atomic_helper_commit_hw_done(old_state); + dma_fence_end_signalling(fence_cookie); + drm_atomic_helper_wait_for_vblanks(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); @@ -1579,6 +1582,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_tail); void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; + bool fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_commit_modeset_disables(dev, old_state); @@ -1591,6 +1595,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state) drm_atomic_helper_commit_hw_done(old_state); + dma_fence_end_signalling(fence_cookie); + drm_atomic_helper_wait_for_vblanks(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); @@ -1606,6 +1612,9 @@ static void commit_tail(struct drm_atomic_state *old_state) ktime_t start; s64 commit_time_ms; unsigned int i, new_self_refresh_mask = 0; + bool fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); funcs = dev->mode_config.helper_private; @@ -1634,6 +1643,8 @@ static void commit_tail(struct drm_atomic_state *old_state) if (new_crtc_state->self_refresh_active) new_self_refresh_mask |= BIT(i); + dma_fence_end_signalling(fence_cookie); + if (funcs && funcs->atomic_commit_tail) funcs->atomic_commit_tail(old_state); else @@ -1789,6 +1800,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, bool nonblock) { int ret; + bool fence_cookie; if (state->async_update) { ret = drm_atomic_helper_prepare_planes(dev, state); @@ -1811,6 +1823,8 @@ int drm_atomic_helper_commit(struct drm_device *dev, if (ret) return ret; + fence_cookie = dma_fence_begin_signalling(); + if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) @@ -1848,6 +1862,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, */ drm_atomic_state_get(state); + dma_fence_end_signalling(fence_cookie); if (nonblock) queue_work(system_unbound_wq, &state->commit_work); else @@ -1856,6 +1871,7 @@ int drm_atomic_helper_commit(struct drm_device *dev, return 0; err: + dma_fence_end_signalling(fence_cookie); drm_atomic_helper_cleanup_planes(dev, state); return ret; } -- 2.26.2

5 years

1
0
0 0

[PATCH 06/18] drm/vblank: Annotate with dma-fence signalling section

by Daniel Vetter

This is rather overkill since currently all drivers call this from hardirq (or at least timers). But maybe in the future we're going to have thread irq handlers and what not, doesn't hurt to be prepared. Plus this is an easy start for sprinkling these fence annotations into shared code. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_vblank.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 85e5f2db1608..93a5bba5f665 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -24,6 +24,7 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/dma-fence.h> #include <linux/export.h> #include <linux/moduleparam.h> @@ -1908,7 +1909,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; unsigned long irqflags; - bool disable_irq; + bool disable_irq, fence_cookie; if (drm_WARN_ON_ONCE(dev, !drm_dev_has_vblank(dev))) return false; @@ -1916,6 +1917,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (drm_WARN_ON(dev, pipe >= dev->num_crtcs)) return false; + fence_cookie = dma_fence_begin_signalling(); + spin_lock_irqsave(&dev->event_lock, irqflags); /* Need timestamp lock to prevent concurrent execution with @@ -1928,6 +1931,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (!vblank->enabled) { spin_unlock(&dev->vblank_time_lock); spin_unlock_irqrestore(&dev->event_lock, irqflags); + dma_fence_end_signalling(fence_cookie); return false; } @@ -1953,6 +1957,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) if (disable_irq) vblank_disable_fn(&vblank->disable_timer); + dma_fence_end_signalling(fence_cookie); + return true; } EXPORT_SYMBOL(drm_handle_vblank); -- 2.26.2

5 years

1
0
0 0

[PATCH 05/18] drm/vkms: Annotate vblank timer

by Daniel Vetter

This is needed to signal the fences from page flips, annotate it accordingly. We need to annotate entire timer callback since if we get stuck anywhere in there, then the timer stops, and hence fences stop. Just annotating the top part that does the vblank handling isn't enough. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo(a)gmail.com> Cc: Haneen Mohammed <hamohammed.sa(a)gmail.com> Cc: Daniel Vetter <daniel(a)ffwll.ch> --- drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index ac85e17428f8..a53a40848a72 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -1,5 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ +#include <linux/dma-fence.h> + #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_probe_helper.h> @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) struct drm_crtc *crtc = &output->crtc; struct vkms_crtc_state *state; u64 ret_overrun; - bool ret; + bool ret, fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, output->period_ns); @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) DRM_DEBUG_DRIVER("Composer worker already queued\n"); } + dma_fence_end_signalling(fence_cookie); + return HRTIMER_RESTART; } -- 2.26.2

5 years

1
0
0 0

[RFC 00/17] dma-fence lockdep annotations

by Daniel Vetter

Hi all, I've dragged my feet for years on this, hoping that cross-release lockdep would do this for us, but well that never really happened unfortunately. So here we are. Cc'ed quite a pile of people since this is about the cross-driver contract around dma_fences. Which is heavily used for dma_buf, and I'm hearing more noises that rdma folks are looking into this, hence also on cc. There's a bunch of different parts to this RFC: - The annotations itself, in the 2nd patch after the prep patch to add might_sleep annotations. Commit message has all the motivation for what kind of deadlocks I want to catch, best you just read it. Since lockdep doesn't understand cross-release natively we need to cobble something together using rwlocks and a few more tricks, but from the test rollout in a few places in drm/vkms, amdgpu & i915 I think what I have now seems to actually work. Downside is that we have to explicitly annotate all code involved in eventual dma_fence signalling. - Second important part is locking down the current dma-fence cross-driver contract, using lockdep priming like we already do for dma_resv_lock. I've just started with my own take on what we probably need to make the current code work (-ish), but both amdgpu and i915 have issues with that. So this needs some careful discussions, and also some thought on how we land it all eventually to not break lockdep completely for everyone. The important patch for that is "dma-fence: prime lockdep annotations" plus of course the various annotations patches and driver hacks to highlight some of the issues caught. Note that depending upon what exactly we end up deciding we might need to improve the annotations for fs_reclaim_acquire/release - for dma_fence_wait in mmu notifiers we can only allow GFP_NOWAIT (afaiui), and currently fs_reclaim_acquire/release only has a lockdep class for __GFP_FS only, we'd need to add another one for __GFP_DIRECT_RECLAIM in general maybe. - Finally there's clearly some gaps in the current dma_fence driver interfaces: Amdgpu's hang recovery is essentially impossible to fix as-is - it needs to reset the display state and you can't get at modeset locks from tdr without deadlock potential. i915 has an internal trick (but it stops working once we involve real cross-driver fences) for this issues, but then for i915 modeset reset is only needed on very ancient gen2/3. Modern hw is a lot more reasonable. I'm kinda hoping that the annotations and priming for basic command submission and atomic modeset paths could be merged soonish, while we the tdr side clearly needs a pile more work to get going. But since we have to explicitly annotate all code paths anyway we can hide bugs in e.g. tdr code by simply not yet annotating those functions. I'm trying to lay out at least one idea for solving the tdr issue in the patch titled "drm/scheduler: use dma-fence annotations in tdr work". Finally, once we have some agreement on where we're going with all this, we also need some documentation. Currently that's missing because I don't want to re-edit the text all the time while we still figure out the details of the exact cross-driver semantics. My goal here is that with this we can lock down the cross-driver contract for the last bit of the dma_buf/resv/fence story and make sure this stops being such a wobbly thing where everyone just does whatever they feel like. Ideas, thoughts, reviews, testing (with specific annotations for that driver) on other drivers very much welcome. Cheers, Daniel Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Daniel Vetter (17): dma-fence: add might_sleep annotation to _wait() dma-fence: basic lockdep annotations dma-fence: prime lockdep annotations drm/vkms: Annotate vblank timer drm/vblank: Annotate with dma-fence signalling section drm/atomic-helper: Add dma-fence annotations drm/amdgpu: add dma-fence annotations to atomic commit path drm/scheduler: use dma-fence annotations in main thread drm/amdgpu: use dma-fence annotations in cs_submit() drm/amdgpu: s/GFP_KERNEL/GFP_ATOMIC in scheduler code drm/amdgpu: DC also loves to allocate stuff where it shouldn't drm/amdgpu/dc: Stop dma_resv_lock inversion in commit_tail drm/scheduler: use dma-fence annotations in tdr work drm/amdgpu: use dma-fence annotations for gpu reset code Revert "drm/amdgpu: add fbdev suspend/resume on gpu reset" drm/amdgpu: gpu recovery does full modesets drm/i915: Annotate dma_fence_work drivers/dma-buf/dma-fence.c | 56 +++++++++++++++++++ drivers/dma-buf/dma-resv.c | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 ++++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 2 +- drivers/gpu/drm/amd/amdgpu/atom.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +++++- drivers/gpu/drm/amd/display/dc/core/dc.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++ drivers/gpu/drm/drm_vblank.c | 8 ++- drivers/gpu/drm/i915/i915_sw_fence_work.c | 3 + drivers/gpu/drm/scheduler/sched_main.c | 11 ++++ drivers/gpu/drm/vkms/vkms_crtc.c | 8 ++- include/linux/dma-fence.h | 13 +++++ 16 files changed, 160 insertions(+), 13 deletions(-) -- 2.26.2

5 years

7
42
0 0

Re: [Linaro-mm-sig] [RFC 02/17] dma-fence: basic lockdep annotations

by Daniel Vetter

On Thu, May 28, 2020 at 11:54 PM Luben Tuikov <luben.tuikov(a)amd.com> wrote: > > On 2020-05-12 4:59 a.m., Daniel Vetter wrote: > > Design is similar to the lockdep annotations for workers, but with > > some twists: > > > > - We use a read-lock for the execution/worker/completion side, so that > > this explicit annotation can be more liberally sprinkled around. > > With read locks lockdep isn't going to complain if the read-side > > isn't nested the same way under all circumstances, so ABBA deadlocks > > are ok. Which they are, since this is an annotation only. > > > > - We're using non-recursive lockdep read lock mode, since in recursive > > read lock mode lockdep does not catch read side hazards. And we > > _very_ much want read side hazards to be caught. For full details of > > this limitation see > > > > commit e91498589746065e3ae95d9a00b068e525eec34f > > Author: Peter Zijlstra <peterz(a)infradead.org> > > Date: Wed Aug 23 13:13:11 2017 +0200 > > > > locking/lockdep/selftests: Add mixed read-write ABBA tests > > > > - To allow nesting of the read-side explicit annotations we explicitly > > keep track of the nesting. lock_is_held() allows us to do that. > > > > - The wait-side annotation is a write lock, and entirely done within > > dma_fence_wait() for everyone by default. > > > > - To be able to freely annotate helper functions I want to make it ok > > to call dma_fence_begin/end_signalling from soft/hardirq context. > > First attempt was using the hardirq locking context for the write > > side in lockdep, but this forces all normal spinlocks nested within > > dma_fence_begin/end_signalling to be spinlocks. That bollocks. > > > > The approach now is to simple check in_atomic(), and for these cases > > entirely rely on the might_sleep() check in dma_fence_wait(). That > > will catch any wrong nesting against spinlocks from soft/hardirq > > contexts. > > > > The idea here is that every code path that's critical for eventually > > signalling a dma_fence should be annotated with > > dma_fence_begin/end_signalling. The annotation ideally starts right > > after a dma_fence is published (added to a dma_resv, exposed as a > > sync_file fd, attached to a drm_syncobj fd, or anything else that > > makes the dma_fence visible to other kernel threads), up to and > > including the dma_fence_wait(). Examples are irq handlers, the > > scheduler rt threads, the tail of execbuf (after the corresponding > > fences are visible), any workers that end up signalling dma_fences and > > really anything else. Not annotated should be code paths that only > > complete fences opportunistically as the gpu progresses, like e.g. > > shrinker/eviction code. > > > > The main class of deadlocks this is supposed to catch are: > > > > Thread A: > > > > mutex_lock(A); > > mutex_unlock(A); > > > > dma_fence_signal(); > > > > Thread B: > > > > mutex_lock(A); > > dma_fence_wait(); > > mutex_unlock(A); > > > > Thread B is blocked on A signalling the fence, but A never gets around > > to that because it cannot acquire the lock A. > > > > Note that dma_fence_wait() is allowed to be nested within > > dma_fence_begin/end_signalling sections. To allow this to happen the > > read lock needs to be upgraded to a write lock, which means that any > > other lock is acquired between the dma_fence_begin_signalling() call and > > the call to dma_fence_wait(), and still held, this will result in an > > immediate lockdep complaint. The only other option would be to not > > annotate such calls, defeating the point. Therefore these annotations > > cannot be sprinkled over the code entirely mindless to avoid false > > positives. > > > > v2: handle soft/hardirq ctx better against write side and dont forget > > EXPORT_SYMBOL, drivers can't use this otherwise. > > > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > Cc: linux-rdma(a)vger.kernel.org > > Cc: amd-gfx(a)lists.freedesktop.org > > Cc: intel-gfx(a)lists.freedesktop.org > > Cc: Chris Wilson <chris(a)chris-wilson.co.uk> > > Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > > Cc: Christian König <christian.koenig(a)amd.com> > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > --- > > drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++ > > include/linux/dma-fence.h | 12 +++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 6802125349fb..d5c0fd2efc70 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num) > > } > > EXPORT_SYMBOL(dma_fence_context_alloc); > > > > +#ifdef CONFIG_LOCKDEP > > +struct lockdep_map dma_fence_lockdep_map = { > > + .name = "dma_fence_map" > > +}; > > + > > +bool dma_fence_begin_signalling(void) > > +{ > > + /* explicitly nesting ... */ > > + if (lock_is_held_type(&dma_fence_lockdep_map, 1)) > > + return true; > > + > > + /* rely on might_sleep check for soft/hardirq locks */ > > + if (in_atomic()) > > + return true; > > + > > + /* ... and non-recursive readlock */ > > + lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_); > > + > > + return false; > > +} > > +EXPORT_SYMBOL(dma_fence_begin_signalling); > > Hi Daniel, > > This is great work and could help a lot. > > If you invert the result of dma_fence_begin_signalling() > then it would naturally mean "locked", i.e. whether we need to > later release "dma_fence_lockedep_map". Then, > in dma_fence_end_signalling(), you can call the "cookie" > argument "locked" and simply do: > > void dma_fence_end_signalling(bool locked) > { > if (locked) > lock_release(&dma_fence_lockdep_map, _RET_IP_); > } > EXPORT_SYMBOL(dma_fence_end_signalling); > > It'll be more natural to understand as well. It's intentionally called cookie so callers don't start doing funny stuff with it. The thing is, after begin_signalling you are _always_ in the locked state. It's just that because of limitations with lockdep we need to play a few tricks, and in some cases we do not take the lockdep map. There's 2 cases: - lockdep map already taken - we want recursive readlock semantics for this, but lockdep does not correctly check recursive read locks. Hence we only use readlock, and make sure we do not actually nest upon ourselves with this explicit check. - when we're in atomic sections - lockdep gets pissed at us if we take the read lock in hard/softirq sections because of hard/softirq ctx mismatch (lockdep thinks it's a real lock, but we don't treat it as one). Simplest fix was to rely on the might_sleep check in patch 1 (already merged) The commit message mentions this already a bit, but I'll try to explain this implementation detail tersely in the kerneldoc too in the next round. Thanks, Daniel > > Regards, > Luben > > > + > > +void dma_fence_end_signalling(bool cookie) > > +{ > > + if (cookie) > > + return; > > + > > + lock_release(&dma_fence_lockdep_map, _RET_IP_); > > +} > > +EXPORT_SYMBOL(dma_fence_end_signalling); > > + > > +void __dma_fence_might_wait(void) > > +{ > > + bool tmp; > > + > > + tmp = lock_is_held_type(&dma_fence_lockdep_map, 1); > > + if (tmp) > > + lock_release(&dma_fence_lockdep_map, _THIS_IP_); > > + lock_map_acquire(&dma_fence_lockdep_map); > > + lock_map_release(&dma_fence_lockdep_map); > > + if (tmp) > > + lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_); > > +} > > +#endif > > + > > + > > /** > > * dma_fence_signal_locked - signal completion of a fence > > * @fence: the fence to signal > > @@ -170,14 +216,19 @@ int dma_fence_signal(struct dma_fence *fence) > > { > > unsigned long flags; > > int ret; > > + bool tmp; > > > > if (!fence) > > return -EINVAL; > > > > + tmp = dma_fence_begin_signalling(); > > + > > spin_lock_irqsave(fence->lock, flags); > > ret = dma_fence_signal_locked(fence); > > spin_unlock_irqrestore(fence->lock, flags); > > > > + dma_fence_end_signalling(tmp); > > + > > return ret; > > } > > EXPORT_SYMBOL(dma_fence_signal); > > @@ -211,6 +262,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) > > if (timeout > 0) > > might_sleep(); > > > > + __dma_fence_might_wait(); > > + > > trace_dma_fence_wait_start(fence); > > if (fence->ops->wait) > > ret = fence->ops->wait(fence, intr, timeout); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > > index 3347c54f3a87..3f288f7db2ef 100644 > > --- a/include/linux/dma-fence.h > > +++ b/include/linux/dma-fence.h > > @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) > > } while (1); > > } > > > > +#ifdef CONFIG_LOCKDEP > > +bool dma_fence_begin_signalling(void); > > +void dma_fence_end_signalling(bool cookie); > > +#else > > +static inline bool dma_fence_begin_signalling(void) > > +{ > > + return true; > > +} > > +static inline void dma_fence_end_signalling(bool cookie) {} > > +static inline void __dma_fence_might_wait(void) {} > > +#endif > > + > > int dma_fence_signal(struct dma_fence *fence); > > int dma_fence_signal_locked(struct dma_fence *fence); > > signed long dma_fence_default_wait(struct dma_fence *fence, > > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years

1
0
0 0

Re: [Linaro-mm-sig] [RFC 02/17] dma-fence: basic lockdep annotations

by Daniel Vetter

On Thu, May 28, 2020 at 3:37 PM Thomas Hellström (Intel) <thomas_os(a)shipmail.org> wrote: > > On 2020-05-12 10:59, Daniel Vetter wrote: > > Design is similar to the lockdep annotations for workers, but with > > some twists: > > > > - We use a read-lock for the execution/worker/completion side, so that > > this explicit annotation can be more liberally sprinkled around. > > With read locks lockdep isn't going to complain if the read-side > > isn't nested the same way under all circumstances, so ABBA deadlocks > > are ok. Which they are, since this is an annotation only. > > > > - We're using non-recursive lockdep read lock mode, since in recursive > > read lock mode lockdep does not catch read side hazards. And we > > _very_ much want read side hazards to be caught. For full details of > > this limitation see > > > > commit e91498589746065e3ae95d9a00b068e525eec34f > > Author: Peter Zijlstra <peterz(a)infradead.org> > > Date: Wed Aug 23 13:13:11 2017 +0200 > > > > locking/lockdep/selftests: Add mixed read-write ABBA tests > > > > - To allow nesting of the read-side explicit annotations we explicitly > > keep track of the nesting. lock_is_held() allows us to do that. > > > > - The wait-side annotation is a write lock, and entirely done within > > dma_fence_wait() for everyone by default. > > > > - To be able to freely annotate helper functions I want to make it ok > > to call dma_fence_begin/end_signalling from soft/hardirq context. > > First attempt was using the hardirq locking context for the write > > side in lockdep, but this forces all normal spinlocks nested within > > dma_fence_begin/end_signalling to be spinlocks. That bollocks. > > > > The approach now is to simple check in_atomic(), and for these cases > > entirely rely on the might_sleep() check in dma_fence_wait(). That > > will catch any wrong nesting against spinlocks from soft/hardirq > > contexts. > > > > The idea here is that every code path that's critical for eventually > > signalling a dma_fence should be annotated with > > dma_fence_begin/end_signalling. The annotation ideally starts right > > after a dma_fence is published (added to a dma_resv, exposed as a > > sync_file fd, attached to a drm_syncobj fd, or anything else that > > makes the dma_fence visible to other kernel threads), up to and > > including the dma_fence_wait(). Examples are irq handlers, the > > scheduler rt threads, the tail of execbuf (after the corresponding > > fences are visible), any workers that end up signalling dma_fences and > > really anything else. Not annotated should be code paths that only > > complete fences opportunistically as the gpu progresses, like e.g. > > shrinker/eviction code. > > > > The main class of deadlocks this is supposed to catch are: > > > > Thread A: > > > > mutex_lock(A); > > mutex_unlock(A); > > > > dma_fence_signal(); > > > > Thread B: > > > > mutex_lock(A); > > dma_fence_wait(); > > mutex_unlock(A); > > > > Thread B is blocked on A signalling the fence, but A never gets around > > to that because it cannot acquire the lock A. > > > > Note that dma_fence_wait() is allowed to be nested within > > dma_fence_begin/end_signalling sections. To allow this to happen the > > read lock needs to be upgraded to a write lock, which means that any > > other lock is acquired between the dma_fence_begin_signalling() call and > > the call to dma_fence_wait(), and still held, this will result in an > > immediate lockdep complaint. The only other option would be to not > > annotate such calls, defeating the point. Therefore these annotations > > cannot be sprinkled over the code entirely mindless to avoid false > > positives. > > > > v2: handle soft/hardirq ctx better against write side and dont forget > > EXPORT_SYMBOL, drivers can't use this otherwise. > > > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > Cc: linux-rdma(a)vger.kernel.org > > Cc: amd-gfx(a)lists.freedesktop.org > > Cc: intel-gfx(a)lists.freedesktop.org > > Cc: Chris Wilson <chris(a)chris-wilson.co.uk> > > Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > > Cc: Christian König <christian.koenig(a)amd.com> > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > LGTM. Perhaps some in-code documentation on how to use the new functions > are called. See cover letter, that's going to be done for next round. For this one here I just wanted to showcase a bit how it's used in a few different places, mostly selected to get as much feedback from across different drivers. Hence e.g. annotating drm/scheduler. > Otherwise for patch 2 and 3, > > Reviewed-by: Thomas Hellstrom <thomas.hellstrom(a)intel.com> I think I'll just cc you for the next round with docs, so you can make sure it looks ok :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 0/3] Support virtio cross-device resources

by Gerd Hoffmann

On Tue, May 26, 2020 at 07:58:08PM +0900, David Stevens wrote: > This patchset implements the current proposal for virtio cross-device > resource sharing [1]. It will be used to import virtio resources into > the virtio-video driver currently under discussion [2]. The patch > under consideration to add support in the virtio-video driver is [3]. > It uses the APIs from v3 of this series, but the changes to update it > are relatively minor. > > This patchset adds a new flavor of dma-bufs that supports querying the > underlying virtio object UUID, as well as adding support for exporting > resources from virtgpu. > > [1] https://markmail.org/thread/2ypjt5cfeu3m6lxu > [2] https://markmail.org/thread/p5d3k566srtdtute > [3] https://markmail.org/thread/j4xlqaaim266qpks > > v3 -> v4 changes: > - Replace dma-buf hooks with virtio dma-buf from v1. > - Remove virtio_attach callback, as the work that had been done > in that callback is now done on dma-buf export. The documented > requirement that get_uuid only be called on attached virtio > dma-bufs is also removed. > - Rebase and add call to virtio_gpu_notify for ASSIGN_UUID. > > David Stevens (3): > virtio: add dma-buf support for exported objects > virtio-gpu: add VIRTIO_GPU_F_RESOURCE_UUID feature > drm/virtio: Support virtgpu exported resources Looks all sane to me. mst, have you looked at the virtio core changes? How we are going to merge this? If you ack I can merge via drm-misc-next. Merging through virtio queue would be fine too. thanks, Gerd

5 years

1
0
0 0

[PATCH v5 00/38] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the. drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used a simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. To avoid similar issues in the future, I've introduced a common wrappers for DMA-mapping calls, which operate directly on the sg_table objects. I've also added wrappers for iterating over the scatterlists stored in the sg_table objects and applied them where possible. This, together with some common DRM prime helpers, allowed me to almost get rid of all nents/orig_nents usage in the drivers. I hope that such change makes the code robust, easier to follow and copy/paste safe. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. Patches are based on top of Linux next-20200512. Christoph Hellwig already offered to take patches 1-3 into his immutable branch [4]. If possible I would like ask for merging most of the remaining patches via DRM tree (on top of that immutable branch). Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt [4] https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/#ma18c9… Changelog: v5: - fixed some minor style issues and typos - fixed lack of the attrs argument in ion, dmabuf, rapidio, fastrpc and vfio patches v4: https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/ - added for_each_sgtable_* wrappers and applied where possible - added drm_prime_get_contiguous_size() and applied where possible - applied drm_prime_sg_to_page_addr_arrays() where possible to remove page extraction from sg_table objects - added documentation for the introduced wrappers - improved patches description a bit v3: https://lore.kernel.org/dri-devel/20200505083926.28503-1-m.szyprowski@samsu… - introduce dma_*_sgtable_* wrappers and use them in all patches v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (38): dma-mapping: add generic helpers for mapping sgtable objects scatterlist: add generic wrappers for iterating over sgtable objects iommu: add generic helper for mapping sgtable objects drm: prime: add common helper to check scatterlist contiguity drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() drm: core: fix common struct sg_table related issues drm: amdgpu: fix common struct sg_table related issues drm: armada: fix common struct sg_table related issues drm: etnaviv: fix common struct sg_table related issues drm: exynos: use common helper for a scatterlist contiguity check drm: exynos: fix common struct sg_table related issues drm: i915: fix common struct sg_table related issues drm: lima: fix common struct sg_table related issues drm: mediatek: use common helper for a scatterlist contiguity check drm: mediatek: use common helper for extracting pages array drm: msm: fix common struct sg_table related issues drm: omapdrm: use common helper for extracting pages array drm: omapdrm: fix common struct sg_table related issues drm: panfrost: fix common struct sg_table related issues drm: radeon: fix common struct sg_table related issues drm: rockchip: use common helper for a scatterlist contiguity check drm: rockchip: fix common struct sg_table related issues drm: tegra: fix common struct sg_table related issues drm: v3d: fix common struct sg_table related issues drm: virtio: fix common struct sg_table related issues drm: vmwgfx: fix common struct sg_table related issues xen: gntdev: fix common struct sg_table related issues drm: host1x: fix common struct sg_table related issues drm: rcar-du: fix common struct sg_table related issues dmabuf: fix common struct sg_table related issues staging: ion: remove dead code staging: ion: fix common struct sg_table related issues staging: tegra-vde: fix common struct sg_table related issues misc: fastrpc: fix common struct sg_table related issues rapidio: fix common struct sg_table related issues samples: vfio-mdev/mbochs: fix common struct sg_table related issues media: pci: fix common ALSA DMA-mapping related codes videobuf2: use sgtable-based scatterlist wrappers drivers/dma-buf/heaps/heap-helpers.c | 13 ++-- drivers/dma-buf/udmabuf.c | 7 +- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 +- drivers/gpu/drm/armada/armada_gem.c | 12 +-- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_cma_helper.c | 23 +----- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 ++-- drivers/gpu/drm/drm_prime.c | 86 ++++++++++++---------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +--- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +-- drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +----- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +-- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +- drivers/gpu/drm/lima/lima_gem.c | 11 ++- drivers/gpu/drm/lima/lima_vm.c | 5 +- drivers/gpu/drm/mediatek/mtk_drm_gem.c | 34 ++------- drivers/gpu/drm/msm/msm_gem.c | 13 ++-- drivers/gpu/drm/msm/msm_gpummu.c | 14 ++-- drivers/gpu/drm/msm/msm_iommu.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 20 ++--- drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +- drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 42 +++-------- drivers/gpu/drm/tegra/gem.c | 27 +++---- drivers/gpu/drm/tegra/plane.c | 15 ++-- drivers/gpu/drm/v3d/v3d_mmu.c | 17 ++--- drivers/gpu/drm/virtio/virtgpu_object.c | 36 +++++---- drivers/gpu/drm/virtio/virtgpu_vq.c | 12 ++- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +---- drivers/gpu/host1x/job.c | 22 ++---- .../media/common/videobuf2/videobuf2-dma-contig.c | 41 +++++------ drivers/media/common/videobuf2/videobuf2-dma-sg.c | 32 +++----- drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +-- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 8 +- drivers/misc/fastrpc.c | 4 +- drivers/rapidio/devices/rio_mport_cdev.c | 8 +- drivers/staging/android/ion/ion.c | 25 +++---- drivers/staging/android/ion/ion.h | 1 - drivers/staging/android/ion/ion_heap.c | 53 ++++--------- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/staging/media/tegra-vde/iommu.c | 4 +- drivers/xen/gntdev-dmabuf.c | 13 ++-- include/drm/drm_prime.h | 2 + include/linux/dma-mapping.h | 78 ++++++++++++++++++++ include/linux/iommu.h | 16 ++++ include/linux/scatterlist.h | 50 ++++++++++++- samples/vfio-mdev/mbochs.c | 3 +- 56 files changed, 451 insertions(+), 477 deletions(-) -- 1.9.1

5 years

7
47
0 0

Re: [Linaro-mm-sig] [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

by Daniel Vetter

On Thu, May 14, 2020 at 02:38:38PM +0300, Oded Gabbay wrote: > On Tue, May 12, 2020 at 9:12 AM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > > On Tue, May 12, 2020 at 4:14 AM Dave Airlie <airlied(a)gmail.com> wrote: > > > > > > On Mon, 11 May 2020 at 19:37, Oded Gabbay <oded.gabbay(a)gmail.com> wrote: > > > > > > > > On Mon, May 11, 2020 at 12:11 PM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > > > > > > > > It's the default. > > > > Thanks for catching that. > > > > > > > > > > > > > > Also so much for "we're not going to tell the graphics people how to > > > > > review their code", dma_fence is a pretty core piece of gpu driver > > > > > infrastructure. And it's very much uapi relevant, including piles of > > > > > corresponding userspace protocols and libraries for how to pass these > > > > > around. > > > > > > > > > > Would be great if habanalabs would not use this (from a quick look > > > > > it's not needed at all), since open source the userspace and playing > > > > > by the usual rules isn't on the table. If that's not possible (because > > > > > it's actually using the uapi part of dma_fence to interact with gpu > > > > > drivers) then we have exactly what everyone promised we'd want to > > > > > avoid. > > > > > > > > We don't use the uapi parts, we currently only using the fencing and > > > > signaling ability of this module inside our kernel code. But maybe I > > > > didn't understand what you request. You want us *not* to use this > > > > well-written piece of kernel code because it is only used by graphics > > > > drivers ? > > > > I'm sorry but I don't get this argument, if this is indeed what you meant. > > > > > > We would rather drivers using a feature that has requirements on > > > correct userspace implementations of the feature have a userspace that > > > is open source and auditable. > > > > > > Fencing is tricky, cross-device fencing is really tricky, and having > > > the ability for a closed userspace component to mess up other people's > > > drivers, think i915 shared with closed habana userspace and shared > > > fences, decreases ability to debug things. > > > > > > Ideally we wouldn't offer users known untested/broken scenarios, so > > > yes we'd prefer that drivers that intend to expose a userspace fencing > > > api around dma-fence would adhere to the rules of the gpu drivers. > > > > > > I'm not say you have to drop using dma-fence, but if you move towards > > > cross-device stuff I believe other drivers would be correct in > > > refusing to interact with fences from here. > > > > The flip side is if you only used dma-fence.c "because it's there", > > and not because it comes with an uapi attached and a cross-driver > > kernel internal contract for how to interact with gpu drivers, then > > there's really not much point in using it. It's a custom-rolled > > wait_queue/event thing, that's all. Without the gpu uapi and gpu > > cross-driver contract it would be much cleaner to just use wait_queue > > directly, and that's a construct all kernel developers understand, not > > just gpu folks. From a quick look at least habanalabs doesn't use any > > of these uapi/cross-driver/gpu bits. > > -Daniel > > Hi Daniel, > I want to say explicitly that we don't use the dma-buf uapi parts, nor > we intend to use them to communicate with any GPU device. We only use > it as simple completion mechanism as it was convenient to use. > I do understand I can exchange that mechanism with a simpler one, and > I will add an internal task to do it (albeit not in a very high > priority) and upstream it, its just that it is part of our data path > so we need to thoroughly validate it first. Sounds good. Wrt merging this patch here, can you include that in one of your next pulls? Or should I toss it entirely, waiting for you to remove dma_fence outright? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

1
0
0 0

[PATCH] dma-fence: add might_sleep annotation to _wait()

by Daniel Vetter

Do it uncontionally, there's a separate peek function with dma_fence_is_signalled() which can be called from atomic context. v2: Consensus calls for an unconditional might_sleep (Chris, Christian) Full audit: - dma-fence.h: Uses MAX_SCHEDULE_TIMOUT, good chance this sleeps - dma-resv.c: Timeout always at least 1 - st-dma-fence.c: Save to sleep in testcases - amdgpu_cs.c: Both callers are for variants of the wait ioctl - amdgpu_device.c: Two callers in vram recover code, both right next to mutex_lock. - amdgpu_vm.c: Use in the vm_wait ioctl, next to _reserve/unreserve - remaining functions in amdgpu: All for test_ib implementations for various engines, caller for that looks all safe (debugfs, driver load, reset) - etnaviv: another wait ioctl - habanalabs: another wait ioctl - nouveau_fence.c: hardcoded 15*HZ ... glorious - nouveau_gem.c: hardcoded 2*HZ ... so not even super consistent, but this one does have a WARN_ON :-/ At least this one is only a fallback path for when kmalloc fails. Maybe this should be put onto some worker list instead, instead of a work per unamp ... - i915/selftests: Hardecoded HZ / 4 or HZ / 8 - i915/gt/selftests: Going up the callchain looks safe looking at nearby callers - i915/gt/intel_gt_requests.c. Wrapped in a mutex_lock - i915/gem_i915_gem_wait.c: The i915-version which is called instead for i915 fences already has a might_sleep() annotation, so all good Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Jani Nikula <jani.nikula(a)linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen(a)linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi(a)intel.com> Cc: Ben Skeggs <bskeggs(a)redhat.com> Cc: "VMware Graphics" <linux-graphics-maintainer(a)vmware.com> Cc: Oded Gabbay <oded.gabbay(a)gmail.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/dma-buf/dma-fence.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 90edf2b281b0..656e9ac2d028 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -208,6 +208,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) if (WARN_ON(timeout < 0)) return -EINVAL; + might_sleep(); + trace_dma_fence_wait_start(fence); if (fence->ops->wait) ret = fence->ops->wait(fence, intr, timeout); -- 2.26.2

5 years

4
3
0 0

Re: [Linaro-mm-sig] [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe

by Lucas Stach

Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > Hi > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach(a)pengutronix.de> wrote: > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > There are two requirements that we need to move the request > > > of dma channel from probe to open. > > > > How do you handle -EPROBE_DEFER return code from the channel request if > > you don't do it in probe? > > I use the dma_request_slave_channel or dma_request_channel instead > of dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > return code. This is a pretty weak argument. The dmaengine device might probe after you try to get the channel. Using a function to request the channel that doesn't allow you to handle probe deferral is IMHO a bug and should be fixed, instead of building even more assumptions on top of it. > > > - When dma device binds with power-domains, the power will > > > be enabled when we request dma channel. If the request of dma > > > channel happen on probe, then the power-domains will be always > > > enabled after kernel boot up, which is not good for power > > > saving, so we need to move the request of dma channel to .open(); > > > > This is certainly something which could be fixed in the dmaengine > > driver. > > Dma driver always call the pm_runtime_get_sync in > device_alloc_chan_resources, the device_alloc_chan_resources is > called when channel is requested. so power is enabled on channel > request. So why can't you fix the dmaengine driver to do that RPM call at a later time when the channel is actually going to be used? This will allow further power savings with other slave devices than the audio PCM. Regards, Lucas > > > - With FE-BE case, if the dma channel is requested in probe, > > > then there will be below issue, which is caused by that the > > > dma channel will be requested duplicately > > > > Why is this requested a second time? Is this just some missing cleanup > > on a deferred probe path? > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, > the DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier > probe, DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel > is not used in this FE-BE case). When FE-BE startup, DMA2 > channel is created, it needs the ESAI's "dma:tx", so below warning > comes out. > > > Regards, > > Lucas > > > > > [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 > > > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > > [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) > > > [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) > > > [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) > > > [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) > > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) > > > [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) > > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) > > > [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) > > > [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) > > > [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) > > > [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) > > > [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > > > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > > > [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 > > > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > > > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> > > > --- > > > sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- > > > 1 file changed, 159 insertions(+), 14 deletions(-) > > > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c > > > index 04a9bc749016..dae53b384df4 100644 > > > --- a/sound/soc/fsl/imx-pcm-dma.c > > > +++ b/sound/soc/fsl/imx-pcm-dma.c > > > @@ -11,6 +11,7 @@ > > > #include <linux/dmaengine.h> > > > #include <linux/types.h> > > > #include <linux/module.h> > > > +#include <linux/dma-mapping.h> > > > > > > #include <sound/core.h> > > > #include <sound/pcm.h> > > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) > > > return true; > > > } > > > > > > -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { > > > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > > > - .compat_filter_fn = filter, > > > -}; > > > +static int imx_pcm_hw_params(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream, > > > + struct snd_pcm_hw_params *params) > > > +{ > > > + struct snd_pcm_runtime *runtime = substream->runtime; > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > + struct dma_slave_config config; > > > + struct dma_chan *chan; > > > + int ret = 0; > > > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > > > + runtime->dma_bytes = params_buffer_bytes(params); > > > + > > > + chan = snd_dmaengine_pcm_get_chan(substream); > > > + if (!chan) > > > + return -EINVAL; > > > + > > > + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); > > > + if (ret) > > > + return ret; > > > + > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > + if (!dma_data) > > > + return -EINVAL; > > > + > > > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > > > + dma_data, > > > + &config); > > > + return dmaengine_slave_config(chan, &config); > > > +} > > > + > > > +static int imx_pcm_hw_free(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > { > > > - struct snd_dmaengine_pcm_config *config; > > > + snd_pcm_set_runtime_buffer(substream, NULL); > > > + return 0; > > > +} > > > + > > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + return snd_dmaengine_pcm_pointer(substream); > > > +} > > > + > > > +static int imx_pcm_trigger(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream, int cmd) > > > +{ > > > + return snd_dmaengine_pcm_trigger(substream, cmd); > > > +} > > > + > > > +static int imx_pcm_open(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > + struct device *dev = component->dev; > > > + struct snd_pcm_hardware hw; > > > + struct dma_chan *chan; > > > + int ret; > > > + > > > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > > > + SNDRV_PCM_HW_PARAM_PERIODS); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to set pcm hw params periods\n"); > > > + return ret; > > > + } > > > + > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > + if (!dma_data) > > > + return -EINVAL; > > > + > > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > > > + if (!chan) { > > > + /* Try to request channel using compat_filter_fn */ > > > + chan = snd_dmaengine_pcm_request_channel(filter, > > > + dma_data->filter_data); > > > + if (!chan) > > > + return -ENXIO; > > > + } > > > > > > - config = devm_kzalloc(&pdev->dev, > > > - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); > > > - if (!config) > > > - return -ENOMEM; > > > - *config = imx_dmaengine_pcm_config; > > > + ret = snd_dmaengine_pcm_open(substream, chan); > > > + if (ret) > > > + goto pcm_open_fail; > > > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > > > - config, > > > - SND_DMAENGINE_PCM_FLAG_COMPAT); > > > + memset(&hw, 0, sizeof(hw)); > > > + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > > > + SNDRV_PCM_INFO_INTERLEAVED; > > > + hw.periods_min = 2; > > > + hw.periods_max = UINT_MAX; > > > + hw.period_bytes_min = 256; > > > + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); > > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > > > + hw.fifo_size = dma_data->fifo_size; > > > + > > > + /* Refine the hw according to caps of DMA. */ > > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > > > + dma_data, > > > + &hw, > > > + chan); > > > + if (ret < 0) > > > + goto refine_runtime_hwparams_fail; > > > + > > > + snd_soc_set_runtime_hwparams(substream, &hw); > > > + > > > + /* Support allocate memory from IRAM */ > > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > > > + chan->device->dev, > > > + hw.buffer_bytes_max, > > > + &substream->dma_buffer); > > > + if (ret < 0) > > > + goto alloc_pagas_fail; > > > + > > > + return 0; > > > + > > > +alloc_pagas_fail: > > > +refine_runtime_hwparams_fail: > > > + snd_dmaengine_pcm_close(substream); > > > +pcm_open_fail: > > > + dma_release_channel(chan); > > > + > > > + return ret; > > > +} > > > + > > > +static int imx_pcm_close(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + if (substream) { > > > + snd_dma_free_pages(&substream->dma_buffer); > > > + substream->dma_buffer.area = NULL; > > > + substream->dma_buffer.addr = 0; > > > + } > > > + > > > + return snd_dmaengine_pcm_close_release_chan(substream); > > > +} > > > + > > > +static int imx_pcm_new(struct snd_soc_component *component, > > > + struct snd_soc_pcm_runtime *rtd) > > > +{ > > > + struct snd_card *card = rtd->card->snd_card; > > > + > > > + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > > +} > > > + > > > +static const struct snd_soc_component_driver imx_pcm_component = { > > > + .name = "imx-pcm-dma", > > > + .pcm_construct = imx_pcm_new, > > > + .open = imx_pcm_open, > > > + .close = imx_pcm_close, > > > + .hw_params = imx_pcm_hw_params, > > > + .hw_free = imx_pcm_hw_free, > > > + .trigger = imx_pcm_trigger, > > > + .pointer = imx_pcm_pointer, > > > +}; > > > + > > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > +{ > > > + return devm_snd_soc_register_component(&pdev->dev, > > > + &imx_pcm_component, NULL, 0); > > > } > > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); > > >

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe

by Lucas Stach

Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > There are two requirements that we need to move the request > of dma channel from probe to open. How do you handle -EPROBE_DEFER return code from the channel request if you don't do it in probe? > - When dma device binds with power-domains, the power will > be enabled when we request dma channel. If the request of dma > channel happen on probe, then the power-domains will be always > enabled after kernel boot up, which is not good for power > saving, so we need to move the request of dma channel to .open(); This is certainly something which could be fixed in the dmaengine driver. > - With FE-BE case, if the dma channel is requested in probe, > then there will be below issue, which is caused by that the > dma channel will be requested duplicately Why is this requested a second time? Is this just some missing cleanup on a deferred probe path? Regards, Lucas > [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) > [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) > [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) > [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) > [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) > [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) > [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) > [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) > [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) > [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > Signed-off-by: Shengjiu Wang <shengjiu.wang(a)nxp.com> > --- > sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- > 1 file changed, 159 insertions(+), 14 deletions(-) > > diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c > index 04a9bc749016..dae53b384df4 100644 > --- a/sound/soc/fsl/imx-pcm-dma.c > +++ b/sound/soc/fsl/imx-pcm-dma.c > @@ -11,6 +11,7 @@ > #include <linux/dmaengine.h> > #include <linux/types.h> > #include <linux/module.h> > +#include <linux/dma-mapping.h> > > #include <sound/core.h> > #include <sound/pcm.h> > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) > return true; > } > > -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > - .compat_filter_fn = filter, > -}; > +static int imx_pcm_hw_params(struct snd_soc_component *component, > + struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > + struct snd_dmaengine_dai_dma_data *dma_data; > + struct dma_slave_config config; > + struct dma_chan *chan; > + int ret = 0; > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > + runtime->dma_bytes = params_buffer_bytes(params); > + > + chan = snd_dmaengine_pcm_get_chan(substream); > + if (!chan) > + return -EINVAL; > + > + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); > + if (ret) > + return ret; > + > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > + if (!dma_data) > + return -EINVAL; > + > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > + dma_data, > + &config); > + return dmaengine_slave_config(chan, &config); > +} > + > +static int imx_pcm_hw_free(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > { > - struct snd_dmaengine_pcm_config *config; > + snd_pcm_set_runtime_buffer(substream, NULL); > + return 0; > +} > + > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + return snd_dmaengine_pcm_pointer(substream); > +} > + > +static int imx_pcm_trigger(struct snd_soc_component *component, > + struct snd_pcm_substream *substream, int cmd) > +{ > + return snd_dmaengine_pcm_trigger(substream, cmd); > +} > + > +static int imx_pcm_open(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > + struct snd_dmaengine_dai_dma_data *dma_data; > + struct device *dev = component->dev; > + struct snd_pcm_hardware hw; > + struct dma_chan *chan; > + int ret; > + > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) { > + dev_err(dev, "failed to set pcm hw params periods\n"); > + return ret; > + } > + > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > + if (!dma_data) > + return -EINVAL; > + > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > + if (!chan) { > + /* Try to request channel using compat_filter_fn */ > + chan = snd_dmaengine_pcm_request_channel(filter, > + dma_data->filter_data); > + if (!chan) > + return -ENXIO; > + } > > - config = devm_kzalloc(&pdev->dev, > - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); > - if (!config) > - return -ENOMEM; > - *config = imx_dmaengine_pcm_config; > + ret = snd_dmaengine_pcm_open(substream, chan); > + if (ret) > + goto pcm_open_fail; > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > - config, > - SND_DMAENGINE_PCM_FLAG_COMPAT); > + memset(&hw, 0, sizeof(hw)); > + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > + SNDRV_PCM_INFO_INTERLEAVED; > + hw.periods_min = 2; > + hw.periods_max = UINT_MAX; > + hw.period_bytes_min = 256; > + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > + hw.fifo_size = dma_data->fifo_size; > + > + /* Refine the hw according to caps of DMA. */ > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > + dma_data, > + &hw, > + chan); > + if (ret < 0) > + goto refine_runtime_hwparams_fail; > + > + snd_soc_set_runtime_hwparams(substream, &hw); > + > + /* Support allocate memory from IRAM */ > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > + chan->device->dev, > + hw.buffer_bytes_max, > + &substream->dma_buffer); > + if (ret < 0) > + goto alloc_pagas_fail; > + > + return 0; > + > +alloc_pagas_fail: > +refine_runtime_hwparams_fail: > + snd_dmaengine_pcm_close(substream); > +pcm_open_fail: > + dma_release_channel(chan); > + > + return ret; > +} > + > +static int imx_pcm_close(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + if (substream) { > + snd_dma_free_pages(&substream->dma_buffer); > + substream->dma_buffer.area = NULL; > + substream->dma_buffer.addr = 0; > + } > + > + return snd_dmaengine_pcm_close_release_chan(substream); > +} > + > +static int imx_pcm_new(struct snd_soc_component *component, > + struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_card *card = rtd->card->snd_card; > + > + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > +} > + > +static const struct snd_soc_component_driver imx_pcm_component = { > + .name = "imx-pcm-dma", > + .pcm_construct = imx_pcm_new, > + .open = imx_pcm_open, > + .close = imx_pcm_close, > + .hw_params = imx_pcm_hw_params, > + .hw_free = imx_pcm_hw_free, > + .trigger = imx_pcm_trigger, > + .pointer = imx_pcm_pointer, > +}; > + > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > +{ > + return devm_snd_soc_register_component(&pdev->dev, > + &imx_pcm_component, NULL, 0); > } > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); >

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/4] dma-buf: add support for virtio exported objects

by Daniel Vetter

On Fri, May 15, 2020 at 02:07:06PM +0900, David Stevens wrote: > On Thu, May 14, 2020 at 9:30 PM Daniel Vetter <daniel(a)ffwll.ch> wrote: > > On Thu, May 14, 2020 at 05:19:40PM +0900, David Stevens wrote: > > > Sorry for the duplicate reply, didn't notice this until now. > > > > > > > Just storing > > > > the uuid should be doable (assuming this doesn't change during the > > > > lifetime of the buffer), so no need for a callback. > > > > > > Directly storing the uuid doesn't work that well because of > > > synchronization issues. The uuid needs to be shared between multiple > > > virtio devices with independent command streams, so to prevent races > > > between importing and exporting, the exporting driver can't share the > > > uuid with other drivers until it knows that the device has finished > > > registering the uuid. That requires a round trip to and then back from > > > the device. Using a callback allows the latency from that round trip > > > registration to be hidden. > > > > Uh, that means you actually do something and there's locking involved. > > Makes stuff more complicated, invariant attributes are a lot easier > > generally. Registering that uuid just always doesn't work, and blocking > > when you're exporting? > > Registering the id at creation and blocking in gem export is feasible, > but it doesn't work well for systems with a centralized buffer > allocator that doesn't support batch allocations (e.g. gralloc). In > such a system, the round trip latency would almost certainly be > included in the buffer allocation time. At least on the system I'm > working on, I suspect that would add 10s of milliseconds of startup > latency to video pipelines (although I haven't benchmarked the > difference). Doing the blocking as late as possible means most or all > of the latency can be hidden behind other pipeline setup work. > > In terms of complexity, I think the synchronization would be basically > the same in either approach, just in different locations. All it would > do is alleviate the need for a callback to fetch the UUID. Hm ok. I guess if we go with the older patch, where this all is a lot more just code in virtio, doing an extra function to allocate the uuid sounds fine. Then synchronization is entirely up to the virtio subsystem and not a dma-buf problem (and hence not mine). You can use dma_resv_lock or so, but no need to. But with callbacks potentially going both ways things always get a bit interesting wrt locking - this is what makes peer2peer dma-buf so painful right now. Hence I'd like to avoid that if needed, at least at the dma-buf level. virtio code I don't mind what you do there :-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/4] dma-buf: add support for virtio exported objects

by Daniel Vetter

On Wed, Mar 11, 2020 at 12:20 PM David Stevens <stevensd(a)chromium.org> wrote: > > This change adds a new dma-buf operation that allows dma-bufs to be used > by virtio drivers to share exported objects. The new operation allows > the importing driver to query the exporting driver for the UUID which > identifies the underlying exported object. > > Signed-off-by: David Stevens <stevensd(a)chromium.org> Adding Tomasz Figa, I've discussed this with him at elce last year I think. Just to make sure. Bunch of things: - obviously we need the users of this in a few drivers, can't really review anything stand-alone - adding very specific ops to the generic interface is rather awkward, eventually everyone wants that and we end up in a mess. I think the best solution here would be if we create a struct virtio_dma_buf which subclasses dma-buf, add a (hopefully safe) runtime upcasting functions, and then a virtio_dma_buf_get_uuid() function. Just storing the uuid should be doable (assuming this doesn't change during the lifetime of the buffer), so no need for a callback. - for the runtime upcasting the usual approach is to check the ->ops pointer. Which means that would need to be the same for all virtio dma_bufs, which might get a bit awkward. But I'd really prefer we not add allocator specific stuff like this to dma-buf. -Daniel > --- > drivers/dma-buf/dma-buf.c | 12 ++++++++++++ > include/linux/dma-buf.h | 18 ++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index d4097856c86b..fa5210ba6aaa 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1158,6 +1158,18 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > > +int dma_buf_get_uuid(struct dma_buf *dmabuf, uuid_t *uuid) > +{ > + if (WARN_ON(!dmabuf) || !uuid) > + return -EINVAL; > + > + if (!dmabuf->ops->get_uuid) > + return -ENODEV; > + > + return dmabuf->ops->get_uuid(dmabuf, uuid); > +} > +EXPORT_SYMBOL_GPL(dma_buf_get_uuid); > + > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index abf5459a5b9d..00758523597d 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -251,6 +251,21 @@ struct dma_buf_ops { > > void *(*vmap)(struct dma_buf *); > void (*vunmap)(struct dma_buf *, void *vaddr); > + > + /** > + * @get_uuid > + * > + * This is called by dma_buf_get_uuid to get the UUID which identifies > + * the buffer to virtio devices. > + * > + * This callback is optional. > + * > + * Returns: > + * > + * 0 on success or a negative error code on failure. On success uuid > + * will be populated with the buffer's UUID. > + */ > + int (*get_uuid)(struct dma_buf *dmabuf, uuid_t *uuid); > }; > > /** > @@ -444,4 +459,7 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > unsigned long); > void *dma_buf_vmap(struct dma_buf *); > void dma_buf_vunmap(struct dma_buf *, void *vaddr); > + > +int dma_buf_get_uuid(struct dma_buf *dmabuf, uuid_t *uuid); > + > #endif /* __DMA_BUF_H__ */ > -- > 2.25.1.481.gfbce0eb801-goog > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years

2
2
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/4] dma-buf: add support for virtio exported objects

by Daniel Vetter

On Thu, May 14, 2020 at 05:19:40PM +0900, David Stevens wrote: > Sorry for the duplicate reply, didn't notice this until now. > > > Just storing > > the uuid should be doable (assuming this doesn't change during the > > lifetime of the buffer), so no need for a callback. > > Directly storing the uuid doesn't work that well because of > synchronization issues. The uuid needs to be shared between multiple > virtio devices with independent command streams, so to prevent races > between importing and exporting, the exporting driver can't share the > uuid with other drivers until it knows that the device has finished > registering the uuid. That requires a round trip to and then back from > the device. Using a callback allows the latency from that round trip > registration to be hidden. Uh, that means you actually do something and there's locking involved. Makes stuff more complicated, invariant attributes are a lot easier generally. Registering that uuid just always doesn't work, and blocking when you're exporting? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/4] dma-buf: add support for virtio exported objects

by Daniel Vetter

On Thu, May 14, 2020 at 11:08:52AM +0900, David Stevens wrote: > On Thu, May 14, 2020 at 12:45 AM Daniel Vetter <daniel(a)ffwll.ch> wrote: > > On Wed, Mar 11, 2020 at 12:20 PM David Stevens <stevensd(a)chromium.org> wrote: > > > > > > This change adds a new dma-buf operation that allows dma-bufs to be used > > > by virtio drivers to share exported objects. The new operation allows > > > the importing driver to query the exporting driver for the UUID which > > > identifies the underlying exported object. > > > > > > Signed-off-by: David Stevens <stevensd(a)chromium.org> > > > > Adding Tomasz Figa, I've discussed this with him at elce last year I > > think. Just to make sure. > > > > Bunch of things: > > - obviously we need the users of this in a few drivers, can't really > > review anything stand-alone > > Here is a link to the usage of this feature by the currently under > development virtio-video driver: > https://markmail.org/thread/j4xlqaaim266qpks > > > - adding very specific ops to the generic interface is rather awkward, > > eventually everyone wants that and we end up in a mess. I think the > > best solution here would be if we create a struct virtio_dma_buf which > > subclasses dma-buf, add a (hopefully safe) runtime upcasting > > functions, and then a virtio_dma_buf_get_uuid() function. Just storing > > the uuid should be doable (assuming this doesn't change during the > > lifetime of the buffer), so no need for a callback. > > So you would prefer a solution similar to the original version of this > patchset? https://markmail.org/message/z7if4u56q5fmaok4 yup. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote: > > Thank you Greg for the comments. > On 5/12/2020 2:22 PM, Greg KH wrote: > > On Fri, May 08, 2020 at 12:11:03PM +0530, Charan Teja Reddy wrote: > >> The following race occurs while accessing the dmabuf object exported as > >> file: > >> P1 P2 > >> dma_buf_release() dmabuffs_dname() > >> [say lsof reading /proc/<P1 pid>/fd/<num>] > >> > >> read dmabuf stored in dentry->d_fsdata > >> Free the dmabuf object > >> Start accessing the dmabuf structure > >> > >> In the above description, the dmabuf object freed in P1 is being > >> accessed from P2 which is resulting into the use-after-free. Below is > >> the dump stack reported. > >> > >> We are reading the dmabuf object stored in the dentry->d_fsdata but > >> there is no binding between the dentry and the dmabuf which means that > >> the dmabuf can be freed while it is being read from ->d_fsdata and > >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > >> with an extra refcount is not a viable solution as the exported dmabuf > >> is already under file's refcount and keeping the multiple refcounts on > >> the same object coordinated is not possible. > >> > >> As we are reading the dmabuf in ->d_fsdata just to get the user passed > >> name, we can directly store the name in d_fsdata thus can avoid the > >> reading of dmabuf altogether. > >> > >> Call Trace: > >> kasan_report+0x12/0x20 > >> __asan_report_load8_noabort+0x14/0x20 > >> dmabuffs_dname+0x4f4/0x560 > >> tomoyo_realpath_from_path+0x165/0x660 > >> tomoyo_get_realpath > >> tomoyo_check_open_permission+0x2a3/0x3e0 > >> tomoyo_file_open > >> tomoyo_file_open+0xa9/0xd0 > >> security_file_open+0x71/0x300 > >> do_dentry_open+0x37a/0x1380 > >> vfs_open+0xa0/0xd0 > >> path_openat+0x12ee/0x3490 > >> do_filp_open+0x192/0x260 > >> do_sys_openat2+0x5eb/0x7e0 > >> do_sys_open+0xf2/0x180 > >> > >> Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > >> Reported-by: syzbot+3643a18836bce555bff6(a)syzkaller.appspotmail.com > >> Cc: <stable(a)vger.kernel.org> [5.3+] > >> Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> > >> --- > >> > >> Changes in v2: > >> > >> - Pass the user passed name in ->d_fsdata instead of dmabuf > >> - Improve the commit message > >> > >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > >> > >> drivers/dma-buf/dma-buf.c | 17 ++++++++++------- > >> 1 file changed, 10 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > >> index 01ce125..0071f7d 100644 > >> --- a/drivers/dma-buf/dma-buf.c > >> +++ b/drivers/dma-buf/dma-buf.c > >> @@ -25,6 +25,7 @@ > >> #include <linux/mm.h> > >> #include <linux/mount.h> > >> #include <linux/pseudo_fs.h> > >> +#include <linux/dcache.h> > >> > >> #include <uapi/linux/dma-buf.h> > >> #include <uapi/linux/magic.h> > >> @@ -40,15 +41,13 @@ struct dma_buf_list { > >> > >> static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) > >> { > >> - struct dma_buf *dmabuf; > >> char name[DMA_BUF_NAME_LEN]; > >> size_t ret = 0; > >> > >> - dmabuf = dentry->d_fsdata; > >> - dma_resv_lock(dmabuf->resv, NULL); > >> - if (dmabuf->name) > >> - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > >> - dma_resv_unlock(dmabuf->resv); > >> + spin_lock(&dentry->d_lock); > > > > Are you sure this lock always protects d_fsdata? > > I think yes. In the dma-buf.c, I have to make sure that d_fsdata should > always be under d_lock thus it will be protected. (In this posted patch > there is one place(in dma_buf_set_name) that is missed, will update this > in V3). > > > > >> + if (dentry->d_fsdata) > >> + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); > >> + spin_unlock(&dentry->d_lock); > >> > >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", > >> dentry->d_name.name, ret > 0 ? name : ""); > > > > If the above check fails the name will be what? How could d_name.name > > be valid but d_fsdata not be valid? > > In case of check fails, empty string "" is appended to the name by the > code, ret > 0 ? name : "", ret is initialized to zero. Thus the name > string will be like "/dmabuf:". So multiple objects can have the same "name" if this happens to multiple ones at once? > Regarding the validity of d_fsdata, we are setting the dmabuf's > dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if > that dmabuf is in the free path. Why are we allowing the name to be set if the dmabuf is on the free path at all? Shouldn't that be the real fix here? > >> @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) > >> static int dma_buf_release(struct inode *inode, struct file *file) > >> { > >> struct dma_buf *dmabuf; > >> + struct dentry *dentry = file->f_path.dentry; > >> > >> if (!is_dma_buf_file(file)) > >> return -EINVAL; > >> > >> dmabuf = file->private_data; > >> > >> + spin_lock(&dentry->d_lock); > >> + dentry->d_fsdata = NULL; > >> + spin_unlock(&dentry->d_lock); > >> BUG_ON(dmabuf->vmapping_counter); > >> > >> /* > >> @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > >> } > >> kfree(dmabuf->name); > >> dmabuf->name = name; > >> + dmabuf->file->f_path.dentry->d_fsdata = name; > > > > You are just changing the use of d_fsdata from being a pointer to the > > dmabuf to being a pointer to the name string? What's to keep that name > > string around and not have the same reference counting issues that the > > dmabuf structure itself has? Who frees that string memory? > > > > Yes, I am just storing the name string in the d_fsdata in place of > dmabuf and this helps to get rid of any extra refcount requirement. > Because the user passed name carried in the d_fsdata is copied to the > local buffer in dmabuffs_dname under spin_lock(d_lock) and the same > d_fsdata is set to NULL(under the d_lock only) when that dmabuf is in > the release path. So, when d_fsdata is NULL, name string is not accessed > from the dmabuffs_dname thus extra count is not required. > > String memory, stored in the dmabuf->name, is released from the > dma_buf_release(). Flow will be like, It fist sets d_fsdata=NULL and > then free the dmabuf->name. > > However from your comments I have realized that there is a race in this > patch when using the name string between dma_buf_set_name() and > dmabuffs_dname(). But, If the idea of passing the name string inplace of > dmabuf in d_fsdata looks fine, I can update this next patch. I'll leave that to the dmabuf authors/maintainers, but it feels odd to me... thanks, greg k-h

5 years

3
2
0 0

[PATCH v4 00/38] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the. drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used a simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. To avoid similar issues in the future, I've introduced a common wrappers for DMA-mapping calls, which operate directly on the sg_table objects. I've also added wrappers for iterating over the scatterlists stored in the sg_table objects and applied them where possible. This, together with some common DRM prime helpers, allowed me to almost get rid of all nents/orig_nents usage in the drivers. I hope that such change makes the code robust, easier to follow and copy/paste safe. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. Patches are based on top of Linux next-20200511. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555. [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Changelog: v4: - added for_each_sgtable_* wrappers and applied where possible - added drm_prime_get_contiguous_size() and applied where possible - applied drm_prime_sg_to_page_addr_arrays() where possible to remove page extraction from sg_table objects - added documentation for the introduced wrappers - improved patches description a bit v3: https://lore.kernel.org/dri-devel/20200505083926.28503-1-m.szyprowski@samsu… - introduce dma_*_sgtable_* wrappers and use them in all patches v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (38): dma-mapping: add generic helpers for mapping sgtable objects scatterlist: add generic wrappers for iterating over sgtable objects iommu: add generic helper for mapping sgtable objects drm: prime: add common helper to check scatterlist contiguity drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays() drm: core: fix common struct sg_table related issues drm: amdgpu: fix common struct sg_table related issues drm: armada: fix common struct sg_table related issues drm: etnaviv: fix common struct sg_table related issues drm: exynos: use common helper for a scatterlist contiguity check drm: exynos: fix common struct sg_table related issues drm: i915: fix common struct sg_table related issues drm: lima: fix common struct sg_table related issues drm: mediatek: use common helper for a scatterlist contiguity check drm: mediatek: use common helper for extracting pages array drm: msm: fix common struct sg_table related issues drm: omapdrm: use common helper for extracting pages array drm: omapdrm: fix common struct sg_table related issues drm: panfrost: fix common struct sg_table related issues drm: radeon: fix common struct sg_table related issues drm: rockchip: use common helper for a scatterlist contiguity check drm: rockchip: fix common struct sg_table related issues drm: tegra: fix common struct sg_table related issues drm: v3d: fix common struct sg_table related issues drm: virtio: fix common struct sg_table related issues drm: vmwgfx: fix common struct sg_table related issues xen: gntdev: fix common struct sg_table related issues drm: host1x: fix common struct sg_table related issues drm: rcar-du: fix common struct sg_table related issues dmabuf: fix common struct sg_table related issues staging: ion: remove dead code staging: ion: fix common struct sg_table related issues staging: tegra-vde: fix common struct sg_table related issues misc: fastrpc: fix common struct sg_table related issues rapidio: fix common struct sg_table related issues samples: vfio-mdev/mbochs: fix common struct sg_table related issues media: pci: fix common ALSA DMA-mapping related codes videobuf2: use sgtable-based scatterlist wrappers drivers/dma-buf/heaps/heap-helpers.c | 13 ++-- drivers/dma-buf/udmabuf.c | 7 +- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 +- drivers/gpu/drm/armada/armada_gem.c | 12 +-- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_cma_helper.c | 23 +----- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 ++-- drivers/gpu/drm/drm_prime.c | 86 ++++++++++++---------- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +--- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +-- drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +----- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +-- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +- drivers/gpu/drm/lima/lima_gem.c | 11 ++- drivers/gpu/drm/lima/lima_vm.c | 5 +- drivers/gpu/drm/mediatek/mtk_drm_gem.c | 34 ++------- drivers/gpu/drm/msm/msm_gem.c | 13 ++-- drivers/gpu/drm/msm/msm_gpummu.c | 14 ++-- drivers/gpu/drm/msm/msm_iommu.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 20 ++--- drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +- drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 42 +++-------- drivers/gpu/drm/tegra/gem.c | 27 +++---- drivers/gpu/drm/tegra/plane.c | 15 ++-- drivers/gpu/drm/v3d/v3d_mmu.c | 17 ++--- drivers/gpu/drm/virtio/virtgpu_object.c | 36 +++++---- drivers/gpu/drm/virtio/virtgpu_vq.c | 12 ++- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +---- drivers/gpu/host1x/job.c | 22 ++---- .../media/common/videobuf2/videobuf2-dma-contig.c | 41 +++++------ drivers/media/common/videobuf2/videobuf2-dma-sg.c | 32 +++----- drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +-- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 8 +- drivers/misc/fastrpc.c | 4 +- drivers/rapidio/devices/rio_mport_cdev.c | 8 +- drivers/staging/android/ion/ion.c | 25 +++---- drivers/staging/android/ion/ion.h | 1 - drivers/staging/android/ion/ion_heap.c | 53 ++++--------- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/staging/media/tegra-vde/iommu.c | 4 +- drivers/xen/gntdev-dmabuf.c | 13 ++-- include/drm/drm_prime.h | 2 + include/linux/dma-mapping.h | 79 ++++++++++++++++++++ include/linux/iommu.h | 16 ++++ include/linux/scatterlist.h | 50 ++++++++++++- samples/vfio-mdev/mbochs.c | 3 +- 56 files changed, 452 insertions(+), 477 deletions(-) -- 1.9.1

5 years

4
49
0 0

Re: [Linaro-mm-sig] [RFC 16/17] drm/amdgpu: gpu recovery does full modesets

by Daniel Vetter

On Tue, May 12, 2020 at 10:10 PM Kazlauskas, Nicholas <nicholas.kazlauskas(a)amd.com> wrote: > > On 2020-05-12 12:12 p.m., Daniel Vetter wrote: > > On Tue, May 12, 2020 at 4:24 PM Alex Deucher <alexdeucher(a)gmail.com> wrote: > >> > >> On Tue, May 12, 2020 at 9:45 AM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > >>> > >>> On Tue, May 12, 2020 at 3:29 PM Alex Deucher <alexdeucher(a)gmail.com> wrote: > >>>> > >>>> On Tue, May 12, 2020 at 9:17 AM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > >>>>> > >>>>> On Tue, May 12, 2020 at 3:12 PM Alex Deucher <alexdeucher(a)gmail.com> wrote: > >>>>>> > >>>>>> On Tue, May 12, 2020 at 8:58 AM Daniel Vetter <daniel(a)ffwll.ch> wrote: > >>>>>>> > >>>>>>> On Tue, May 12, 2020 at 08:54:45AM -0400, Alex Deucher wrote: > >>>>>>>> On Tue, May 12, 2020 at 5:00 AM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > >>>>>>>>> > >>>>>>>>> ... > >>>>>>>>> > >>>>>>>>> I think it's time to stop this little exercise. > >>>>>>>>> > >>>>>>>>> The lockdep splat, for the record: > >>>>>>>>> > >>>>>>>>> [ 132.583381] ====================================================== > >>>>>>>>> [ 132.584091] WARNING: possible circular locking dependency detected > >>>>>>>>> [ 132.584775] 5.7.0-rc3+ #346 Tainted: G W > >>>>>>>>> [ 132.585461] ------------------------------------------------------ > >>>>>>>>> [ 132.586184] kworker/2:3/865 is trying to acquire lock: > >>>>>>>>> [ 132.586857] ffffc90000677c70 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper] > >>>>>>>>> [ 132.587569] > >>>>>>>>> but task is already holding lock: > >>>>>>>>> [ 132.589044] ffffffff82318c80 (dma_fence_map){++++}-{0:0}, at: drm_sched_job_timedout+0x25/0xf0 [gpu_sched] > >>>>>>>>> [ 132.589803] > >>>>>>>>> which lock already depends on the new lock. > >>>>>>>>> > >>>>>>>>> [ 132.592009] > >>>>>>>>> the existing dependency chain (in reverse order) is: > >>>>>>>>> [ 132.593507] > >>>>>>>>> -> #2 (dma_fence_map){++++}-{0:0}: > >>>>>>>>> [ 132.595019] dma_fence_begin_signalling+0x50/0x60 > >>>>>>>>> [ 132.595767] drm_atomic_helper_commit+0xa1/0x180 [drm_kms_helper] > >>>>>>>>> [ 132.596567] drm_client_modeset_commit_atomic+0x1ea/0x250 [drm] > >>>>>>>>> [ 132.597420] drm_client_modeset_commit_locked+0x55/0x190 [drm] > >>>>>>>>> [ 132.598178] drm_client_modeset_commit+0x24/0x40 [drm] > >>>>>>>>> [ 132.598948] drm_fb_helper_restore_fbdev_mode_unlocked+0x4b/0xa0 [drm_kms_helper] > >>>>>>>>> [ 132.599738] drm_fb_helper_set_par+0x30/0x40 [drm_kms_helper] > >>>>>>>>> [ 132.600539] fbcon_init+0x2e8/0x660 > >>>>>>>>> [ 132.601344] visual_init+0xce/0x130 > >>>>>>>>> [ 132.602156] do_bind_con_driver+0x1bc/0x2b0 > >>>>>>>>> [ 132.602970] do_take_over_console+0x115/0x180 > >>>>>>>>> [ 132.603763] do_fbcon_takeover+0x58/0xb0 > >>>>>>>>> [ 132.604564] register_framebuffer+0x1ee/0x300 > >>>>>>>>> [ 132.605369] __drm_fb_helper_initial_config_and_unlock+0x36e/0x520 [drm_kms_helper] > >>>>>>>>> [ 132.606187] amdgpu_fbdev_init+0xb3/0xf0 [amdgpu] > >>>>>>>>> [ 132.607032] amdgpu_device_init.cold+0xe90/0x1677 [amdgpu] > >>>>>>>>> [ 132.607862] amdgpu_driver_load_kms+0x5a/0x200 [amdgpu] > >>>>>>>>> [ 132.608697] amdgpu_pci_probe+0xf7/0x180 [amdgpu] > >>>>>>>>> [ 132.609511] local_pci_probe+0x42/0x80 > >>>>>>>>> [ 132.610324] pci_device_probe+0x104/0x1a0 > >>>>>>>>> [ 132.611130] really_probe+0x147/0x3c0 > >>>>>>>>> [ 132.611939] driver_probe_device+0xb6/0x100 > >>>>>>>>> [ 132.612766] device_driver_attach+0x53/0x60 > >>>>>>>>> [ 132.613593] __driver_attach+0x8c/0x150 > >>>>>>>>> [ 132.614419] bus_for_each_dev+0x7b/0xc0 > >>>>>>>>> [ 132.615249] bus_add_driver+0x14c/0x1f0 > >>>>>>>>> [ 132.616071] driver_register+0x6c/0xc0 > >>>>>>>>> [ 132.616902] do_one_initcall+0x5d/0x2f0 > >>>>>>>>> [ 132.617731] do_init_module+0x5c/0x230 > >>>>>>>>> [ 132.618560] load_module+0x2981/0x2bc0 > >>>>>>>>> [ 132.619391] __do_sys_finit_module+0xaa/0x110 > >>>>>>>>> [ 132.620228] do_syscall_64+0x5a/0x250 > >>>>>>>>> [ 132.621064] entry_SYSCALL_64_after_hwframe+0x49/0xb3 > >>>>>>>>> [ 132.621903] > >>>>>>>>> -> #1 (crtc_ww_class_mutex){+.+.}-{3:3}: > >>>>>>>>> [ 132.623587] __ww_mutex_lock.constprop.0+0xcc/0x10c0 > >>>>>>>>> [ 132.624448] ww_mutex_lock+0x43/0xb0 > >>>>>>>>> [ 132.625315] drm_modeset_lock+0x44/0x120 [drm] > >>>>>>>>> [ 132.626184] drmm_mode_config_init+0x2db/0x8b0 [drm] > >>>>>>>>> [ 132.627098] amdgpu_device_init.cold+0xbd1/0x1677 [amdgpu] > >>>>>>>>> [ 132.628007] amdgpu_driver_load_kms+0x5a/0x200 [amdgpu] > >>>>>>>>> [ 132.628920] amdgpu_pci_probe+0xf7/0x180 [amdgpu] > >>>>>>>>> [ 132.629804] local_pci_probe+0x42/0x80 > >>>>>>>>> [ 132.630690] pci_device_probe+0x104/0x1a0 > >>>>>>>>> [ 132.631583] really_probe+0x147/0x3c0 > >>>>>>>>> [ 132.632479] driver_probe_device+0xb6/0x100 > >>>>>>>>> [ 132.633379] device_driver_attach+0x53/0x60 > >>>>>>>>> [ 132.634275] __driver_attach+0x8c/0x150 > >>>>>>>>> [ 132.635170] bus_for_each_dev+0x7b/0xc0 > >>>>>>>>> [ 132.636069] bus_add_driver+0x14c/0x1f0 > >>>>>>>>> [ 132.636974] driver_register+0x6c/0xc0 > >>>>>>>>> [ 132.637870] do_one_initcall+0x5d/0x2f0 > >>>>>>>>> [ 132.638765] do_init_module+0x5c/0x230 > >>>>>>>>> [ 132.639654] load_module+0x2981/0x2bc0 > >>>>>>>>> [ 132.640522] __do_sys_finit_module+0xaa/0x110 > >>>>>>>>> [ 132.641372] do_syscall_64+0x5a/0x250 > >>>>>>>>> [ 132.642203] entry_SYSCALL_64_after_hwframe+0x49/0xb3 > >>>>>>>>> [ 132.643022] > >>>>>>>>> -> #0 (crtc_ww_class_acquire){+.+.}-{0:0}: > >>>>>>>>> [ 132.644643] __lock_acquire+0x1241/0x23f0 > >>>>>>>>> [ 132.645469] lock_acquire+0xad/0x370 > >>>>>>>>> [ 132.646274] drm_modeset_acquire_init+0xd2/0x100 [drm] > >>>>>>>>> [ 132.647071] drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper] > >>>>>>>>> [ 132.647902] dm_suspend+0x1c/0x60 [amdgpu] > >>>>>>>>> [ 132.648698] amdgpu_device_ip_suspend_phase1+0x83/0xe0 [amdgpu] > >>>>>>>>> [ 132.649498] amdgpu_device_ip_suspend+0x1c/0x60 [amdgpu] > >>>>>>>>> [ 132.650300] amdgpu_device_gpu_recover.cold+0x4e6/0xe64 [amdgpu] > >>>>>>>>> [ 132.651084] amdgpu_job_timedout+0xfb/0x150 [amdgpu] > >>>>>>>>> [ 132.651825] drm_sched_job_timedout+0x8a/0xf0 [gpu_sched] > >>>>>>>>> [ 132.652594] process_one_work+0x23c/0x580 > >>>>>>>>> [ 132.653402] worker_thread+0x50/0x3b0 > >>>>>>>>> [ 132.654139] kthread+0x12e/0x150 > >>>>>>>>> [ 132.654868] ret_from_fork+0x27/0x50 > >>>>>>>>> [ 132.655598] > >>>>>>>>> other info that might help us debug this: > >>>>>>>>> > >>>>>>>>> [ 132.657739] Chain exists of: > >>>>>>>>> crtc_ww_class_acquire --> crtc_ww_class_mutex --> dma_fence_map > >>>>>>>>> > >>>>>>>>> [ 132.659877] Possible unsafe locking scenario: > >>>>>>>>> > >>>>>>>>> [ 132.661416] CPU0 CPU1 > >>>>>>>>> [ 132.662126] ---- ---- > >>>>>>>>> [ 132.662847] lock(dma_fence_map); > >>>>>>>>> [ 132.663574] lock(crtc_ww_class_mutex); > >>>>>>>>> [ 132.664319] lock(dma_fence_map); > >>>>>>>>> [ 132.665063] lock(crtc_ww_class_acquire); > >>>>>>>>> [ 132.665799] > >>>>>>>>> *** DEADLOCK *** > >>>>>>>>> > >>>>>>>>> [ 132.667965] 4 locks held by kworker/2:3/865: > >>>>>>>>> [ 132.668701] #0: ffff8887fb81c938 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1bc/0x580 > >>>>>>>>> [ 132.669462] #1: ffffc90000677e58 ((work_completion)(&(&sched->work_tdr)->work)){+.+.}-{0:0}, at: process_one_work+0x1bc/0x580 > >>>>>>>>> [ 132.670242] #2: ffffffff82318c80 (dma_fence_map){++++}-{0:0}, at: drm_sched_job_timedout+0x25/0xf0 [gpu_sched] > >>>>>>>>> [ 132.671039] #3: ffff8887b84a1748 (&adev->lock_reset){+.+.}-{3:3}, at: amdgpu_device_gpu_recover.cold+0x59e/0xe64 [amdgpu] > >>>>>>>>> [ 132.671902] > >>>>>>>>> stack backtrace: > >>>>>>>>> [ 132.673515] CPU: 2 PID: 865 Comm: kworker/2:3 Tainted: G W 5.7.0-rc3+ #346 > >>>>>>>>> [ 132.674347] Hardware name: System manufacturer System Product Name/PRIME X370-PRO, BIOS 4011 04/19/2018 > >>>>>>>>> [ 132.675194] Workqueue: events drm_sched_job_timedout [gpu_sched] > >>>>>>>>> [ 132.676046] Call Trace: > >>>>>>>>> [ 132.676897] dump_stack+0x8f/0xd0 > >>>>>>>>> [ 132.677748] check_noncircular+0x162/0x180 > >>>>>>>>> [ 132.678604] ? stack_trace_save+0x4b/0x70 > >>>>>>>>> [ 132.679459] __lock_acquire+0x1241/0x23f0 > >>>>>>>>> [ 132.680311] lock_acquire+0xad/0x370 > >>>>>>>>> [ 132.681163] ? drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper] > >>>>>>>>> [ 132.682021] ? cpumask_next+0x16/0x20 > >>>>>>>>> [ 132.682880] ? module_assert_mutex_or_preempt+0x14/0x40 > >>>>>>>>> [ 132.683737] ? __module_address+0x28/0xf0 > >>>>>>>>> [ 132.684601] drm_modeset_acquire_init+0xd2/0x100 [drm] > >>>>>>>>> [ 132.685466] ? drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper] > >>>>>>>>> [ 132.686335] drm_atomic_helper_suspend+0x38/0x120 [drm_kms_helper] > >>>>>>>>> [ 132.687255] dm_suspend+0x1c/0x60 [amdgpu] > >>>>>>>>> [ 132.688152] amdgpu_device_ip_suspend_phase1+0x83/0xe0 [amdgpu] > >>>>>>>>> [ 132.689057] ? amdgpu_fence_process+0x4c/0x150 [amdgpu] > >>>>>>>>> [ 132.689963] amdgpu_device_ip_suspend+0x1c/0x60 [amdgpu] > >>>>>>>>> [ 132.690893] amdgpu_device_gpu_recover.cold+0x4e6/0xe64 [amdgpu] > >>>>>>>>> [ 132.691818] amdgpu_job_timedout+0xfb/0x150 [amdgpu] > >>>>>>>>> [ 132.692707] drm_sched_job_timedout+0x8a/0xf0 [gpu_sched] > >>>>>>>>> [ 132.693597] process_one_work+0x23c/0x580 > >>>>>>>>> [ 132.694487] worker_thread+0x50/0x3b0 > >>>>>>>>> [ 132.695373] ? process_one_work+0x580/0x580 > >>>>>>>>> [ 132.696264] kthread+0x12e/0x150 > >>>>>>>>> [ 132.697154] ? kthread_create_worker_on_cpu+0x70/0x70 > >>>>>>>>> [ 132.698057] ret_from_fork+0x27/0x50 > >>>>>>>>> > >>>>>>>>> Cc: linux-media(a)vger.kernel.org > >>>>>>>>> Cc: linaro-mm-sig(a)lists.linaro.org > >>>>>>>>> Cc: linux-rdma(a)vger.kernel.org > >>>>>>>>> Cc: amd-gfx(a)lists.freedesktop.org > >>>>>>>>> Cc: intel-gfx(a)lists.freedesktop.org > >>>>>>>>> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> > >>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > >>>>>>>>> Cc: Christian König <christian.koenig(a)amd.com> > >>>>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > >>>>>>>>> --- > >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 ++++++++ > >>>>>>>>> 1 file changed, 8 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>>>>>>>> index 3584e29323c0..b3b84a0d3baf 100644 > >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > >>>>>>>>> @@ -2415,6 +2415,14 @@ static int amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev) > >>>>>>>>> /* displays are handled separately */ > >>>>>>>>> if (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_DCE) { > >>>>>>>>> /* XXX handle errors */ > >>>>>>>>> + > >>>>>>>>> + /* > >>>>>>>>> + * This is dm_suspend, which calls modeset locks, and > >>>>>>>>> + * that a pretty good inversion against dma_fence_signal > >>>>>>>>> + * which gpu recovery is supposed to guarantee. > >>>>>>>>> + * > >>>>>>>>> + * Dont ask me how to fix this. > >>>>>>>>> + */ > >>>>>>>> > >>>>>>>> We actually have a fix for this. Will be out shortly. > >>>>>>> > >>>>>>> Spoilers? Solid way is to sidesteck the entire thing by avoiding to reset > >>>>>>> the display block entirely. Fixing the locking while still resetting the > >>>>>>> display is going to be really hard otoh ... > >>>>>> > >>>>>> There's no way to avoid that. On dGPUs at least a full asic reset is > >>>>>> a full asic reset. Mostly just skips the modeset and does the minimum > >>>>>> amount necessary to get the display block into a good state for reset. > >>>>> > >>>>> But how do you restore the display afterwards? "[RFC 13/17] > >>>>> drm/scheduler: use dma-fence annotations in tdr work" earlier in the > >>>>> series has some ideas from me for at least > >>>>> some of the problems for tdr when the display gets reset along. > >>>>> Whacking the display while a modeset/flip/whatever is ongoing > >>>>> concurrently doesn't sound like a good idea, so not sure how you can > >>>>> do that without taking the drm_modeset_locks. And once you do that, > >>>>> it's deadlock time. > >>>> > >>>> We cache the current display hw state and restore it after the reset > >>>> without going through the atomic interfaces so everything is back the > >>>> way it was before the reset. > >>> > >>> Hm this sounds interesting ... how do you make sure a concurrent > >>> atomic update doesn't trample over the same mmio registers while you > >>> do that dance? > >> > >> We take the dm->dc_lock across the reset. > > > > Ok if that's an innermost lock and you don't do any dma_fence_wait() > > while holding that (or anything that somehow depends upon that again) > > I think that should work. From a quick look at current code in > > drm-next that seems to be the case. But would be good to check with my > > annotations whether everything is placed correctly (or maybe there's a > > bug in my annotations). > > > > I still think something like I described in the drm/scheduler patch, > > which would allow us to take drm_modeset_locks in tdr path, would be a > > cleaner and more robust solution longer term. Forcing drivers to do > > their own modeset state looking just doesn't feel that awesome ... I > > guess that also depends upon how many other drivers have this problem. > > > > I worked a bit on analyzing the original problem faced here with using > our regular dm_suspend/dm_resume here for GPU reset and I'm not sure how > taking the drm_modeset_locks really buys us anything. > > We ran into a deadlock caused by an async pageflip commit coming in > along with the TDR suspend commit and the async commit wanted the > dma_fence associated with the plane. The tdr commit suspend commit then > started waiting for the pageflip commit to finish - which never happens > since the fence hasn't been signaled yet and DRM spins forever. > > If the dma_fence is "force" signaled in suspend then the async commit > continues and the suspend commit follows after as normal. The async > commit no longer holds any of the DRM locks because atomic_commit has > already finished - we had no trouble grabbing the locks for the tdr commit. > > Force signaling the fence wasn't really an option for solving this issue > though, and DRM doesn't support aborting atomic commits nor would most > drivers be well equipped to handle this I think - it's no longer really > atomic at that point. > > Ideally we don't actually have that pageflip commit programmed during > the TDR since we'll have to reapply that same state again after, so what > we're stuck with is the solution that we're planning on putting into > driver - hold the internal driver locks across the suspend/reset so > there's no way for the pageflip commit to start processing after having > its fence signaled. > > We're essentially just faking that the state is the exact same as it was > before the GPU reset process started. > > I'm open to suggestions or improvements for handling this but the > easiest solution seemed to just bypass the entire DRM stack under the hood. Yeah gets the job done, plus aside from the locks should also avoid the memory allocations (which I also think are no-go, but much harder to hit). The downside is that it's all custom code, plus you need that internal lock (not really needed in atomic drivers, not sure why you had that to begin with), and if we have a pile of other drivers which have the same problem, might be worth fixing this in helpers itself. One idea I sketched is here: https://patchwork.freedesktop.org/patch/365335/?series=77179&rev=1 But it's incomplete and probably not the best one. Cheers, Daniel > > Regards, > Nicholas Kazlauskas > > >>>> IIRC, when we reset the reset of the > >>>> GPU, we disconnect the fences, and then re-attach them after a reset. > >>> > >>> Where is that code? Since I'm not sure how you can make that work > >>> without getting stuck in another kind of deadlock in tdr. But maybe > >>> the code has some clever trick to pull that off somehow. > >> > >> The GPU scheduler. drm_sched_stop, drm_sched_resubmit_jobs, and > >> drm_sched_start. > > > > That seems to just be about the scheduler-internal fences. That > > tracking you kinda have to throw away and restart with a reset. But I > > don't think these are the fences visible to other places, like in > > dma_resv or drm_syncobj or wherever. > > -Daniel > > > >> Alex > >> > >> > >>> -Daniel > >>> > >>>> > >>>> Alex > >>>> > >>>>> -Daniel > >>>>> -- > >>>>> Daniel Vetter > >>>>> Software Engineer, Intel Corporation > >>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch/ > >>> > >>> > >>> > >>> -- > >>> Daniel Vetter > >>> Software Engineer, Intel Corporation > >>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch/ > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4 38/38] videobuf2: use sgtable-based scatterlist wrappers

by Marek Szyprowski

Hi Michael, On 12.05.2020 19:52, Ruhl, Michael J wrote: >> -----Original Message----- >> From: dri-devel <dri-devel-bounces(a)lists.freedesktop.org> On Behalf Of >> Marek Szyprowski >> Sent: Tuesday, May 12, 2020 5:01 AM >> To: dri-devel(a)lists.freedesktop.org; iommu(a)lists.linux-foundation.org; >> linaro-mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org >> Cc: Pawel Osciak <pawel(a)osciak.com>; Bartlomiej Zolnierkiewicz >> <b.zolnierkie(a)samsung.com>; David Airlie <airlied(a)linux.ie>; linux- >> media(a)vger.kernel.org; Hans Verkuil <hverkuil-cisco(a)xs4all.nl>; Mauro >> Carvalho Chehab <mchehab(a)kernel.org>; Robin Murphy >> <robin.murphy(a)arm.com>; Christoph Hellwig <hch(a)lst.de>; linux-arm- >> kernel(a)lists.infradead.org; Marek Szyprowski >> <m.szyprowski(a)samsung.com> >> Subject: [PATCH v4 38/38] videobuf2: use sgtable-based scatterlist wrappers >> >> Use recently introduced common wrappers operating directly on the struct >> sg_table objects and scatterlist page iterators to make the code a bit >> more compact, robust, easier to follow and copy/paste safe. >> >> No functional change, because the code already properly did all the >> scaterlist related calls. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski(a)samsung.com> >> --- >> For more information, see '[PATCH v4 00/38] DRM: fix struct sg_table nents >> vs. orig_nents misuse' thread: >> https://lore.kernel.org/dri-devel/20200512085710.14688-1- >> m.szyprowski(a)samsung.com/T/ >> --- >> .../media/common/videobuf2/videobuf2-dma-contig.c | 41 ++++++++++---- >> -------- >> drivers/media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++-------- >> -- >> drivers/media/common/videobuf2/videobuf2-vmalloc.c | 12 +++---- >> 3 files changed, 34 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> b/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> index d3a3ee5..bf31a9d 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> @@ -48,16 +48,15 @@ struct vb2_dc_buf { >> >> static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) >> { >> - struct scatterlist *s; >> dma_addr_t expected = sg_dma_address(sgt->sgl); >> - unsigned int i; >> + struct sg_dma_page_iter dma_iter; >> unsigned long size = 0; >> >> - for_each_sg(sgt->sgl, s, sgt->nents, i) { >> - if (sg_dma_address(s) != expected) >> + for_each_sgtable_dma_page(sgt, &dma_iter, 0) { >> + if (sg_page_iter_dma_address(&dma_iter) != expected) >> break; >> - expected = sg_dma_address(s) + sg_dma_len(s); >> - size += sg_dma_len(s); >> + expected += PAGE_SIZE; >> + size += PAGE_SIZE; > This code in drm_prime_t_contiguous_size and here. I seem to remember seeing > the same pattern in other drivers. > > Would it worthwhile to make this a helper as well? I think I've identified such patterns in all DRM drivers and replaced with a common helper. So far I have no idea where to put such helper to make it available for media/videobuf2, so those a few lines are indeed duplicated here. > Also, isn't the sg_dma_len() the actual length of the chunk we are looking at? > > If its I not PAGE_SIZE (ie. dma chunk is 4 * PAGE_SIZE?), does your loop/calculation still work? scaterlist page iterators (for_each_sg_page/for_each_sg_dma_page and their sgtable variants) always operates on PAGE_SIZE units. They correctly handle larger sg_dma_len(). Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

5 years

1
0
0 0

Re: [Linaro-mm-sig] [RFC 02/17] dma-fence: basic lockdep annotations

by Daniel Vetter

On Tue, May 12, 2020 at 09:09:52AM -0300, Jason Gunthorpe wrote: > On Tue, May 12, 2020 at 10:59:29AM +0200, Daniel Vetter wrote: > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 6802125349fb..d5c0fd2efc70 100644 > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num) > > } > > EXPORT_SYMBOL(dma_fence_context_alloc); > > > > +#ifdef CONFIG_LOCKDEP > > +struct lockdep_map dma_fence_lockdep_map = { > > + .name = "dma_fence_map" > > +}; > > + > > +bool dma_fence_begin_signalling(void) > > +{ > > Why is this global? I would have expected it to be connected to a > single fence? It's the same rules for all fences, since they can be shared across drivers in various ways. Lockdep usually achieves that with a static variable hidden in the macro, but that doesn't work if you have lots of different ways from different drivers to create a dma_fence. Hence the unique global one that we explicitly allocate. We have similar stuff for the global dma_resv_lock ww_mutex class, just there it's a bit more standardized and hidden behind a neat macro. But really lockdep needs global lockdep_maps or it doesn't work. > It would also be alot nicer if this was some general lockdep feature, > not tied to dmabuf. This exact problem also strikes anyone using > completions, for instance, and the same solution should be > applicable?? There was: https://lwn.net/Articles/709849/ It even got merged, and seems to have worked. Unfortunately (and I'm not entirely clear on the reasons) it was thrown out again, so we can't use it. That means wait_event/wake_up dependencies need to be manually annotated, like e.g. flush_work() already is. flush_work is more or less where I've stolen this idea from, with some adjustements and tricks on top to make it work for dma_fence users. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years

1
0
0 0

[PATCH v3 00/25] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. To avoid similar issues in the future, I've introduced a common dma-mapping wrappers, which operate directly on the sg_table objects. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. Patches are based on top of Linux next-20200504. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Changelog: v3: - introduce dma_*_sgtable_* wrappers and use them in all patches v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (25): dma-mapping: add generic helpers for mapping sgtable objects drm: core: fix common struct sg_table related issues drm: amdgpu: fix common struct sg_table related issues drm: armada: fix common struct sg_table related issues drm: etnaviv: fix common struct sg_table related issues drm: exynos: fix common struct sg_table related issues drm: i915: fix common struct sg_table related issues drm: lima: fix common struct sg_table related issues drm: msm: fix common struct sg_table related issues drm: panfrost: fix common struct sg_table related issues drm: radeon: fix common struct sg_table related issues drm: rockchip: fix common struct sg_table related issues drm: tegra: fix common struct sg_table related issues drm: virtio: fix common struct sg_table related issues drm: vmwgfx: fix common struct sg_table related issues xen: gntdev: fix common struct sg_table related issues drm: host1x: fix common struct sg_table related issues drm: rcar-du: fix common struct sg_table related issues dmabuf: fix common struct sg_table related issues staging: ion: fix common struct sg_table related issues staging: tegra-vde: fix common struct sg_table related issues misc: fastrpc: fix common struct sg_table related issues rapidio: fix common struct sg_table related issues samples: vfio-mdev/mbochs: fix common struct sg_table related issues media: pci: fix common ALSA DMA-mapping related codes drivers/dma-buf/heaps/heap-helpers.c | 13 +++++----- drivers/dma-buf/udmabuf.c | 7 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +++---- drivers/gpu/drm/armada/armada_gem.c | 10 ++++---- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++---- drivers/gpu/drm/drm_prime.c | 13 +++++----- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++++----- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 +++---- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 ++++------ drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++--- drivers/gpu/drm/lima/lima_gem.c | 11 +++++--- drivers/gpu/drm/msm/msm_gem.c | 13 ++++------ drivers/gpu/drm/msm/msm_iommu.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +-- drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 ++-- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++++---- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 26 +++++++++---------- drivers/gpu/drm/tegra/gem.c | 27 ++++++++------------ drivers/gpu/drm/tegra/plane.c | 15 ++++------- drivers/gpu/drm/virtio/virtgpu_object.c | 17 ++++++------- drivers/gpu/drm/virtio/virtgpu_vq.c | 10 +++----- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +++---------- drivers/gpu/host1x/job.c | 22 ++++++---------- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 9 ++++--- drivers/misc/fastrpc.c | 4 +-- drivers/rapidio/devices/rio_mport_cdev.c | 8 +++--- drivers/staging/android/ion/ion.c | 25 ++++++++---------- drivers/staging/android/ion/ion_heap.c | 6 ++--- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/staging/media/tegra-vde/iommu.c | 4 +-- drivers/xen/gntdev-dmabuf.c | 7 +++--- include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++ include/linux/iommu.h | 6 +++++ samples/vfio-mdev/mbochs.c | 3 ++- 40 files changed, 202 insertions(+), 207 deletions(-) -- 1.9.1

5 years

3
34
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

On Fri, May 08, 2020 at 12:11:03PM +0530, Charan Teja Reddy wrote: > The following race occurs while accessing the dmabuf object exported as > file: > P1 P2 > dma_buf_release() dmabuffs_dname() > [say lsof reading /proc/<P1 pid>/fd/<num>] > > read dmabuf stored in dentry->d_fsdata > Free the dmabuf object > Start accessing the dmabuf structure > > In the above description, the dmabuf object freed in P1 is being > accessed from P2 which is resulting into the use-after-free. Below is > the dump stack reported. > > We are reading the dmabuf object stored in the dentry->d_fsdata but > there is no binding between the dentry and the dmabuf which means that > the dmabuf can be freed while it is being read from ->d_fsdata and > inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > with an extra refcount is not a viable solution as the exported dmabuf > is already under file's refcount and keeping the multiple refcounts on > the same object coordinated is not possible. > > As we are reading the dmabuf in ->d_fsdata just to get the user passed > name, we can directly store the name in d_fsdata thus can avoid the > reading of dmabuf altogether. > > Call Trace: > kasan_report+0x12/0x20 > __asan_report_load8_noabort+0x14/0x20 > dmabuffs_dname+0x4f4/0x560 > tomoyo_realpath_from_path+0x165/0x660 > tomoyo_get_realpath > tomoyo_check_open_permission+0x2a3/0x3e0 > tomoyo_file_open > tomoyo_file_open+0xa9/0xd0 > security_file_open+0x71/0x300 > do_dentry_open+0x37a/0x1380 > vfs_open+0xa0/0xd0 > path_openat+0x12ee/0x3490 > do_filp_open+0x192/0x260 > do_sys_openat2+0x5eb/0x7e0 > do_sys_open+0xf2/0x180 > > Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > Reported-by: syzbot+3643a18836bce555bff6(a)syzkaller.appspotmail.com > Cc: <stable(a)vger.kernel.org> [5.3+] > Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> > --- > > Changes in v2: > > - Pass the user passed name in ->d_fsdata instead of dmabuf > - Improve the commit message > > Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > > drivers/dma-buf/dma-buf.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 01ce125..0071f7d 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -25,6 +25,7 @@ > #include <linux/mm.h> > #include <linux/mount.h> > #include <linux/pseudo_fs.h> > +#include <linux/dcache.h> > > #include <uapi/linux/dma-buf.h> > #include <uapi/linux/magic.h> > @@ -40,15 +41,13 @@ struct dma_buf_list { > > static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) > { > - struct dma_buf *dmabuf; > char name[DMA_BUF_NAME_LEN]; > size_t ret = 0; > > - dmabuf = dentry->d_fsdata; > - dma_resv_lock(dmabuf->resv, NULL); > - if (dmabuf->name) > - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > - dma_resv_unlock(dmabuf->resv); > + spin_lock(&dentry->d_lock); Are you sure this lock always protects d_fsdata? > + if (dentry->d_fsdata) > + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); > + spin_unlock(&dentry->d_lock); > > return dynamic_dname(dentry, buffer, buflen, "/%s:%s", > dentry->d_name.name, ret > 0 ? name : ""); If the above check fails the name will be what? How could d_name.name be valid but d_fsdata not be valid? > @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) > static int dma_buf_release(struct inode *inode, struct file *file) > { > struct dma_buf *dmabuf; > + struct dentry *dentry = file->f_path.dentry; > > if (!is_dma_buf_file(file)) > return -EINVAL; > > dmabuf = file->private_data; > > + spin_lock(&dentry->d_lock); > + dentry->d_fsdata = NULL; > + spin_unlock(&dentry->d_lock); > BUG_ON(dmabuf->vmapping_counter); > > /* > @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > } > kfree(dmabuf->name); > dmabuf->name = name; > + dmabuf->file->f_path.dentry->d_fsdata = name; You are just changing the use of d_fsdata from being a pointer to the dmabuf to being a pointer to the name string? What's to keep that name string around and not have the same reference counting issues that the dmabuf structure itself has? Who frees that string memory? thanks, greg k-h

5 years

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

On Tue, May 12, 2020 at 10:43:18AM +0530, Charan Teja Kalla wrote: > > Ok, but watch out, now you have 2 different reference counts for the > > same structure. Keeping them coordinated is almost always an impossible > > task so you need to only rely on one. If you can't use the file api, > > just drop all of the reference counting logic in there and only use the > > kref one. > > I feel that changing the refcount logic now to dma-buf objects involve > changes in > > the core dma-buf framework. NO? Instead, how about passing the user passed > name directly > > in the ->d_fsdata inplace of dmabuf object? Because we just need user passed > name in the > > dmabuffs_dname(). With this we can avoid the need for extra refcount on > dmabuf. Odd formatting :( > Posted patch-V2: https://lkml.org/lkml/2020/5/8/158 Please just post links to lore.kernel.org, we have no control over lkml.org at all. I'll go review that patch now... greg k-h

5 years

1
0
0 0

Re: [Linaro-mm-sig] [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

by Dave Airlie

On Mon, 11 May 2020 at 19:37, Oded Gabbay <oded.gabbay(a)gmail.com> wrote: > > On Mon, May 11, 2020 at 12:11 PM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > > It's the default. > Thanks for catching that. > > > > > Also so much for "we're not going to tell the graphics people how to > > review their code", dma_fence is a pretty core piece of gpu driver > > infrastructure. And it's very much uapi relevant, including piles of > > corresponding userspace protocols and libraries for how to pass these > > around. > > > > Would be great if habanalabs would not use this (from a quick look > > it's not needed at all), since open source the userspace and playing > > by the usual rules isn't on the table. If that's not possible (because > > it's actually using the uapi part of dma_fence to interact with gpu > > drivers) then we have exactly what everyone promised we'd want to > > avoid. > > We don't use the uapi parts, we currently only using the fencing and > signaling ability of this module inside our kernel code. But maybe I > didn't understand what you request. You want us *not* to use this > well-written piece of kernel code because it is only used by graphics > drivers ? > I'm sorry but I don't get this argument, if this is indeed what you meant. We would rather drivers using a feature that has requirements on correct userspace implementations of the feature have a userspace that is open source and auditable. Fencing is tricky, cross-device fencing is really tricky, and having the ability for a closed userspace component to mess up other people's drivers, think i915 shared with closed habana userspace and shared fences, decreases ability to debug things. Ideally we wouldn't offer users known untested/broken scenarios, so yes we'd prefer that drivers that intend to expose a userspace fencing api around dma-fence would adhere to the rules of the gpu drivers. I'm not say you have to drop using dma-fence, but if you move towards cross-device stuff I believe other drivers would be correct in refusing to interact with fences from here. Dave.

5 years

2
1
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig