From: Daniel Vetter <daniel@ffwll.ch>
To: "Thomas Hellström (VMware)" <thomas_os@shipmail.org>
Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>,
"moderated list:DMA BUFFER SHARING FRAMEWORK"
<linaro-mm-sig@lists.linaro.org>,
intel-gfx <intel-gfx@lists.freedesktop.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
"open list:DMA BUFFER SHARING FRAMEWORK"
<linux-media@vger.kernel.org>
Subject: Re: [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2
Date: Tue, 18 Feb 2020 22:01:58 +0100 [thread overview]
Message-ID: <CAKMK7uHG3EkEPbAQ3UEHHLcfmR+0NPq0wZuBX+s2-WCFdso8ew@mail.gmail.com> (raw)
In-Reply-To: <f8ac7cbc-7c90-7119-735c-9f55adb6fa7f@shipmail.org>
On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware)
<thomas_os@shipmail.org> wrote:
>
> On 2/17/20 6:55 PM, Daniel Vetter wrote:
> > On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:
> >> Implement the importer side of unpinned DMA-buf handling.
> >>
> >> v2: update page tables immediately
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 ++++++++++++++++++++-
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++
> >> 2 files changed, 71 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> index 770baba621b3..48de7624d49c 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> >> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
> >> return ERR_PTR(ret);
> >> }
> >>
> >> +/**
> >> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
> >> + *
> >> + * @attach: the DMA-buf attachment
> >> + *
> >> + * Invalidate the DMA-buf attachment, making sure that the we re-create the
> >> + * mapping before the next use.
> >> + */
> >> +static void
> >> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
> >> +{
> >> + struct drm_gem_object *obj = attach->importer_priv;
> >> + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
> >> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> >> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> >> + struct ttm_operation_ctx ctx = { false, false };
> >> + struct ttm_placement placement = {};
> >> + struct amdgpu_vm_bo_base *bo_base;
> >> + int r;
> >> +
> >> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
> >> + return;
> >> +
> >> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
> >> + if (r) {
> >> + DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
> >> + return;
> >> + }
> >> +
> >> + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
> >> + struct amdgpu_vm *vm = bo_base->vm;
> >> + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
> >> +
> >> + if (ticket) {
> > Yeah so this is kinda why I've been a total pain about the exact semantics
> > of the move_notify hook. I think we should flat-out require that importers
> > _always_ have a ticket attach when they call this, and that they can cope
> > with additional locks being taken (i.e. full EDEADLCK) handling.
> >
> > Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to
> > the dma_resv object, which we then can take #ifdef
> > CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).
> >
> > Now the real disaster is how we handle deadlocks. Two issues:
> >
> > - Ideally we'd keep any lock we've taken locked until the end, it helps
> > needless backoffs. I've played around a bit with that but not even poc
> > level, just an idea:
> >
> > https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27be37331255ab7582
> >
> > Idea is essentially to track a list of objects we had to lock as part of
> > the ttm_bo_validate of the main object.
> >
> > - Second one is if we get a EDEADLCK on one of these sublocks (like the
> > one here). We need to pass that up the entire callchain, including a
> > temporary reference (we have to drop locks to do the ww_mutex_lock_slow
> > call), and need a custom callback to drop that temporary reference
> > (since that's all driver specific, might even be internal ww_mutex and
> > not anything remotely looking like a normal dma_buf). This probably
> > needs the exec util helpers from ttm, but at the dma_resv level, so that
> > we can do something like this:
> >
> > struct dma_resv_ticket {
> > struct ww_acquire_ctx base;
> >
> > /* can be set by anyone (including other drivers) that got hold of
> > * this ticket and had to acquire some new lock. This lock might
> > * protect anything, including driver-internal stuff, and isn't
> > * required to be a dma_buf or even just a dma_resv. */
> > struct ww_mutex *contended_lock;
> >
> > /* callback which the driver (which might be a dma-buf exporter
> > * and not matching the driver that started this locking ticket)
> > * sets together with @contended_lock, for the main driver to drop
> > * when it calls dma_resv_unlock on the contended_lock. */
> > void (drop_ref*)(struct ww_mutex *contended_lock);
> > };
> >
> > This is all supremely nasty (also ttm_bo_validate would need to be
> > improved to handle these sublocks and random new objects that could force
> > a ww_mutex_lock_slow).
> >
> Just a short comment on this:
>
> Neither the currently used wait-die or the wound-wait algorithm
> *strictly* requires a slow lock on the contended lock. For wait-die it's
> just very convenient since it makes us sleep instead of spinning with
> -EDEADLK on the contended lock. For wound-wait IIRC one could just
> immediately restart the whole locking transaction after an -EDEADLK, and
> the transaction would automatically end up waiting on the contended
> lock, provided the mutex lock stealing is not allowed. There is however
> a possibility that the transaction will be wounded again on another
> lock, taken before the contended lock, but I think there are ways to
> improve the wound-wait algorithm to reduce that probability.
>
> So in short, choosing the wound-wait algorithm instead of wait-die and
> perhaps modifying the ww mutex code somewhat would probably help passing
> an -EDEADLK up the call chain without requiring passing the contended
> lock, as long as each locker releases its own locks when receiving an
> -EDEADLK.
Hm this is kinda tempting, since rolling out the full backoff tricker
across driver boundaries is going to be real painful.
What I'm kinda worried about is the debug/validation checks we're
losing with this. The required backoff has this nice property that
ww_mutex debug code can check that we've fully unwound everything when
we should, that we've blocked on the right lock, and that we're
restarting everything without keeling over. Without that I think we
could end up with situations where a driver in the middle feels like
handling the EDEADLCK, which might go well most of the times (the
deadlock will probably be mostly within a given driver, not across).
Right up to the point where someone creates a deadlock across drivers,
and the lack of full rollback will be felt.
So not sure whether we can still keep all these debug/validation
checks, or whether this is a step too far towards clever tricks.
But definitely a neat idea ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2020-02-18 21:02 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-17 15:45 RFC: Unpinned DMA-buf handling Christian König
2020-02-17 15:45 ` [PATCH 1/5] dma-buf: add dynamic DMA-buf handling v14 Christian König
2020-02-17 15:50 ` Christian König
2020-02-17 15:45 ` [PATCH 2/5] drm/ttm: remove the backing store if no placement is given Christian König
2020-02-17 15:45 ` [PATCH 3/5] drm/amdgpu: use allowed_domains for exported DMA-bufs Christian König
2020-02-17 15:45 ` [PATCH 4/5] drm/amdgpu: add amdgpu_dma_buf_pin/unpin v2 Christian König
2020-02-17 15:45 ` [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2 Christian König
2020-02-17 17:55 ` Daniel Vetter
2020-02-17 18:58 ` Christian König
2020-02-17 19:38 ` Daniel Vetter
2020-02-18 20:17 ` Thomas Hellström (VMware)
2020-02-18 21:01 ` Daniel Vetter [this message]
2020-02-19 6:42 ` Thomas Hellström (VMware)
2020-02-20 9:39 ` Thomas Hellström (VMware)
2020-02-20 18:04 ` Daniel Vetter
2020-02-20 19:46 ` Thomas Hellström (VMware)
2020-02-20 20:08 ` Daniel Vetter
2020-02-20 22:51 ` Thomas Hellström (VMware)
2020-02-21 17:12 ` Daniel Vetter
2020-02-21 19:45 ` Thomas Hellström (VMware)
2020-02-23 15:45 ` Christian König
2020-02-23 16:54 ` Thomas Hellström (VMware)
2020-02-24 18:46 ` Christian König
2020-02-24 21:11 ` Thomas Hellström (VMware)
2020-02-25 17:16 ` Daniel Vetter
2020-02-26 16:32 ` Daniel Vetter
2020-02-27 9:20 ` Christian König
2020-02-27 9:38 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKMK7uHG3EkEPbAQ3UEHHLcfmR+0NPq0wZuBX+s2-WCFdso8ew@mail.gmail.com \
--to=daniel@ffwll.ch \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-media@vger.kernel.org \
--cc=thomas_os@shipmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).