Am 12.05.20 um 10:59 schrieb Daniel Vetter:

But only for non-zero timeout, to avoid false positives.

One question here is whether the might_sleep should be unconditional, or only for real timeouts. I'm not sure, so went with the more defensive option. But in the interest of locking down the cross-driver dma_fence rules we might want to be more aggressive.

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: Daniel Vetter daniel.vetter@intel.com

---

drivers/dma-buf/dma-fence.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 052a41e2451c..6802125349fb 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) if (WARN_ON(timeout < 0)) return -EINVAL;

• if (timeout > 0)

• might_sleep();
  

I would rather like to see might_sleep() called here all the time even with timeout==0.

IIRC I removed the code in TTM abusing this in atomic context quite a while ago, but could be that some leaked in again or it is called in atomic context elsewhere as well.

Christian.

trace_dma_fence_wait_start(fence); if (fence->ops->wait) ret = fence->ops->wait(fence, intr, timeout);

On Tue, May 12, 2020 at 10:04:22AM +0100, Chris Wilson wrote:

Quoting Daniel Vetter (2020-05-12 09:59:29)

...

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@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@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: Daniel Vetter daniel.vetter@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"
  

+};

Not another false global sharing lockmap.

It's a global contract, it needs a global lockdep map. And yes a big reason for the motivation here is that i915-gem has a tremendous urge to just redefine all these global locks to fit to some local interpretation of what's going on.

That doesn't make the resulting real&existing deadlocks go away. -Daniel

On Tue, May 12, 2020 at 11:19 AM Chris Wilson chris@chris-wilson.co.uk wrote:

Quoting Daniel Vetter (2020-05-12 10:08:47)

...

On Tue, May 12, 2020 at 10:04:22AM +0100, Chris Wilson wrote:

...

Quoting Daniel Vetter (2020-05-12 09:59:29)

...

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@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@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: Daniel Vetter daniel.vetter@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"
  

+};

Not another false global sharing lockmap.

It's a global contract, it needs a global lockdep map. And yes a big reason for the motivation here is that i915-gem has a tremendous urge to just redefine all these global locks to fit to some local interpretation of what's going on.

No, you can build the global contract out of the actual contracts between fence drivers. If you introduce a struct lockdep_map *map into the fence_ops (so the fence_ops can remain const), you gain correctness at the cost of having to run through all possible interactions once. You can also then do if ops->lockmap ?: &global_fence_lockmap for piecemeal conversion of drivers that do not already use lockmaps for contract enforcement of their fence waits.

I'm not quite sure whether you're actually proposing to have locking contracts per drivers, since that seems rather out of ... I dunno. But if that's what you want, that just doesn't make any sense at all:

- Locking is rather core to kernel programming, aside from a few other things like hard/softirq/preempt/... disabled sections and how recursion works for these, or where and what you're allowed to allocate memory. Lockdep, might_sleep and a bunch of other such debug checks help us enforce that. If you instead go with every driver does what they please yolo, then you don't have an abstraction, all you have is smashing a rose and rose and Rose into one thing because they have the same 4 letter name. It's just an interface that can be used only when understanding every single implementation in detail - really not something that's an abstraction. Yes I've seen some of these dubious abstractions in i915, merged fairly recently, that doesn't make them a good idea.

- You need to test the full NxN matrix (yes you need to test the driver against itself in this world, since testing against something fake like vgem doesn't cut it). That's nuts. Strike that, that's impossible.

- Review is impossible, because the documentation can be summed up as "yolo". Without clear rules all review can do is check every code against every other piece of code, on every change. That's impossible, because we humans are mere mortals, and we're left with push&pray engineering, which really isn't.

The other issue with this approach is that it's full on platform problem in extremis. Instead of extending the shared abstraction or adding new useful functionality, i915-gem has resorted to reinpreting rules to fix local problems. That leads to stuff like roughly

if (mutex_lock_timeout(HZ*10) == -ETIME) { /* I guess we deadlocked, try to bail out */ }

except it's for fences. That's neither solid engineering - we don't generally let the kernel deadlock on itself to test whether maybe it was a deadlock or not, nor is this solid upstreaming in a open source project - we fix the problems where they are, not work around them just in our own driver. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Op 12-05-2020 om 10:59 schreef Daniel Vetter:

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@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@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: Daniel Vetter daniel.vetter@intel.com

---

This is something we definitely need, all drivers need to follow the same rules, in order to put some light in the darkness. :)

Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.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);

+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,

On Wed, May 13, 2020 at 9:02 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:

Am 12.05.20 um 10:59 schrieb Daniel Vetter:

...

This is a bit tricky, since ->notifier_lock is held while calling dma_fence_wait we must ensure that also the read side (i.e. dma_fence_begin_signalling) is on the same side. If we mix this up lockdep complaints, and that's again why we want to have these annotations.

A nice side effect of this is that because of the fs_reclaim priming for dma_fence_enable lockdep now automatically checks for us that nothing in here allocates memory, without even running any userptr workloads.

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: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 7653f62b1b2d..6db3f3c629b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1213,6 +1213,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, struct amdgpu_job *job; uint64_t seq; int r;

• bool fence_cookie;
  
  job = p->job;
  p->job = NULL;
  

@@ -1227,6 +1228,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, */ mutex_lock(&p->adev->notifier_lock);

• fence_cookie = dma_fence_begin_signalling();
  

• /* If userptr are invalidated after amdgpu_cs_parser_bos(), return
   * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl.
   */
  

@@ -1264,12 +1267,14 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);

  ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);

• dma_fence_end_signalling(fence_cookie);
  

Mhm, this could come earlier in theory. E.g. after pushing the job to the scheduler.

Yeah, I have not much clue about how amdgpu works :-) In practice it doesn't matter much, since the enclosing adev->notifier_lock is a lot more strict about what it allows than the dma_fence signalling fake lock. -Daniel

Christian.

...

  mutex_unlock(&p->adev->notifier_lock);

  return 0;

error_abort: drm_sched_job_cleanup(&job->base);

• dma_fence_end_signalling(fence_cookie);
  mutex_unlock(&p->adev->notifier_lock);
  
  

  error_unlock:

Hui what? Of hand that doesn't looks correct to me.

Why the heck should this be an atomic context? If that's correct allocating memory is the least of the problems we have.

Regards, Christian.

Am 12.05.20 um 10:59 schrieb Daniel Vetter:

My dma-fence lockdep annotations caught an inversion because we allocate memory where we really shouldn't:

kmem_cache_alloc+0x2b/0x6d0 amdgpu_fence_emit+0x30/0x330 [amdgpu] amdgpu_ib_schedule+0x306/0x550 [amdgpu] amdgpu_job_run+0x10f/0x260 [amdgpu] drm_sched_main+0x1b9/0x490 [gpu_sched] kthread+0x12e/0x150

Trouble right now is that lockdep only validates against GFP_FS, which would be good enough for shrinkers. But for mmu_notifiers we actually need !GFP_ATOMIC, since they can be called from any page laundering, even if GFP_NOFS or GFP_NOIO are set.

I guess we should improve the lockdep annotations for fs_reclaim_acquire/release.

Ofc real fix is to properly preallocate this fence and stuff it into the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of the way.

v2: Two more allocations in scheduler paths.

Frist one:

__kmalloc+0x58/0x720 amdgpu_vmid_grab+0x100/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched]

Second one:

kmem_cache_alloc+0x2b/0x6d0 amdgpu_sync_fence+0x7e/0x110 [amdgpu] amdgpu_vmid_grab+0x86b/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched]

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: Daniel Vetter daniel.vetter@intel.com

---

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 +- 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index d878fe7fee51..055b47241bb1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -143,7 +143,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, uint32_t seq; int r;

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_ATOMIC); if (fence == NULL) return -ENOMEM;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index fe92dcd94d4a..fdcd6659f5ad 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -208,7 +208,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm, if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait)) return amdgpu_sync_fence(sync, ring->vmid_wait, false);

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_ATOMIC); if (!fences) return -ENOMEM;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index b87ca171986a..330476cc0c86 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -168,7 +168,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f, if (amdgpu_sync_add_later(sync, f, explicit)) return 0;

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_KERNEL);

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_ATOMIC); if (!e) return -ENOMEM;

On Tue, May 12, 2020 at 6:20 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:

On Tue, May 12, 2020 at 5:56 PM Christian König christian.koenig@amd.com wrote:

...

Hui what? Of hand that doesn't looks correct to me.

It's not GFP_ATOMIC, it's just that GFP_ATOMIC is the only shotgun we have to avoid direct reclaim. And direct reclaim might need to call into your mmu notifier, which might need to wait on a fence, which is never going to happen because your scheduler is stuck.

Note that all the explanations for the deadlocks and stuff I'm trying to hunt here are in the other patches, the driver ones are more informational, so I left these here rather bare-bones to shut up lockdep so I can get through the entire driver and all major areas (scheduler, reset, modeset code).

Now you can do something like GFP_NOFS, but the only reasons that works is because the direct reclaim annotations (fs_reclaim_acquire/release) only validates against __GFP_FS, and not against any of the other flags. We should probably add some lockdep annotations so that __GFP_RECLAIM is annotated against the __mmu_notifier_invalidate_range_start_map lockdep map I've recently added for mmu notifiers. End result (assuming I'm not mixing anything up here, this is all rather tricky stuff): GFP_ATOMIC is the only kind of memory allocation you can do.

...

Why the heck should this be an atomic context? If that's correct allocating memory is the least of the problems we have.

It's not about atomic, it's !__GFP_RECLAIM. Which more or less is GFP_ATOMIC. Correct fix is probably GFP_ATOMIC + a mempool for the scheduler fixes so that if you can't allocate them for some reason, you at least know that your scheduler should eventually retire retire some of them, which you can then pick up from the mempool to guarantee forward progress.

But I really didn't dig into details of the code, this was just a quick hack.

So sleeping and taking all kinds of locks (but not all, e.g. dma_resv_lock and drm_modeset_lock are no-go) is still totally ok. Just think

#define GFP_NO_DIRECT_RECLAIM GFP_ATOMIC

Maybe slightly different take that's easier to understand: You've already made the observation that anything holding adev->notifier_lock isn't allowed to allocate memory (well GFP_ATOMIC is ok, like here).

Only thing I'm adding is that the situation is a lot worse. Plus the lockdep annotations to help us catch these issues. -Daniel

Cheers, Daniel

...

Regards, Christian.

Am 12.05.20 um 10:59 schrieb Daniel Vetter:

...

My dma-fence lockdep annotations caught an inversion because we allocate memory where we really shouldn't:

  kmem_cache_alloc+0x2b/0x6d0
  amdgpu_fence_emit+0x30/0x330 [amdgpu]
  amdgpu_ib_schedule+0x306/0x550 [amdgpu]
  amdgpu_job_run+0x10f/0x260 [amdgpu]
  drm_sched_main+0x1b9/0x490 [gpu_sched]
  kthread+0x12e/0x150

Trouble right now is that lockdep only validates against GFP_FS, which would be good enough for shrinkers. But for mmu_notifiers we actually need !GFP_ATOMIC, since they can be called from any page laundering, even if GFP_NOFS or GFP_NOIO are set.

I guess we should improve the lockdep annotations for fs_reclaim_acquire/release.

Ofc real fix is to properly preallocate this fence and stuff it into the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of the way.

v2: Two more allocations in scheduler paths.

Frist one:

  __kmalloc+0x58/0x720
  amdgpu_vmid_grab+0x100/0xca0 [amdgpu]
  amdgpu_job_dependency+0xf9/0x120 [amdgpu]
  drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched]
  drm_sched_main+0xf9/0x490 [gpu_sched]

Second one:

  kmem_cache_alloc+0x2b/0x6d0
  amdgpu_sync_fence+0x7e/0x110 [amdgpu]
  amdgpu_vmid_grab+0x86b/0xca0 [amdgpu]
  amdgpu_job_dependency+0xf9/0x120 [amdgpu]
  drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched]
  drm_sched_main+0xf9/0x490 [gpu_sched]

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: Daniel Vetter daniel.vetter@intel.com

---

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 +- 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index d878fe7fee51..055b47241bb1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -143,7 +143,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, uint32_t seq; int r;

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
  

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_ATOMIC);
  if (fence == NULL)
          return -ENOMEM;
  
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index fe92dcd94d4a..fdcd6659f5ad 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -208,7 +208,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm, if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait)) return amdgpu_sync_fence(sync, ring->vmid_wait, false);

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
  

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_ATOMIC);
  if (!fences)
          return -ENOMEM;
  
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index b87ca171986a..330476cc0c86 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -168,7 +168,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f, if (amdgpu_sync_add_later(sync, f, explicit)) return 0;

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_KERNEL);
  

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_ATOMIC);
  if (!e)
          return -ENOMEM;
  
  

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Tue, May 12, 2020 at 7:31 PM Christian König christian.koenig@amd.com wrote:

Ah!

So we can't allocate memory while scheduling anything because it could be that memory reclaim is waiting for the scheduler to finish pushing things to the hardware, right?

Yup, that's my understanding. But like with all things kernel, I'm not sure, so I tried to come up with some annotations. One of them is the memory allocation stuff, but it also did find the modeset/dc related issues in tdr/gpu recovery, so I think overall it's fairly sound. But the memory side definitely needs more discussion (like really the entire thing I'm proposing here, hence rfc).

My rough hope here is that first we figure out the exact current semantics and nail them down in lockdep annotations and kerneldoc. And then we need to figure out how to step-by-step land this, since lots of drivers will have smaller and bigger issues all over.

I tried to backsearch our CI for the memory allocation issue specifically, but unfortunately we're not retaining a whole lot of the full logs because it's so much. But the more general issue of taking locks somewhere in the path towards completing a fence (tail end of CS ioctl, scheduler, tdr, modeset code since that generates fences too for at least android, ...) that are also held while waiting for said fences to complete is fairly common. I've seen those way too often, and up to now lockdep is simply silent about them.

Indeed a nice problem, haven't noticed that one.

It's pretty glorious indeed :-)

Cheers, Daniel

Christian.

Am 12.05.20 um 18:27 schrieb Daniel Vetter:

...

On Tue, May 12, 2020 at 6:20 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Tue, May 12, 2020 at 5:56 PM Christian König christian.koenig@amd.com wrote:

...

Hui what? Of hand that doesn't looks correct to me.

It's not GFP_ATOMIC, it's just that GFP_ATOMIC is the only shotgun we have to avoid direct reclaim. And direct reclaim might need to call into your mmu notifier, which might need to wait on a fence, which is never going to happen because your scheduler is stuck.

Note that all the explanations for the deadlocks and stuff I'm trying to hunt here are in the other patches, the driver ones are more informational, so I left these here rather bare-bones to shut up lockdep so I can get through the entire driver and all major areas (scheduler, reset, modeset code).

Now you can do something like GFP_NOFS, but the only reasons that works is because the direct reclaim annotations (fs_reclaim_acquire/release) only validates against __GFP_FS, and not against any of the other flags. We should probably add some lockdep annotations so that __GFP_RECLAIM is annotated against the __mmu_notifier_invalidate_range_start_map lockdep map I've recently added for mmu notifiers. End result (assuming I'm not mixing anything up here, this is all rather tricky stuff): GFP_ATOMIC is the only kind of memory allocation you can do.

...

Why the heck should this be an atomic context? If that's correct allocating memory is the least of the problems we have.

It's not about atomic, it's !__GFP_RECLAIM. Which more or less is GFP_ATOMIC. Correct fix is probably GFP_ATOMIC + a mempool for the scheduler fixes so that if you can't allocate them for some reason, you at least know that your scheduler should eventually retire retire some of them, which you can then pick up from the mempool to guarantee forward progress.

But I really didn't dig into details of the code, this was just a quick hack.

So sleeping and taking all kinds of locks (but not all, e.g. dma_resv_lock and drm_modeset_lock are no-go) is still totally ok. Just think

#define GFP_NO_DIRECT_RECLAIM GFP_ATOMIC

Maybe slightly different take that's easier to understand: You've already made the observation that anything holding adev->notifier_lock isn't allowed to allocate memory (well GFP_ATOMIC is ok, like here).

Only thing I'm adding is that the situation is a lot worse. Plus the lockdep annotations to help us catch these issues. -Daniel

...

Cheers, Daniel

...

Regards, Christian.

Am 12.05.20 um 10:59 schrieb Daniel Vetter:

...

My dma-fence lockdep annotations caught an inversion because we allocate memory where we really shouldn't:

   kmem_cache_alloc+0x2b/0x6d0
   amdgpu_fence_emit+0x30/0x330 [amdgpu]
   amdgpu_ib_schedule+0x306/0x550 [amdgpu]
   amdgpu_job_run+0x10f/0x260 [amdgpu]
   drm_sched_main+0x1b9/0x490 [gpu_sched]
   kthread+0x12e/0x150

Trouble right now is that lockdep only validates against GFP_FS, which would be good enough for shrinkers. But for mmu_notifiers we actually need !GFP_ATOMIC, since they can be called from any page laundering, even if GFP_NOFS or GFP_NOIO are set.

I guess we should improve the lockdep annotations for fs_reclaim_acquire/release.

Ofc real fix is to properly preallocate this fence and stuff it into the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of the way.

v2: Two more allocations in scheduler paths.

Frist one:

   __kmalloc+0x58/0x720
   amdgpu_vmid_grab+0x100/0xca0 [amdgpu]
   amdgpu_job_dependency+0xf9/0x120 [amdgpu]
   drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched]
   drm_sched_main+0xf9/0x490 [gpu_sched]

Second one:

   kmem_cache_alloc+0x2b/0x6d0
   amdgpu_sync_fence+0x7e/0x110 [amdgpu]
   amdgpu_vmid_grab+0x86b/0xca0 [amdgpu]
   amdgpu_job_dependency+0xf9/0x120 [amdgpu]
   drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched]
   drm_sched_main+0xf9/0x490 [gpu_sched]

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: Daniel Vetter daniel.vetter@intel.com

---

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 +- 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index d878fe7fee51..055b47241bb1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -143,7 +143,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, uint32_t seq; int r;

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
  

• fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_ATOMIC);
   if (fence == NULL)
           return -ENOMEM;
  
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index fe92dcd94d4a..fdcd6659f5ad 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -208,7 +208,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm, if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait)) return amdgpu_sync_fence(sync, ring->vmid_wait, false);

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL);
  

• fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_ATOMIC);
   if (!fences)
           return -ENOMEM;
  
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index b87ca171986a..330476cc0c86 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -168,7 +168,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f, if (amdgpu_sync_add_later(sync, f, explicit)) return 0;

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_KERNEL);
  

• e = kmem_cache_alloc(amdgpu_sync_slab, GFP_ATOMIC);
   if (!e)
           return -ENOMEM;
  
  

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll....

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@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@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: Daniel Vetter daniel.vetter@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 ... -Daniel

On Tue, May 12, 2020 at 3:12 PM Alex Deucher alexdeucher@gmail.com wrote:

On Tue, May 12, 2020 at 8:58 AM Daniel Vetter daniel@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@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@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: Daniel Vetter daniel.vetter@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. -Daniel

On Tue, May 12, 2020 at 3:29 PM Alex Deucher alexdeucher@gmail.com wrote:

On Tue, May 12, 2020 at 9:17 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Tue, May 12, 2020 at 3:12 PM Alex Deucher alexdeucher@gmail.com wrote:

...

On Tue, May 12, 2020 at 8:58 AM Daniel Vetter daniel@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@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@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: Daniel Vetter daniel.vetter@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?

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. -Daniel

Alex

...

-Daniel

Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Tue, May 12, 2020 at 4:24 PM Alex Deucher alexdeucher@gmail.com wrote:

On Tue, May 12, 2020 at 9:45 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Tue, May 12, 2020 at 3:29 PM Alex Deucher alexdeucher@gmail.com wrote:

...

On Tue, May 12, 2020 at 9:17 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Tue, May 12, 2020 at 3:12 PM Alex Deucher alexdeucher@gmail.com wrote:

...

On Tue, May 12, 2020 at 8:58 AM Daniel Vetter daniel@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@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@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: Daniel Vetter daniel.vetter@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.

...

...

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