Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface | From | Paul Cercueil <> | Date | Tue, 09 Jan 2024 12:06:58 +0100 |
| |
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.
> Cheers, Sima
Cheers, -Paul
| ![\](/images/icornerr.gif) |