Re: [PATCH 2/2] dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Fri, Aug 16, 2019 at 9:02 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 16.08.19 um 17:21 schrieb Chris Wilson:
> > Currently dma_fence_signal() tries to avoid the spinlock and only takes
> > it if absolutely required to walk the callback list. However, to allow
> > for some users to surreptitiously insert lazy signal callbacks that
> > do not depend on enabling the signaling mechanism around every fence,
> > we always need to notify the callbacks on signaling. As such, we will
> > always need to take the spinlock and dma_fence_signal() effectively
> > becomes a clone of dma_fence_signal_locked().
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >   drivers/dma-buf/dma-fence.c | 19 +++++--------------
> >   1 file changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index ff0cd6eae766..f23eb9f19b4e 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -176,6 +176,7 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
> >   int dma_fence_signal(struct dma_fence *fence)
> >   {
> >       unsigned long flags;
> > +     int ret;
> >
> >       if (!fence)
> >               return -EINVAL;
> > @@ -183,21 +184,11 @@ int dma_fence_signal(struct dma_fence *fence)
> >       if (test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >               return -EINVAL;
>
> I need to take my review back. You also need to drop this
> test_and_set_bit here or your completely break drivers using this.

Time for a pile of dma_fence selftests? Maybe with a bit of dma_resv
thrown in for good? This stuff is tricky, and it feels like we had a
few oopsies in review recently ...
-Daniel

> Regards,
> Christian.
>
> >
> > -     fence->timestamp = ktime_get();
> > -     set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
> > -     trace_dma_fence_signaled(fence);
> > -
> > -     if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
> > -             struct dma_fence_cb *cur, *tmp;
> > +     spin_lock_irqsave(fence->lock, flags);
> > +     ret = dma_fence_signal_locked(fence);
> > +     spin_unlock_irqrestore(fence->lock, flags);
> >
> > -             spin_lock_irqsave(fence->lock, flags);
> > -             list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> > -                     list_del_init(&cur->node);
> > -                     cur->func(fence, cur);
> > -             }
> > -             spin_unlock_irqrestore(fence->lock, flags);
> > -     }
> > -     return 0;
> > +     return ret;
> >   }
> >   EXPORT_SYMBOL(dma_fence_signal);
> >
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel