Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v12

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Tue, Jun 25, 2019 at 05:13:42PM +0200, Christian König wrote:
> Am 25.06.19 um 17:07 schrieb Daniel Vetter:
> > On Tue, Jun 25, 2019 at 4:45 PM Christian König
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > > -    if (attach->sgt)
> > > > > +    if (attach->sgt) {
> > > > > +            if (dma_buf_is_dynamic(attach->dmabuf))
> > > > > +                    reservation_object_lock(attach->dmabuf->resv, NULL);
> > > > > +
> > > > >               dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir);
> > > > > 
> > > > > +            if (dma_buf_is_dynamic(attach->dmabuf)) {
> > > > > +                    dma_buf_unpin(attach);
> > > > > +                    reservation_object_unlock(attach->dmabuf->resv);
> > > > > +            }
> > > > > +    }
> > > > > +
> > > > >       mutex_lock(&dmabuf->lock);
> > > > Time to ditch dmabuf->lock in favour of the reservation obj? We have a
> > > > fallback resv_obj in struct dma_buf already, so this is never null, and I
> > > > think would clean up the code a bit.
> > > Yeah, thought about that as well. But then decided against it for now.
> > > 
> > > Key point is that exporters currently doesn't care about dmabuf->lock,
> > > but they do care about the reservation lock.
> > > 
> > > So we will probably have a bunch of cases where we have to fix up
> > > exporters because they will try to grab the reservation lock as well.
> > > 
> > > On the other hand we maybe not need a lock at all here if we just can
> > > live with multiple attach/detach callbacks running in parallel.
> > Well looking through the code I thought that all the places you grab
> > dmabuf->resv we also grab dmabuf->lock, so seemed fully redundant. But
> > I didn't check completely.
> 
> The difference is that I didn't call attach/detach with the lock held yet.
> 
> And we could actually drop locking the reservation object while manipulating
> the list of attachments since we no longer use that workaround.
> 
> Going to fix that up tomorrow as well,

You rely on the reservation lock for walking the list when calling
move_notify. So that lock is definitely needed there. If you'd put it in
there then lockdep would splat about locking inversions.

In general dmabuf->lock not being a ww_mutex kinda gets in the way and
just needless complicates the hierarchy. Atm its even outside of the
reservation lock, so anywhere where we expect callers to hold the
reservation lock already we can't take it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx