On Tue, Jun 22, 2021 at 3:07 PM Christian König <ckoenig.leichtzumerken@xxxxxxxxx> 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@xxxxxxx> > > CC: stable@xxxxxxxxxxxxxxx > > --- > > 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
