Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
3027 discussions

[PATCH 09/18] drm/scheduler: use dma-fence annotations in main thread

by Daniel Vetter

If the scheduler rt thread gets stuck on a mutex that we're holding while waiting for gpu workloads to complete, we have a problem. Add dma-fence annotations so that lockdep can check this for us. I've tried to quite carefully review this, and I think it's at the right spot. But obviosly no expert on drm scheduler. 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/scheduler/sched_main.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 2f319102ae9f..06a736e506ad 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -763,9 +763,12 @@ static int drm_sched_main(void *param) struct sched_param sparam = {.sched_priority = 1}; struct drm_gpu_scheduler *sched = (struct drm_gpu_scheduler *)param; int r; + bool fence_cookie; sched_setscheduler(current, SCHED_FIFO, &sparam); + fence_cookie = dma_fence_begin_signalling(); + while (!kthread_should_stop()) { struct drm_sched_entity *entity = NULL; struct drm_sched_fence *s_fence; @@ -823,6 +826,9 @@ static int drm_sched_main(void *param) wake_up(&sched->job_scheduled); } + + dma_fence_end_signalling(fence_cookie); + return 0; } -- 2.26.2

5 years, 3 months

[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, 3 months

[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, 3 months

[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, 3 months

[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, 3 months

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, 3 months

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, 3 months

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, 3 months

[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, 3 months

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, 3 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig