On Thu, Jun 4, 2020 at 10:57 AM Thomas Hellström (Intel)
<thomas_os(a)shipmail.org> wrote:
>
>
> On 6/4/20 10:12 AM, Daniel Vetter wrote:
> ...
> > 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.
>
> Just realized, isn't that example actually a true positive, or at least
> a great candidate for a true positive, since if another thread reenters
> that signaling path, it will block on that mutex, and the fence would
> never be signaled unless there is another signaling path?
Not sure I understand fully, but I think the answer is "it's complicated".
dma_fence are meant to be a DAG (directed acyclic graph). Now it would
be nice to enforce that, and i915 has some attempts to that effect,
but these annotations here don't try to pull off that miracle. I'm
assuming that all the dependencies between dma_fence don't create a
loop, and instead I'm only focusing on deadlocks between dma_fences
and other locks. Usually an async work looks like this:
1. wait for a bunch of dma_fence that we have as dependencies
2. do work (e.g. atomic commit)
3. signal the dma_fence that represents our work
This can happen on the cpu in a kthread or worker, or on the gpu. Now
for reasons you might want to have a per-work mutex or something and
hold that while going through all this, and this is the false positive
I'm thinking off. Of course, if your fences aren't a DAG, or if you're
holding a mutex that's shared with some other work which is part of
your dependency chain, then this goes boom. But it doesn't have to.
I think in general it's best to purely rely on ordering, and remove as
much locking as possible. This is the design behind the atomic modeset
commit code, which is does not take any mutexes in the commit path, at
least not in the helpers. Drivers can still do stuff of course. Then
the only locks you're left with are spinlocks (maybe irq safe ones) to
coordinate with interrupt handlers, workers, handle the wait/wake
queues, manage work/scheduler run queues and all that stuff, and no
spinlocks.
Now for the case where you have something like the below:
thread 1:
dma_fence_begin_signalling()
mutex_lock(a);
dma_fence_wait(b1);
mutex_unlock(a);
dma_fence_signal(b2);
dma_fence_end_signalling();
That's indeed a bit problematic, assuming you're annotating stuff
correctly, and the locking is actually required. I've seen a few of
these, and annotating the properly needs care:
- often the mutex_lock/unlock is not needed, and just gets in the way.
This was the case for the original atomic modeset commit work patches,
which again locked all the modeset locks. But strict ordering of
commit work was all that was needed to make this work, plus making
sure data structure lifetimes are handled correctly too. I think the
tendency to abuse locking to handle lifetime and ordering problems is
fairly common, but it can lead to lots of trouble. Ime all async work
items with the above problematic pattern can be fixed like this.
- other often case is that the dma_fence_begin_signalling() can&should
be pushed down past the mutex_lock, and maybe even past the
dma_fence_wait, depending upon when/how the dma_fence is published.
The fence signalling critical section can still extend past the
mutex_unlock, lockdep and semantics are fine with that (I think at
least). This is more the case for execbuf tails, where you take locks,
set up some async work, publish the fences and then begin to process
these fences (which could just be pushing the work to the job
scheduler, but could also involve running it directly in the userspace
process thread context, but with locks already dropped).
So I wouldn't go out and say these are true positives, just maybe
unecessary locking and over-eager annotations, without any real bugs
in the code.
Or am I completely off the track and you're thinking of something else?
> Although I agree the conclusion is sound: These annotations cannot be
> sprinkled mindlessly over the code.
Yup, that much is for sure.
-Daniel
>
> /Thomas
>
>
>
>
>
>
> >
> > 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
> >
> > Cc: Mika Kuoppala <mika.kuoppala(a)intel.com>
> > Cc: Thomas Hellstrom <thomas.hellstrom(a)intel.com>
> > Cc: linux-media(a)vger.kernel.org
> > Cc: linaro-mm-sig(a)lists.linaro.org
> > Cc: linux-rdma(a)vger.kernel.org
> > Cc: amd-gfx(a)lists.freedesktop.org
> > Cc: intel-gfx(a)lists.freedesktop.org
> > Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
> > Cc: Christian König <christian.koenig(a)amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
> > ---
> > Documentation/driver-api/dma-buf.rst | 12 +-
> > drivers/dma-buf/dma-fence.c | 161 +++++++++++++++++++++++++++
> > include/linux/dma-fence.h | 12 ++
> > 3 files changed, 182 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
> > index 63dec76d1d8d..05d856131140 100644
> > --- a/Documentation/driver-api/dma-buf.rst
> > +++ b/Documentation/driver-api/dma-buf.rst
> > @@ -100,11 +100,11 @@ CPU Access to DMA Buffer Objects
> > .. kernel-doc:: drivers/dma-buf/dma-buf.c
> > :doc: cpu access
> >
> > -Fence Poll Support
> > -~~~~~~~~~~~~~~~~~~
> > +Implicit Fence Poll Support
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > .. kernel-doc:: drivers/dma-buf/dma-buf.c
> > - :doc: fence polling
> > + :doc: implicit fence polling
> >
> > Kernel Functions and Structures Reference
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > @@ -133,6 +133,12 @@ DMA Fences
> > .. kernel-doc:: drivers/dma-buf/dma-fence.c
> > :doc: DMA fences overview
> >
> > +DMA Fence Signalling Annotations
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +.. kernel-doc:: drivers/dma-buf/dma-fence.c
> > + :doc: fence signalling annotation
> > +
> > DMA Fences Functions Reference
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 656e9ac2d028..0005bc002529 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -110,6 +110,160 @@ u64 dma_fence_context_alloc(unsigned num)
> > }
> > EXPORT_SYMBOL(dma_fence_context_alloc);
> >
> > +/**
> > + * DOC: fence signalling annotation
> > + *
> > + * Proving correctness of all the kernel code around &dma_fence through code
> > + * review and testing is tricky for a few reasons:
> > + *
> > + * * It is a cross-driver contract, and therefore all drivers must follow the
> > + * same rules for lock nesting order, calling contexts for various functions
> > + * and anything else significant for in-kernel interfaces. But it is also
> > + * impossible to test all drivers in a single machine, hence brute-force N vs.
> > + * N testing of all combinations is impossible. Even just limiting to the
> > + * possible combinations is infeasible.
> > + *
> > + * * There is an enormous amount of driver code involved. For render drivers
> > + * there's the tail of command submission, after fences are published,
> > + * scheduler code, interrupt and workers to process job completion,
> > + * and timeout, gpu reset and gpu hang recovery code. Plus for integration
> > + * with core mm with have &mmu_notifier, respectively &mmu_interval_notifier,
> > + * and &shrinker. For modesetting drivers there's the commit tail functions
> > + * between when fences for an atomic modeset are published, and when the
> > + * corresponding vblank completes, including any interrupt processing and
> > + * related workers. Auditing all that code, across all drivers, is not
> > + * feasible.
> > + *
> > + * * Due to how many other subsystems are involved and the locking hierarchies
> > + * this pulls in there is extremely thin wiggle-room for driver-specific
> > + * differences. &dma_fence interacts with almost all of the core memory
> > + * handling through page fault handlers via &dma_resv, dma_resv_lock() and
> > + * dma_resv_unlock(). On the other side it also interacts through all
> > + * allocation sites through &mmu_notifier and &shrinker.
> > + *
> > + * Furthermore lockdep does not handle cross-release dependencies, which means
> > + * any deadlocks between dma_fence_wait() and dma_fence_signal() can't be caught
> > + * at runtime with some quick testing. The simplest example is one thread
> > + * waiting on a &dma_fence while holding a lock::
> > + *
> > + * lock(A);
> > + * dma_fence_wait(B);
> > + * unlock(A);
> > + *
> > + * while the other thread is stuck trying to acquire the same lock, which
> > + * prevents it from signalling the fence the previous thread is stuck waiting
> > + * on::
> > + *
> > + * lock(A);
> > + * unlock(A);
> > + * dma_fence_signal(B);
> > + *
> > + * By manually annotating all code relevant to signalling a &dma_fence we can
> > + * teach lockdep about these dependencies, which also helps with the validation
> > + * headache since now lockdep can check all the rules for us::
> > + *
> > + * cookie = dma_fence_begin_signalling();
> > + * lock(A);
> > + * unlock(A);
> > + * dma_fence_signal(B);
> > + * dma_fence_end_signalling(cookie);
> > + *
> > + * For using dma_fence_begin_signalling() and dma_fence_end_signalling() to
> > + * annotate critical sections the following rules need to be observed:
> > + *
> > + * * All code necessary to complete a &dma_fence must be annotated, from the
> > + * point where a fence is accessible to other threads, to the point where
> > + * dma_fence_signal() is called. Un-annotated code can contain deadlock issues,
> > + * and due to the very strict rules and many corner cases it is infeasible to
> > + * catch these just with review or normal stress testing.
> > + *
> > + * * &struct dma_resv deserves a special note, since the readers are only
> > + * protected by rcu. This means the signalling critical section starts as soon
> > + * as the new fences are installed, even before dma_resv_unlock() is called.
> > + *
> > + * * The only exception are fast paths and opportunistic signalling code, which
> > + * calls dma_fence_signal() purely as an optimization, but is not required to
> > + * guarantee completion of a &dma_fence. The usual example is a wait IOCTL
> > + * which calls dma_fence_signal(), while the mandatory completion path goes
> > + * through a hardware interrupt and possible job completion worker.
> > + *
> > + * * To aid composability of code, the annotations can be freely nested, as long
> > + * as the overall locking hierarchy is consistent. The annotations also work
> > + * both in interrupt and process context. Due to implementation details this
> > + * requires that callers pass an opaque cookie from
> > + * dma_fence_begin_signalling() to dma_fence_end_signalling().
> > + *
> > + * * Validation against the cross driver contract is implemented by priming
> > + * lockdep with the relevant hierarchy at boot-up. This means even just
> > + * testing with a single device is enough to validate a driver, at least as
> > + * far as deadlocks with dma_fence_wait() against dma_fence_signal() are
> > + * concerned.
> > + */
> > +#ifdef CONFIG_LOCKDEP
> > +struct lockdep_map dma_fence_lockdep_map = {
> > + .name = "dma_fence_map"
> > +};
> > +
> > +/**
> > + * dma_fence_begin_signalling - begin a critical DMA fence signalling section
> > + *
> > + * Drivers should use this to annotate the beginning of any code section
> > + * required to eventually complete &dma_fence by calling dma_fence_signal().
> > + *
> > + * The end of these critical sections are annotated with
> > + * dma_fence_end_signalling().
> > + *
> > + * Returns:
> > + *
> > + * Opaque cookie needed by the implementation, which needs to be passed to
> > + * dma_fence_end_signalling().
> > + */
> > +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);
> > +
> > +/**
> > + * dma_fence_end_signalling - end a critical DMA fence signalling section
> > + *
> > + * Closes a critical section annotation opened by 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 +324,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);
> > @@ -210,6 +369,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
> >
> > might_sleep();
> >
> > + __dma_fence_might_wait();
> > +
> > trace_dma_fence_wait_start(fence);
> > if (fence->ops->wait)
> > ret = fence->ops->wait(fence, intr, timeout);
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 3347c54f3a87..3f288f7db2ef 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
> > } while (1);
> > }
> >
> > +#ifdef CONFIG_LOCKDEP
> > +bool dma_fence_begin_signalling(void);
> > +void dma_fence_end_signalling(bool cookie);
> > +#else
> > +static inline bool dma_fence_begin_signalling(void)
> > +{
> > + return true;
> > +}
> > +static inline void dma_fence_end_signalling(bool cookie) {}
> > +static inline void __dma_fence_might_wait(void) {}
> > +#endif
> > +
> > int dma_fence_signal(struct dma_fence *fence);
> > int dma_fence_signal_locked(struct dma_fence *fence);
> > signed long dma_fence_default_wait(struct dma_fence *fence,
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
i915 does tons of allocations from this worker, which lockdep catches.
Also generic infrastructure like this with big potential for how
dma_fence or other cross driver contracts work, really should be
reviewed on dri-devel. Implementing custom wheels for everything
within the driver is a classic case of "platform problem" [1]. Which in
upstream we really shouldn't have.
Since there's no quick way to solve these splats (dma_fence_work is
used a bunch in basic buffer management and command submission) like
for amdgpu, I'm giving up at this point here. Annotating i915
scheduler and gpu reset could would be interesting, but since lockdep
is one-shot we can't see what surprises would lurk there.
1: https://lwn.net/Articles/443531/
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: linux-rdma(a)vger.kernel.org
Cc: amd-gfx(a)lists.freedesktop.org
Cc: intel-gfx(a)lists.freedesktop.org
Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/i915/i915_sw_fence_work.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.c b/drivers/gpu/drm/i915/i915_sw_fence_work.c
index a3a81bb8f2c3..5b74acadaef5 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence_work.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence_work.c
@@ -17,12 +17,15 @@ static void fence_work(struct work_struct *work)
{
struct dma_fence_work *f = container_of(work, typeof(*f), work);
int err;
+ bool fence_cookie;
+ fence_cookie = dma_fence_begin_signalling();
err = f->ops->work(f);
if (err)
dma_fence_set_error(&f->dma, err);
fence_complete(f);
+ dma_fence_end_signalling(fence_cookie);
dma_fence_put(&f->dma);
}
--
2.26.2
In the face of unpriviledged userspace being able to submit bogus gpu
workloads the kernel needs gpu timeout and reset (tdr) to guarantee
that dma_fences actually complete. Annotate this worker to make sure
we don't have any accidental locking inversions or other problems
lurking.
Originally this was part of the overall scheduler annotation patch.
But amdgpu has some glorious inversions here:
- grabs console_lock
- does a full modeset, which grabs all kinds of locks
(drm_modeset_lock, dma_resv_lock) which can deadlock with
dma_fence_wait held inside them.
- almost minor at that point, but the modeset code also allocates
memory
These all look like they'll be very hard to fix properly, the hardware
seems to require a full display reset with any gpu recovery.
Hence split out as a seperate patch.
Since amdgpu isn't the only hardware driver that needs to reset the
display (at least gen2/3 on intel have the same problem) we need a
generic solution for this. There's two tricks we could still from
drm/i915 and lift to dma-fence:
- The big whack, aka force-complete all fences. i915 does this for all
pending jobs if the reset is somehow stuck. Trouble is we'd need to
do this for all fences in the entire system, and just the
book-keeping for that will be fun. Plus lots of drivers use fences
for all kinds of internal stuff like memory management, so
unconditionally resetting all of them doesn't work.
I'm also hoping that with these fence annotations we could enlist
lockdep in finding the last offenders causing deadlocks, and we
could remove this get-out-of-jail trick.
- The more feasible approach (across drivers at least as part of the
dma_fence contract) is what drm/i915 does for gen2/3: When we need
to reset the display we wake up all dma_fence_wait_interruptible
calls, or well at least the equivalent of those in i915 internally.
Relying on ioctl restart we force all other threads to release their
locks, which means the tdr thread is guaranteed to be able to get
them. I think we could implement this at the dma_fence level,
including proper lockdep annotations.
dma_fence_begin_tdr():
- must be nested within a dma_fence_begin/end_signalling section
- will wake up all interruptible (but not the non-interruptible)
dma_fence_wait() calls and force them to complete with a
-ERESTARTSYS errno code. All new interrupitble calls to
dma_fence_wait() will immeidately fail with the same error code.
dma_fence_end_trdr():
- this will convert dma_fence_wait() calls back to normal.
Of course interrupting dma_fence_wait is only ok if the caller
specified that, which means we need to split the annotations into
interruptible and non-interruptible version. If we then make sure
that we only use interruptible dma_fence_wait() calls while holding
drm_modeset_lock we can grab them in tdr code, and allow display
resets. Doing the same for dma_resv_lock might be a lot harder, so
buffer updates must be avoided.
What's worse, we're not going to be able to make the dma_fence_wait
calls in mmu-notifiers interruptible, that doesn't work. So
allocating memory still wont' be allowed, even in tdr sections. Plus
obviously we can use this trick only in tdr, it is rather intrusive.
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: linux-rdma(a)vger.kernel.org
Cc: amd-gfx(a)lists.freedesktop.org
Cc: intel-gfx(a)lists.freedesktop.org
Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/scheduler/sched_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 06a736e506ad..e34a44376e87 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -279,9 +279,12 @@ static void drm_sched_job_timedout(struct work_struct *work)
{
struct drm_gpu_scheduler *sched;
struct drm_sched_job *job;
+ bool fence_cookie;
sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
+ fence_cookie = dma_fence_begin_signalling();
+
/* Protects against concurrent deletion in drm_sched_get_cleanup_job */
spin_lock(&sched->job_list_lock);
job = list_first_entry_or_null(&sched->ring_mirror_list,
@@ -313,6 +316,8 @@ static void drm_sched_job_timedout(struct work_struct *work)
spin_lock(&sched->job_list_lock);
drm_sched_start_timeout(sched);
spin_unlock(&sched->job_list_lock);
+
+ dma_fence_end_signalling(fence_cookie);
}
/**
--
2.26.2
Trying to grab dma_resv_lock while in commit_tail before we've done
all the code that leads to the eventual signalling of the vblank event
(which can be a dma_fence) is deadlock-y. Don't do that.
Here the solution is easy because just grabbing locks to read
something races anyway. We don't need to bother, READ_ONCE is
equivalent. And avoids the locking issue.
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: linux-rdma(a)vger.kernel.org
Cc: amd-gfx(a)lists.freedesktop.org
Cc: intel-gfx(a)lists.freedesktop.org
Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c575e7394d03..04c11443b9ca 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6910,7 +6910,11 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
* explicitly on fences instead
* and in general should be called for
* blocking commit to as per framework helpers
+ *
+ * Yes, this deadlocks, since you're calling dma_resv_lock in a
+ * path that leads to a dma_fence_signal(). Don't do that.
*/
+#if 0
r = amdgpu_bo_reserve(abo, true);
if (unlikely(r != 0))
DRM_ERROR("failed to reserve buffer before flip\n");
@@ -6920,6 +6924,12 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
tmz_surface = amdgpu_bo_encrypted(abo);
amdgpu_bo_unreserve(abo);
+#endif
+ /*
+ * this races anyway, so READ_ONCE isn't any better or worse
+ * than the stuff above. Except the stuff above can deadlock.
+ */
+ tiling_flags = READ_ONCE(abo->tiling_flags);
fill_dc_plane_info_and_addr(
dm->adev, new_plane_state, tiling_flags,
--
2.26.2
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(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: linux-rdma(a)vger.kernel.org
Cc: amd-gfx(a)lists.freedesktop.org
Cc: intel-gfx(a)lists.freedesktop.org
Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_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;
--
2.26.2
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(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: linux-rdma(a)vger.kernel.org
Cc: amd-gfx(a)lists.freedesktop.org
Cc: intel-gfx(a)lists.freedesktop.org
Cc: Chris Wilson <chris(a)chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
Cc: Christian König <christian.koenig(a)amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_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 a25fb59c127c..e109666aec14 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1212,6 +1212,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;
@@ -1226,6 +1227,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.
*/
@@ -1262,12 +1265,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);
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:
--
2.26.2