Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface

[Paul Cercueil <paul@crapouillou.net>]

Hi Daniel / Sima,

Le mardi 09 janvier 2024 à 14:01 +0100, Daniel Vetter a écrit :
> On Tue, Jan 09, 2024 at 12:06:58PM +0100, Paul Cercueil wrote:
> > Hi Daniel / Sima,
> > 
> > Le lundi 08 janvier 2024 à 20:19 +0100, Daniel Vetter a écrit :
> > > On Mon, Jan 08, 2024 at 05:27:33PM +0100, Paul Cercueil wrote:
> > > > Le lundi 08 janvier 2024 à 16:29 +0100, Daniel Vetter a écrit :
> > > > > On Mon, Jan 08, 2024 at 03:21:21PM +0100, Paul Cercueil
> > > > > wrote:
> > > > > > Hi Daniel (Sima?),
> > > > > > 
> > > > > > Le lundi 08 janvier 2024 à 13:39 +0100, Daniel Vetter a
> > > > > > écrit :
> > > > > > > On Mon, Jan 08, 2024 at 01:00:55PM +0100, Paul Cercueil
> > > > > > > wrote:
> > > > > > > > +static void ffs_dmabuf_signal_done(struct
> > > > > > > > ffs_dma_fence
> > > > > > > > *dma_fence, int ret)
> > > > > > > > +{
> > > > > > > > +	struct ffs_dmabuf_priv *priv = dma_fence-
> > > > > > > > >priv;
> > > > > > > > +	struct dma_fence *fence = &dma_fence->base;
> > > > > > > > +
> > > > > > > > +	dma_fence_get(fence);
> > > > > > > > +	fence->error = ret;
> > > > > > > > +	dma_fence_signal(fence);
> > > > > > > > +
> > > > > > > > +	dma_buf_unmap_attachment(priv->attach,
> > > > > > > > dma_fence-
> > > > > > > > > sgt,
> > > > > > > > dma_fence->dir);
> > > > > > > > +	dma_fence_put(fence);
> > > > > > > > +	ffs_dmabuf_put(priv->attach);
> > > > > > > 
> > > > > > > So this can in theory take the dma_resv lock, and if the
> > > > > > > usb
> > > > > > > completion
> > > > > > > isn't an unlimited worker this could hold up completion
> > > > > > > of
> > > > > > > future
> > > > > > > dma_fence, resulting in a deadlock.
> > > > > > > 
> > > > > > > Needs to be checked how usb works, and if stalling
> > > > > > > indefinitely
> > > > > > > in
> > > > > > > the
> > > > > > > io_complete callback can hold up the usb stack you need
> > > > > > > to:
> > > > > > > 
> > > > > > > - drop a dma_fence_begin/end_signalling annotations in
> > > > > > > here
> > > > > > > - pull out the unref stuff into a separate preallocated
> > > > > > > worker
> > > > > > > (or at
> > > > > > >   least the final unrefs for ffs_dma_buf).
> > > > > > 
> > > > > > Only ffs_dmabuf_put() can attempt to take the dma_resv and
> > > > > > would
> > > > > > have
> > > > > > to be in a worker, right? Everything else would be inside
> > > > > > the
> > > > > > dma_fence_begin/end_signalling() annotations?
> > > > > 
> > > > > Yup. Also I noticed that unlike the iio patches you don't
> > > > > have
> > > > > the
> > > > > dma_buf_unmap here in the completion path (or I'm blind?),
> > > > > which
> > > > > helps a
> > > > > lot with avoiding trouble.
> > > > 
> > > > They both call dma_buf_unmap_attachment() in the "signal done"
> > > > callback, the only difference I see is that it is called after
> > > > the
> > > > dma_fence_put() in the iio patches, while it's called before
> > > > dma_fence_put() here.
> > > 
> > > I was indeed blind ...
> > > 
> > > So the trouble is this wont work because:
> > > - dma_buf_unmap_attachment() requires dma_resv_lock. This is a
> > > somewhat
> > >   recent-ish change from 47e982d5195d ("dma-buf: Move
> > >   dma_buf_map_attachment() to dynamic locking specification"), so
> > > maybe
> > >   old kernel or you don't have full lockdep enabled to get the
> > > right
> > >   splat.
> > > 
> > > - dma_fence critical section forbids dma_resv_lock
> > > 
> > > Which means you need to move this out, but then there's the
> > > potential
> > > cache management issue. Which current gpu drivers just kinda
> > > ignore
> > > because it doesn't matter for current use-case, they all cache
> > > the
> > > mapping
> > > for about as long as the attachment exists. You might want to do
> > > the
> > > same,
> > > unless that somehow breaks a use-case you have, I have no idea
> > > about
> > > that.
> > > If something breaks with unmap_attachment moved out of the fence
> > > handling
> > > then I guess it's high time to add separate cache-management only
> > > to
> > > dma_buf (and that's probably going to be quite some wiring up,
> > > not
> > > sure
> > > even how easy that would be to do nor what exactly the interface
> > > should
> > > look like).
> > 
> > Ok. Then I'll just cache the mapping for now, I think.
> 
> Yeah I think that's simplest. I did ponder a bit and I don't think
> it'd be
> too much pain to add the cache-management functions for device
> attachments/mappings. But it would be quite some typing ...
> -Sima

It looks like I actually do have some hardware which requires the cache
management. If I cache the mappings in both my IIO and USB code, it
works fine on my ZedBoard, but it doesn't work on my ZCU102.

(Or maybe it's something else? What I get from USB in that case is a
stream of zeros, I'd expect it to be more like a stream of
garbage/stale data).

So, change of plans; I will now unmap the attachment in the cleanup
worker after the fence is signalled, and add a warning comment before
the end of the fence critical section about the need to do cache
management before the signal.

Does that work for you?

Cheers,
-Paul