1. 21 Jul, 2020 3 commits
    • Daniel Vetter's avatar
      dma-fence: prime lockdep annotations · d0b9a9ae
      Daniel Vetter authored
      Two in one go:
      - it is allowed to call dma_fence_wait() while holding a
        dma_resv_lock(). This is fundamental to how eviction works with ttm,
        so required.
      
      - it is allowed to call dma_fence_wait() from memory reclaim contexts,
        specifically from shrinker callbacks (which i915 does), and from mmu
        notifier callbacks (which amdgpu does, and which i915 sometimes also
        does, and probably always should, but that's kinda a debate). Also
        for stuff like HMM we really need to be able to do this, or things
        get real dicey.
      
      Consequence is that any critical path necessary to get to a
      dma_fence_signal for a fence must never a) call dma_resv_lock nor b)
      allocate memory with GFP_KERNEL. Also by implication of
      dma_resv_lock(), no userspace faulting allowed. That's some supremely
      obnoxious limitations, which is why we need to sprinkle the right
      annotations to all relevant paths.
      
      The one big locking context we're leaving out here is mmu notifiers,
      added in
      
      commit 23b68395
      Author: Daniel Vetter <daniel.vetter@ffwll.ch>
      Date:   Mon Aug 26 22:14:21 2019 +0200
      
          mm/mmu_notifiers: add a lockdep map for invalidate_range_start/end
      
      that one covers a lot of other callsites, and it's also allowed to
      wait on dma-fences from mmu notifiers. But there's no ready-made
      functions exposed to prime this, so I've left it out for now.
      
      v2: Also track against mmu notifier context.
      
      v3: kerneldoc to spec the cross-driver contract. Note that currently
      i915 throws in a hard-coded 10s timeout on foreign fences (not sure
      why that was done, but it's there), which is why that rule is worded
      with SHOULD instead of MUST.
      
      Also some of the mmu_notifier/shrinker rules might surprise SoC
      drivers, I haven't fully audited them all. Which is infeasible anyway,
      we'll need to run them with lockdep and dma-fence annotations and see
      what goes boom.
      
      v4: A spelling fix from Mika
      
      v5: #ifdef for CONFIG_MMU_NOTIFIER. Reported by 0day. Unfortunately
      this means lockdep enforcement is slightly inconsistent, it won't spot
      GFP_NOIO and GFP_NOFS allocations in the wrong spot if
      CONFIG_MMU_NOTIFIER is disabled in the kernel config. Oh well.
      
      v5: Note that only drivers/gpu has a reasonable (or at least
      historical) excuse to use dma_fence_wait() from shrinker and mmu
      notifier callbacks. Everyone else should either have a better memory
      manager model, or better hardware. This reflects discussions with
      Jason Gunthorpe.
      
      Cc: Jason Gunthorpe <jgg@mellanox.com>
      Cc: Felix Kuehling <Felix.Kuehling@amd.com>
      Cc: kernel test robot <lkp@intel.com>
      Acked-by: default avatarChristian König <christian.koenig@amd.com>
      Acked-by: default avatarDave Airlie <airlied@redhat.com>
      Reviewed-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com> (v4)
      Cc: Mika Kuoppala <mika.kuoppala@intel.com>
      Cc: Thomas Hellstrom <thomas.hellstrom@intel.com>
      Cc: linux-media@vger.kernel.org
      Cc: linaro-mm-sig@lists.linaro.org
      Cc: linux-rdma@vger.kernel.org
      Cc: amd-gfx@lists.freedesktop.org
      Cc: intel-gfx@lists.freedesktop.org
      Cc: Chris Wilson <chris@chris-wilson.co.uk>
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Christian König <christian.koenig@amd.com>
      Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200707201229.472834-3-daniel.vetter@ffwll.ch
      d0b9a9ae
    • Daniel Vetter's avatar
      dma-fence: basic lockdep annotations · 5fbff813
      Daniel Vetter authored
      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 e9149858
        Author: Peter Zijlstra <peterz@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.
      
      Originally I hope that the cross-release lockdep extensions would
      alleviate the need for explicit annotations:
      
      https://lwn.net/Articles/709849/
      
      But there's a few reasons why that's not an option:
      
      - It's not happening in upstream, since it got reverted due to too
        many false positives:
      
      	commit e966eaee
      	Author: Ingo Molnar <mingo@kernel.org>
      	Date:   Tue Dec 12 12:31:16 2017 +0100
      
      	    locking/lockdep: Remove the cross-release locking checks
      
      	    This code (CONFIG_LOCKDEP_CROSSRELEASE=y and CONFIG_LOCKDEP_COMPLETIONS=y),
      	    while it found a number of old bugs initially, was also causing too many
      	    false positives that caused people to disable lockdep - which is arguably
      	    a worse overall outcome.
      
      - cross-release uses the complete() call to annotate the end of
        critical sections, for dma_fence that would be dma_fence_signal().
        But we do not want all dma_fence_signal() calls to be treated as
        critical, since many are opportunistic cleanup of gpu requests. If
        these get stuck there's still the main completion interrupt and
        workers who can unblock everyone. Automatically annotating all
        dma_fence_signal() calls would hence cause false positives.
      
      - cross-release had some educated guesses for when a critical section
        starts, like fresh syscall or fresh work callback. This would again
        cause false positives without explicit annotations, since for
        dma_fence the critical sections only starts when we publish a fence.
      
      - Furthermore there can be cases where a thread never does a
        dma_fence_signal, but is still critical for reaching completion of
        fences. One example would be a scheduler kthread which picks up jobs
        and pushes them into hardware, where the interrupt handler or
        another completion thread calls dma_fence_signal(). But if the
        scheduler thread hangs, then all the fences hang, hence we need to
        manually annotate it. cross-release aimed to solve this by chaining
        cross-release dependencies, but the dependency from scheduler thread
        to the completion interrupt handler goes through hw where
        cross-release code can't observe it.
      
      In short, without manual annotations and careful review of the start
      and end of critical sections, cross-relese dependency tracking doesn't
      work. We need explicit annotations.
      
      v2: handle soft/hardirq ctx better against write side and dont forget
      EXPORT_SYMBOL, drivers can't use this otherwise.
      
      v3: Kerneldoc.
      
      v4: Some spelling fixes from Mika
      
      v5: Amend commit message to explain in detail why cross-release isn't
      the solution.
      
      v6: Pull out misplaced .rst hunk.
      Acked-by: default avatarChristian König <christian.koenig@amd.com>
      Acked-by: default avatarDave Airlie <airlied@redhat.com>
      Cc: Felix Kuehling <Felix.Kuehling@amd.com>
      Reviewed-by: default avatarThomas Hellström <thomas.hellstrom@intel.com>
      Reviewed-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Mika Kuoppala <mika.kuoppala@intel.com>
      Cc: Thomas Hellstrom <thomas.hellstrom@intel.com>
      Cc: linux-media@vger.kernel.org
      Cc: linaro-mm-sig@lists.linaro.org
      Cc: linux-rdma@vger.kernel.org
      Cc: amd-gfx@lists.freedesktop.org
      Cc: intel-gfx@lists.freedesktop.org
      Cc: Chris Wilson <chris@chris-wilson.co.uk>
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Christian König <christian.koenig@amd.com>
      Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
      Link: https://patchwork.freedesktop.org/patch/msgid/20200707201229.472834-2-daniel.vetter@ffwll.ch
      5fbff813
    • Christian König's avatar
      drm/vram-helper: stop using TTM_MEMTYPE_FLAG_MAPPABLE · 23f166ca
      Christian König authored
      The helper doesn't expose any not-mapable memory resources.
      Signed-off-by: default avatarChristian König <christian.koenig@amd.com>
      Reviewed-by: default avatarThomas Zimmermann <tzimmermann@suse.de>
      Link: https://patchwork.freedesktop.org/patch/377649/
      23f166ca
  2. 20 Jul, 2020 12 commits
  3. 19 Jul, 2020 1 commit
  4. 16 Jul, 2020 24 commits