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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch