[PATCH] dma-buf: fix and rework dma_buf_poll v3
Daniel Vetter
daniel.vetter at ffwll.ch
Tue Jun 22 17:02:39 UTC 2021
On Tue, Jun 22, 2021 at 3:07 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Crap, hit enter to early before adding a cover letter.
>
> This is the same patch as before, but as requested I'm keeping the
> exclusive fence handling as it is for now.
>
> Daniel can you double check this and/or make sure that it is tested?
>
> I only smoke tested it and the code is so complicated that I'm not sure
> I catched all side effects.
So I've thought about this some more, and we actually have docs for
how this is supposed to work:
https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#implicit-fence-poll-support
Docs are pretty clear that we want both read and write for EPOLLOUT or
well both exclusive and shared fences. So I guess back to your actual
thing, but maybe we should get some acks from userspace people for it
(Michel, Pekka, Simon probably usual suspects).
The other thing I brought up and I haven't seen you reply to (maybe
missed it) is whether we shouldn't just use dma_resv_get_fences(). We
need to do the refcounting anyway, and this avoids us having to
open-code this very nasty code. Finally, and imo most important, this
is what's also used in drm_gem_fence_array_add_implicit(), which many
drivers use to handle their implicit in-fences. So would be nice to
have exactly matching code between that and what dma-buf poll does for
"can I read" and "can I write".
Thoughts?
-Daniel
>
> Regards,
> Christian.
>
> Am 22.06.21 um 15:04 schrieb Christian König:
> > Daniel pointed me towards this function and there are multiple obvious problems
> > in the implementation.
> >
> > First of all the retry loop is not working as intended. In general the retry
> > makes only sense if you grab the reference first and then check the sequence
> > values.
> >
> > It's also good practice to keep the reference around when installing callbacks
> > to fences you don't own.
> >
> > And last the whole implementation was unnecessary complex and rather hard to
> > understand which could lead to probably unexpected behavior of the IOCTL.
> >
> > Fix all this by reworking the implementation from scratch.
> >
> > Only mildly tested and needs a thoughtful review of the code.
> >
> > v2: fix the reference counting as well
> > v3: keep the excl fence handling as is for stable
> >
> > Signed-off-by: Christian König <christian.koenig at amd.com>
> > CC: stable at vger.kernel.org
> > ---
> > drivers/dma-buf/dma-buf.c | 133 ++++++++++++++++----------------------
> > include/linux/dma-buf.h | 2 +-
> > 2 files changed, 55 insertions(+), 80 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index eadd1eaa2fb5..e97c3a9d98d5 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry)
> > * If you hit this BUG() it means someone dropped their ref to the
> > * dma-buf while still having pending operation to the buffer.
> > */
> > - BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
> > + BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
> >
> > dmabuf->ops->release(dmabuf);
> >
> > @@ -202,16 +202,20 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> > wake_up_locked_poll(dcb->poll, dcb->active);
> > dcb->active = 0;
> > spin_unlock_irqrestore(&dcb->poll->lock, flags);
> > + dma_fence_put(fence);
> > }
> >
> > static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> > {
> > + struct dma_buf_poll_cb_t *dcb;
> > struct dma_buf *dmabuf;
> > struct dma_resv *resv;
> > struct dma_resv_list *fobj;
> > struct dma_fence *fence_excl;
> > - __poll_t events;
> > unsigned shared_count, seq;
> > + struct dma_fence *fence;
> > + __poll_t events;
> > + int r, i;
> >
> > dmabuf = file->private_data;
> > if (!dmabuf || !dmabuf->resv)
> > @@ -225,99 +229,70 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> > if (!events)
> > return 0;
> >
> > + dcb = events & EPOLLOUT ? &dmabuf->cb_out : &dmabuf->cb_in;
> > +
> > + /* Only queue a new one if we are not still waiting for the old one */
> > + spin_lock_irq(&dmabuf->poll.lock);
> > + if (dcb->active)
> > + events = 0;
> > + else
> > + dcb->active = events;
> > + spin_unlock_irq(&dmabuf->poll.lock);
> > + if (!events)
> > + return 0;
> > +
> > retry:
> > seq = read_seqcount_begin(&resv->seq);
> > rcu_read_lock();
> >
> > fobj = rcu_dereference(resv->fence);
> > - if (fobj)
> > + if (fobj && events & EPOLLOUT)
> > shared_count = fobj->shared_count;
> > else
> > shared_count = 0;
> > - fence_excl = rcu_dereference(resv->fence_excl);
> > - if (read_seqcount_retry(&resv->seq, seq)) {
> > - rcu_read_unlock();
> > - goto retry;
> > - }
> >
> > - if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) {
> > - struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
> > - __poll_t pevents = EPOLLIN;
> > -
> > - if (shared_count == 0)
> > - pevents |= EPOLLOUT;
> > -
> > - spin_lock_irq(&dmabuf->poll.lock);
> > - if (dcb->active) {
> > - dcb->active |= pevents;
> > - events &= ~pevents;
> > - } else
> > - dcb->active = pevents;
> > - spin_unlock_irq(&dmabuf->poll.lock);
> > -
> > - if (events & pevents) {
> > - if (!dma_fence_get_rcu(fence_excl)) {
> > - /* force a recheck */
> > - events &= ~pevents;
> > - dma_buf_poll_cb(NULL, &dcb->cb);
> > - } else if (!dma_fence_add_callback(fence_excl, &dcb->cb,
> > - dma_buf_poll_cb)) {
> > - events &= ~pevents;
> > - dma_fence_put(fence_excl);
> > - } else {
> > - /*
> > - * No callback queued, wake up any additional
> > - * waiters.
> > - */
> > - dma_fence_put(fence_excl);
> > - dma_buf_poll_cb(NULL, &dcb->cb);
> > - }
> > + for (i = 0; i < shared_count; ++i) {
> > + fence = rcu_dereference(fobj->shared[i]);
> > + fence = dma_fence_get_rcu(fence);
> > + if (!fence || read_seqcount_retry(&resv->seq, seq)) {
> > + /* Concurrent modify detected, force re-check */
> > + dma_fence_put(fence);
> > + rcu_read_unlock();
> > + goto retry;
> > }
> > - }
> >
> > - if ((events & EPOLLOUT) && shared_count > 0) {
> > - struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared;
> > - int i;
> > -
> > - /* Only queue a new callback if no event has fired yet */
> > - spin_lock_irq(&dmabuf->poll.lock);
> > - if (dcb->active)
> > - events &= ~EPOLLOUT;
> > - else
> > - dcb->active = EPOLLOUT;
> > - spin_unlock_irq(&dmabuf->poll.lock);
> > -
> > - if (!(events & EPOLLOUT))
> > + r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > + if (!r) {
> > + /* Callback queued */
> > + events = 0;
> > goto out;
> > + }
> > + dma_fence_put(fence);
> > + }
> >
> > - for (i = 0; i < shared_count; ++i) {
> > - struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
> > -
> > - if (!dma_fence_get_rcu(fence)) {
> > - /*
> > - * fence refcount dropped to zero, this means
> > - * that fobj has been freed
> > - *
> > - * call dma_buf_poll_cb and force a recheck!
> > - */
> > - events &= ~EPOLLOUT;
> > - dma_buf_poll_cb(NULL, &dcb->cb);
> > - break;
> > - }
> > - if (!dma_fence_add_callback(fence, &dcb->cb,
> > - dma_buf_poll_cb)) {
> > - dma_fence_put(fence);
> > - events &= ~EPOLLOUT;
> > - break;
> > - }
> > + fence = dma_resv_get_excl(resv);
> > + if (fence && shared_count == 0) {
> > + fence = dma_fence_get_rcu(fence);
> > + if (!fence || read_seqcount_retry(&resv->seq, seq)) {
> > + /* Concurrent modify detected, force re-check */
> > dma_fence_put(fence);
> > + rcu_read_unlock();
> > + goto retry;
> > +
> > }
> >
> > - /* No callback queued, wake up any additional waiters. */
> > - if (i == shared_count)
> > - dma_buf_poll_cb(NULL, &dcb->cb);
> > + r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > + if (!r) {
> > + /* Callback queued */
> > + events = 0;
> > + goto out;
> > + }
> > + dma_fence_put(fence_excl);
> > }
> >
> > + /* No callback queued, wake up any additional waiters. */
> > + dma_buf_poll_cb(NULL, &dcb->cb);
> > +
> > out:
> > rcu_read_unlock();
> > return events;
> > @@ -562,8 +537,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
> > dmabuf->owner = exp_info->owner;
> > spin_lock_init(&dmabuf->name_lock);
> > init_waitqueue_head(&dmabuf->poll);
> > - dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
> > - dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
> > + dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
> > + dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
> >
> > if (!resv) {
> > resv = (struct dma_resv *)&dmabuf[1];
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index efdc56b9d95f..7e747ad54c81 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -329,7 +329,7 @@ struct dma_buf {
> > wait_queue_head_t *poll;
> >
> > __poll_t active;
> > - } cb_excl, cb_shared;
> > + } cb_in, cb_out;
> > };
> >
> > /**
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list