Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
On Thu, Jan 18, 2024 at 08:39:23PM +0100, Paul Cercueil wrote:
> Hi Daniel / Sima,
>
> Le jeudi 18 janvier 2024 à 14:59 +0100, Daniel Vetter a écrit :
> > On Thu, Jan 18, 2024 at 02:56:31PM +0100, Daniel Vetter wrote:
> > > On Mon, Jan 15, 2024 at 01:54:27PM +0100, Paul Cercueil wrote:
> > > > 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 = _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
>
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
Hi Daniel / Sima,
Le jeudi 18 janvier 2024 à 14:59 +0100, Daniel Vetter a écrit :
> On Thu, Jan 18, 2024 at 02:56:31PM +0100, Daniel Vetter wrote:
> > On Mon, Jan 15, 2024 at 01:54:27PM +0100, Paul Cercueil wrote:
> > > 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 = _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,
> > >
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
On Thu, Jan 18, 2024 at 02:56:31PM +0100, Daniel Vetter wrote:
> On Mon, Jan 15, 2024 at 01:54:27PM +0100, Paul Cercueil wrote:
> > 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 = _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
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
On Mon, Jan 15, 2024 at 01:54:27PM +0100, Paul Cercueil wrote:
> 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 = _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
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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 = _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,
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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 = _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
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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 = _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
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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 = _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).
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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:
> > > > This patch introduces three new ioctls. They all should be
> > > > called
> > > > on a
> > > > data endpoint (ie. not ep0). They are:
> > > >
> > > > - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of
> > > > a
> > > > DMABUF
> > > > object to attach to the endpoint.
> > > >
> > > > - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of
> > > > the
> > > > DMABUF to detach from the endpoint. Note that closing the
> > > > endpoint's
> > > > file descriptor will automatically detach all attached
> > > > DMABUFs.
> > > >
> > > > - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer
> > > > from /
> > > > to
> > > > the given DMABUF. Its argument is a structure that packs the
> > > > DMABUF's
> > > > file descriptor, the size in bytes to transfer (which should
> > > > generally
> > > > be set to the size of the DMABUF), and a 'flags' field which
> > > > is
> > > > unused
> > > > for now.
> > > > Before this ioctl can be used, the related DMABUF must be
> > > > attached
> > > > with FUNCTIONFS_DMABUF_ATTACH.
> > > >
> > > > These three ioctls enable the FunctionFS code to transfer data
> > > > between
> > > > the USB stack and a DMABUF object, which can be provided by a
> > > > driver
> > > > from a completely different subsystem, in a zero-copy fashion.
> > > >
> > > > Signed-off-by: Paul Cercueil
> > > >
> > > > ---
> > > > v2:
> > > > - Make ffs_dma_resv_lock() static
> > > > - Add MODULE_IMPORT_NS(DMA_BUF);
> > > > - The attach/detach functions are now performed without locking
> > > > the
> > > > eps_lock spinlock. The transfer function starts with the
> > > > spinlock
> > > > unlocked, then locks it before allocating and queueing the
> > > > USB
> > > > transfer.
> > > >
> > > > v3:
> > > > - Inline to_ffs_dma_fence() which was called only once.
> > > > - Simplify ffs_dma_resv_lock()
> > > > - Add comment explaining why we unref twice in
> > > > ffs_dmabuf_detach()
> > > > - Document uapi struct usb_ffs_dmabuf_transfer_req and IOCTLs
> > > > ---
> > > > drivers/usb/gadget/function/f_fs.c | 417
> > > >
> > > > include/uapi/linux/usb/functionfs.h | 41 +++
> > > > 2 files changed, 458 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/gadget/function/f_fs.c
> > > > b/drivers/usb/gadget/function/f_fs.c
> > > > index ed2a6d5fcef7..9df1f5abb0d4 100644
> > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > @@ -15,6 +15,9 @@
> > > > /* #define VERBOSE_DEBUG */
> > > >
> > > > #include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -43,6 +46,8 @@
> > > >
> > > > #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a
> > > > honest
> > > > dice roll ;) */
> > > >
> > > > +MODULE_IMPORT_NS(DMA_BUF);
> > > > +
> > > > /* Reference counter handling */
> > > > static void ffs_data_get(struct ffs_data *ffs);
> > > > static void ffs_data_put(struct ffs_data *ffs);
> > > > @@ -124,6 +129,21 @@ struct ffs_ep {
> > > > u8 num;
> > > > };
> > > >
> > > > +struct ffs_dmabuf_priv {
> > > > + struct list_head entry;
> > > > + struct kref ref;
> > > > + struct dma_buf_attachment *attach;
> > > > + spinlock_t lock;
> > > > + u64 context;
> > > > +};
> > > > +
> > > > +struct ffs_dma_fence {
> > > > + struct dma_fence base;
> > > > + struct ffs_dmabuf_priv *priv;
> > > > + struct sg_table *sgt;
> > > > + enum dma_data_direction dir;
> > > > +};
> > > > +
> > > > struct ffs_epfile {
> > > > /* Protects ep->ep and ep->req. */
> > > > struct mutexmutex;
> > > > @@ -197,6 +217,8 @@ struct ffs_epfile {
> > > > unsigned char isoc; /* P: ffs-
> > > > > eps_lock */
> > > >
> > > > unsigned char _pad;
> > > > +
> > > > + struct list_headdmabufs;
> > > > };
> > > >
> > > > struct ffs_buffer {
> > > > @@ -1271,10 +1293,44 @@ static ssize_t
> > > > ffs_epfile_read_iter(struct
> > > > kiocb *kiocb, struct iov_iter *to)
> > > > return res;
> > > > }
> > > >
> > > > +static void ffs_dmabuf_release(struct kref *ref)
> > > > +{
> > > > + struct ffs_dmabuf_priv *priv = container_of(ref,
> > > > struct
> > > > ffs_dmabuf_priv, ref);
> > > > + struct dma_buf_attachment *attach = priv->attach;
> > > > + struct dma_buf *dmabuf = attach->dmabuf;
> > > > +
> > > > + pr_debug("FFS DMABUF release\n");
> > > > + dma_buf_detach(attach->dmabuf, attach);
> > > > +
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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:
> > > This patch introduces three new ioctls. They all should be called
> > > on a
> > > data endpoint (ie. not ep0). They are:
> > >
> > > - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a
> > > DMABUF
> > > object to attach to the endpoint.
> > >
> > > - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
> > > DMABUF to detach from the endpoint. Note that closing the
> > > endpoint's
> > > file descriptor will automatically detach all attached DMABUFs.
> > >
> > > - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from /
> > > to
> > > the given DMABUF. Its argument is a structure that packs the
> > > DMABUF's
> > > file descriptor, the size in bytes to transfer (which should
> > > generally
> > > be set to the size of the DMABUF), and a 'flags' field which is
> > > unused
> > > for now.
> > > Before this ioctl can be used, the related DMABUF must be
> > > attached
> > > with FUNCTIONFS_DMABUF_ATTACH.
> > >
> > > These three ioctls enable the FunctionFS code to transfer data
> > > between
> > > the USB stack and a DMABUF object, which can be provided by a
> > > driver
> > > from a completely different subsystem, in a zero-copy fashion.
> > >
> > > Signed-off-by: Paul Cercueil
> > >
> > > ---
> > > v2:
> > > - Make ffs_dma_resv_lock() static
> > > - Add MODULE_IMPORT_NS(DMA_BUF);
> > > - The attach/detach functions are now performed without locking the
> > > eps_lock spinlock. The transfer function starts with the spinlock
> > > unlocked, then locks it before allocating and queueing the USB
> > > transfer.
> > >
> > > v3:
> > > - Inline to_ffs_dma_fence() which was called only once.
> > > - Simplify ffs_dma_resv_lock()
> > > - Add comment explaining why we unref twice in ffs_dmabuf_detach()
> > > - Document uapi struct usb_ffs_dmabuf_transfer_req and IOCTLs
> > > ---
> > > drivers/usb/gadget/function/f_fs.c | 417
> > >
> > > include/uapi/linux/usb/functionfs.h | 41 +++
> > > 2 files changed, 458 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_fs.c
> > > b/drivers/usb/gadget/function/f_fs.c
> > > index ed2a6d5fcef7..9df1f5abb0d4 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -15,6 +15,9 @@
> > > /* #define VERBOSE_DEBUG */
> > >
> > > #include
> > > +#include
> > > +#include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -43,6 +46,8 @@
> > >
> > > #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest
> > > dice roll ;) */
> > >
> > > +MODULE_IMPORT_NS(DMA_BUF);
> > > +
> > > /* Reference counter handling */
> > > static void ffs_data_get(struct ffs_data *ffs);
> > > static void ffs_data_put(struct ffs_data *ffs);
> > > @@ -124,6 +129,21 @@ struct ffs_ep {
> > > u8 num;
> > > };
> > >
> > > +struct ffs_dmabuf_priv {
> > > + struct list_head entry;
> > > + struct kref ref;
> > > + struct dma_buf_attachment *attach;
> > > + spinlock_t lock;
> > > + u64 context;
> > > +};
> > > +
> > > +struct ffs_dma_fence {
> > > + struct dma_fence base;
> > > + struct ffs_dmabuf_priv *priv;
> > > + struct sg_table *sgt;
> > > + enum dma_data_direction dir;
> > > +};
> > > +
> > > struct ffs_epfile {
> > > /* Protects ep->ep and ep->req. */
> > > struct mutexmutex;
> > > @@ -197,6 +217,8 @@ struct ffs_epfile {
> > > unsigned char isoc; /* P: ffs-
> > > >eps_lock */
> > >
> > > unsigned char _pad;
> > > +
> > > + struct list_headdmabufs;
> > > };
> > >
> > > struct ffs_buffer {
> > > @@ -1271,10 +1293,44 @@ static ssize_t ffs_epfile_read_iter(struct
> > > kiocb *kiocb, struct iov_iter *to)
> > > return res;
> > > }
> > >
> > > +static void ffs_dmabuf_release(struct kref *ref)
> > > +{
> > > + struct ffs_dmabuf_priv *priv = container_of(ref, struct
> > > ffs_dmabuf_priv, ref);
> > > + struct dma_buf_attachment *attach = priv->attach;
> > > + struct dma_buf *dmabuf = attach->dmabuf;
> > > +
> > > + pr_debug("FFS DMABUF release\n");
> > > + dma_buf_detach(attach->dmabuf, attach);
> > > + dma_buf_put(dmabuf);
> > > +
> > > + list_del(>entry);
> >
> > I didn't find any locking for this list here.
>
> Yeah. I'll add some.
>
> > > + kfree(priv);
> > > +}
> > > +
> > > +static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
> > > +{
> > > + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > > +
> > > + kref_get(>ref);
> > > +}
> > > +
> > > +static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
> > > +{
> > > + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > > +
> > > + kref_put(>ref, ffs_dmabuf_release);
> > > +}
> > > +
>
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
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:
> > This patch introduces three new ioctls. They all should be called
> > on a
> > data endpoint (ie. not ep0). They are:
> >
> > - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a
> > DMABUF
> > object to attach to the endpoint.
> >
> > - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
> > DMABUF to detach from the endpoint. Note that closing the
> > endpoint's
> > file descriptor will automatically detach all attached DMABUFs.
> >
> > - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from /
> > to
> > the given DMABUF. Its argument is a structure that packs the
> > DMABUF's
> > file descriptor, the size in bytes to transfer (which should
> > generally
> > be set to the size of the DMABUF), and a 'flags' field which is
> > unused
> > for now.
> > Before this ioctl can be used, the related DMABUF must be
> > attached
> > with FUNCTIONFS_DMABUF_ATTACH.
> >
> > These three ioctls enable the FunctionFS code to transfer data
> > between
> > the USB stack and a DMABUF object, which can be provided by a
> > driver
> > from a completely different subsystem, in a zero-copy fashion.
> >
> > Signed-off-by: Paul Cercueil
> >
> > ---
> > v2:
> > - Make ffs_dma_resv_lock() static
> > - Add MODULE_IMPORT_NS(DMA_BUF);
> > - The attach/detach functions are now performed without locking the
> > eps_lock spinlock. The transfer function starts with the spinlock
> > unlocked, then locks it before allocating and queueing the USB
> > transfer.
> >
> > v3:
> > - Inline to_ffs_dma_fence() which was called only once.
> > - Simplify ffs_dma_resv_lock()
> > - Add comment explaining why we unref twice in ffs_dmabuf_detach()
> > - Document uapi struct usb_ffs_dmabuf_transfer_req and IOCTLs
> > ---
> > drivers/usb/gadget/function/f_fs.c | 417
> >
> > include/uapi/linux/usb/functionfs.h | 41 +++
> > 2 files changed, 458 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/function/f_fs.c
> > b/drivers/usb/gadget/function/f_fs.c
> > index ed2a6d5fcef7..9df1f5abb0d4 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -15,6 +15,9 @@
> > /* #define VERBOSE_DEBUG */
> >
> > #include
> > +#include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -43,6 +46,8 @@
> >
> > #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest
> > dice roll ;) */
> >
> > +MODULE_IMPORT_NS(DMA_BUF);
> > +
> > /* Reference counter handling */
> > static void ffs_data_get(struct ffs_data *ffs);
> > static void ffs_data_put(struct ffs_data *ffs);
> > @@ -124,6 +129,21 @@ struct ffs_ep {
> > u8 num;
> > };
> >
> > +struct ffs_dmabuf_priv {
> > + struct list_head entry;
> > + struct kref ref;
> > + struct dma_buf_attachment *attach;
> > + spinlock_t lock;
> > + u64 context;
> > +};
> > +
> > +struct ffs_dma_fence {
> > + struct dma_fence base;
> > + struct ffs_dmabuf_priv *priv;
> > + struct sg_table *sgt;
> > + enum dma_data_direction dir;
> > +};
> > +
> > struct ffs_epfile {
> > /* Protects ep->ep and ep->req. */
> > struct mutexmutex;
> > @@ -197,6 +217,8 @@ struct ffs_epfile {
> > unsigned char isoc; /* P: ffs-
> > >eps_lock */
> >
> > unsigned char _pad;
> > +
> > + struct list_headdmabufs;
> > };
> >
> > struct ffs_buffer {
> > @@ -1271,10 +1293,44 @@ static ssize_t ffs_epfile_read_iter(struct
> > kiocb *kiocb, struct iov_iter *to)
> > return res;
> > }
> >
> > +static void ffs_dmabuf_release(struct kref *ref)
> > +{
> > + struct ffs_dmabuf_priv *priv = container_of(ref, struct
> > ffs_dmabuf_priv, ref);
> > + struct dma_buf_attachment *attach = priv->attach;
> > + struct dma_buf *dmabuf = attach->dmabuf;
> > +
> > + pr_debug("FFS DMABUF release\n");
> > + dma_buf_detach(attach->dmabuf, attach);
> > + dma_buf_put(dmabuf);
> > +
> > + list_del(>entry);
>
> I didn't find any locking for this list here.
Yeah. I'll add some.
> > + kfree(priv);
> > +}
> > +
> > +static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
> > +{
> > + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > +
> > + kref_get(>ref);
> > +}
> > +
> > +static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
> > +{
> > + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> > +
> > + kref_put(>ref, ffs_dmabuf_release);
> > +}
> > +
> > static int
> > ffs_epfile_release(struct inode *inode, struct file *file)
> > {
> > struct ffs_epfile *epfile = inode->i_private;
> > + struct ffs_dmabuf_priv *priv, *tmp;
> > +
> > + /* Close all attached DMABUFs */
> > + list_for_each_entry_safe(priv, tmp, >dmabufs,
> > entry) {
> > +
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
On Mon, Jan 08, 2024 at 01:00:55PM +0100, Paul Cercueil wrote:
> This patch introduces three new ioctls. They all should be called on a
> data endpoint (ie. not ep0). They are:
>
> - FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a DMABUF
> object to attach to the endpoint.
>
> - FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
> DMABUF to detach from the endpoint. Note that closing the endpoint's
> file descriptor will automatically detach all attached DMABUFs.
>
> - FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from / to
> the given DMABUF. Its argument is a structure that packs the DMABUF's
> file descriptor, the size in bytes to transfer (which should generally
> be set to the size of the DMABUF), and a 'flags' field which is unused
> for now.
> Before this ioctl can be used, the related DMABUF must be attached
> with FUNCTIONFS_DMABUF_ATTACH.
>
> These three ioctls enable the FunctionFS code to transfer data between
> the USB stack and a DMABUF object, which can be provided by a driver
> from a completely different subsystem, in a zero-copy fashion.
>
> Signed-off-by: Paul Cercueil
>
> ---
> v2:
> - Make ffs_dma_resv_lock() static
> - Add MODULE_IMPORT_NS(DMA_BUF);
> - The attach/detach functions are now performed without locking the
> eps_lock spinlock. The transfer function starts with the spinlock
> unlocked, then locks it before allocating and queueing the USB
> transfer.
>
> v3:
> - Inline to_ffs_dma_fence() which was called only once.
> - Simplify ffs_dma_resv_lock()
> - Add comment explaining why we unref twice in ffs_dmabuf_detach()
> - Document uapi struct usb_ffs_dmabuf_transfer_req and IOCTLs
> ---
> drivers/usb/gadget/function/f_fs.c | 417
> include/uapi/linux/usb/functionfs.h | 41 +++
> 2 files changed, 458 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index ed2a6d5fcef7..9df1f5abb0d4 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -15,6 +15,9 @@
> /* #define VERBOSE_DEBUG */
>
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -43,6 +46,8 @@
>
> #define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
>
> +MODULE_IMPORT_NS(DMA_BUF);
> +
> /* Reference counter handling */
> static void ffs_data_get(struct ffs_data *ffs);
> static void ffs_data_put(struct ffs_data *ffs);
> @@ -124,6 +129,21 @@ struct ffs_ep {
> u8 num;
> };
>
> +struct ffs_dmabuf_priv {
> + struct list_head entry;
> + struct kref ref;
> + struct dma_buf_attachment *attach;
> + spinlock_t lock;
> + u64 context;
> +};
> +
> +struct ffs_dma_fence {
> + struct dma_fence base;
> + struct ffs_dmabuf_priv *priv;
> + struct sg_table *sgt;
> + enum dma_data_direction dir;
> +};
> +
> struct ffs_epfile {
> /* Protects ep->ep and ep->req. */
> struct mutexmutex;
> @@ -197,6 +217,8 @@ struct ffs_epfile {
> unsigned char isoc; /* P: ffs->eps_lock */
>
> unsigned char _pad;
> +
> + struct list_headdmabufs;
> };
>
> struct ffs_buffer {
> @@ -1271,10 +1293,44 @@ static ssize_t ffs_epfile_read_iter(struct kiocb
> *kiocb, struct iov_iter *to)
> return res;
> }
>
> +static void ffs_dmabuf_release(struct kref *ref)
> +{
> + struct ffs_dmabuf_priv *priv = container_of(ref, struct
> ffs_dmabuf_priv, ref);
> + struct dma_buf_attachment *attach = priv->attach;
> + struct dma_buf *dmabuf = attach->dmabuf;
> +
> + pr_debug("FFS DMABUF release\n");
> + dma_buf_detach(attach->dmabuf, attach);
> + dma_buf_put(dmabuf);
> +
> + list_del(>entry);
I didn't find any locking for this list here.
> + kfree(priv);
> +}
> +
> +static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
> +{
> + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> +
> + kref_get(>ref);
> +}
> +
> +static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
> +{
> + struct ffs_dmabuf_priv *priv = attach->importer_priv;
> +
> + kref_put(>ref, ffs_dmabuf_release);
> +}
> +
> static int
> ffs_epfile_release(struct inode *inode, struct file *file)
> {
> struct ffs_epfile *epfile = inode->i_private;
> + struct ffs_dmabuf_priv *priv, *tmp;
> +
> + /* Close all attached DMABUFs */
> + list_for_each_entry_safe(priv, tmp, >dmabufs, entry) {
> + ffs_dmabuf_put(priv->attach);
> + }
>
> __ffs_epfile_read_buffer_free(epfile);
> ffs_data_closed(epfile->ffs);
> @@ -1282,6 +1338,328 @@ ffs_epfile_release(struct inode *inode, struct file
> *file)
> return 0;
> }
>
> +static void ffs_dmabuf_signal_done(struct ffs_dma_fence *dma_fence, int ret)
> +{
> + struct ffs_dmabuf_priv *priv =
Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface
Am 08.01.24 um 13:00 schrieb Paul Cercueil:
This patch introduces three new ioctls. They all should be called on a
data endpoint (ie. not ep0). They are:
- FUNCTIONFS_DMABUF_ATTACH, which takes the file descriptor of a DMABUF
object to attach to the endpoint.
- FUNCTIONFS_DMABUF_DETACH, which takes the file descriptor of the
DMABUF to detach from the endpoint. Note that closing the endpoint's
file descriptor will automatically detach all attached DMABUFs.
- FUNCTIONFS_DMABUF_TRANSFER, which requests a data transfer from / to
the given DMABUF. Its argument is a structure that packs the DMABUF's
file descriptor, the size in bytes to transfer (which should generally
be set to the size of the DMABUF), and a 'flags' field which is unused
for now.
Before this ioctl can be used, the related DMABUF must be attached
with FUNCTIONFS_DMABUF_ATTACH.
These three ioctls enable the FunctionFS code to transfer data between
the USB stack and a DMABUF object, which can be provided by a driver
from a completely different subsystem, in a zero-copy fashion.
Signed-off-by: Paul Cercueil
---
v2:
- Make ffs_dma_resv_lock() static
- Add MODULE_IMPORT_NS(DMA_BUF);
- The attach/detach functions are now performed without locking the
eps_lock spinlock. The transfer function starts with the spinlock
unlocked, then locks it before allocating and queueing the USB
transfer.
v3:
- Inline to_ffs_dma_fence() which was called only once.
- Simplify ffs_dma_resv_lock()
- Add comment explaining why we unref twice in ffs_dmabuf_detach()
- Document uapi struct usb_ffs_dmabuf_transfer_req and IOCTLs
---
drivers/usb/gadget/function/f_fs.c | 417
include/uapi/linux/usb/functionfs.h | 41 +++
2 files changed, 458 insertions(+)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index ed2a6d5fcef7..9df1f5abb0d4 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -15,6 +15,9 @@
/* #define VERBOSE_DEBUG */
#include
+#include
+#include
+#include
#include
#include
#include
@@ -43,6 +46,8 @@
#define FUNCTIONFS_MAGIC 0xa647361 /* Chosen by a honest dice roll ;) */
+MODULE_IMPORT_NS(DMA_BUF);
+
/* Reference counter handling */
static void ffs_data_get(struct ffs_data *ffs);
static void ffs_data_put(struct ffs_data *ffs);
@@ -124,6 +129,21 @@ struct ffs_ep {
u8 num;
};
+struct ffs_dmabuf_priv {
+ struct list_head entry;
+ struct kref ref;
+ struct dma_buf_attachment *attach;
+ spinlock_t lock;
+ u64 context;
+};
+
+struct ffs_dma_fence {
+ struct dma_fence base;
+ struct ffs_dmabuf_priv *priv;
+ struct sg_table *sgt;
+ enum dma_data_direction dir;
+};
+
struct ffs_epfile {
/* Protects ep->ep and ep->req. */
struct mutexmutex;
@@ -197,6 +217,8 @@ struct ffs_epfile {
unsigned char isoc; /* P: ffs->eps_lock */
unsigned char _pad;
+
+ struct list_headdmabufs;
};
struct ffs_buffer {
@@ -1271,10 +1293,44 @@ static ssize_t ffs_epfile_read_iter(struct kiocb
*kiocb, struct iov_iter *to)
return res;
}
+static void ffs_dmabuf_release(struct kref *ref)
+{
+ struct ffs_dmabuf_priv *priv = container_of(ref, struct
ffs_dmabuf_priv, ref);
+ struct dma_buf_attachment *attach = priv->attach;
+ struct dma_buf *dmabuf = attach->dmabuf;
+
+ pr_debug("FFS DMABUF release\n");
+ dma_buf_detach(attach->dmabuf, attach);
+ dma_buf_put(dmabuf);
+
+ list_del(>entry);
+ kfree(priv);
+}
+
+static void ffs_dmabuf_get(struct dma_buf_attachment *attach)
+{
+ struct ffs_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_get(>ref);
+}
+
+static void ffs_dmabuf_put(struct dma_buf_attachment *attach)
+{
+ struct ffs_dmabuf_priv *priv = attach->importer_priv;
+
+ kref_put(>ref, ffs_dmabuf_release);
+}
+
static int
ffs_epfile_release(struct inode *inode, struct file *file)
{
struct ffs_epfile *epfile = inode->i_private;
+ struct ffs_dmabuf_priv *priv, *tmp;
+
+ /* Close all attached DMABUFs */
+ list_for_each_entry_safe(priv, tmp, >dmabufs, entry) {
+ ffs_dmabuf_put(priv->attach);
+ }
__ffs_epfile_read_buffer_free(epfile);
ffs_data_closed(epfile->ffs);
@@ -1282,6 +1338,328 @@ ffs_epfile_release(struct inode *inode, struct file
*file)
return 0;
}
+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 = _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);
+
