[PATCH 02/12] dma-buf: add explicit buffer pinning v2
Daniel Vetter
daniel at ffwll.ch
Tue Apr 30 13:59:37 UTC 2019
On Tue, Apr 30, 2019 at 03:42:22PM +0200, Christian König wrote:
> Am 29.04.19 um 10:40 schrieb Daniel Vetter:
> > On Fri, Apr 26, 2019 at 02:36:28PM +0200, Christian König wrote:
> > > Add optional explicit pinning callbacks instead of implicitly assume the
> > > exporter pins the buffer when a mapping is created.
> > >
> > > v2: move in patchset and pin the dma-buf in the old mapping code paths.
> > >
> > > Signed-off-by: Christian König <christian.koenig at amd.com>
> > > ---
> > > drivers/dma-buf/dma-buf.c | 49 ++++++++++++++++++++++++++++++++++++++-
> > > include/linux/dma-buf.h | 38 +++++++++++++++++++++++++-----
> > > 2 files changed, 80 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index 50b4c6af04c7..0656dcf289be 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -529,6 +529,41 @@ void dma_buf_put(struct dma_buf *dmabuf)
> > > }
> > > EXPORT_SYMBOL_GPL(dma_buf_put);
> > > +/**
> > > + * dma_buf_pin - Lock down the DMA-buf
> > > + *
> > > + * @dmabuf: [in] DMA-buf to lock down.
> > > + *
> > > + * Returns:
> > > + * 0 on success, negative error code on failure.
> > > + */
> > > +int dma_buf_pin(struct dma_buf *dmabuf)
> > I think this should be on the attachment, not on the buffer. Or is the
> > idea that a pin is for the entire buffer, and all subsequent
> > dma_buf_map_attachment must work for all attachments? I think this matters
> > for sufficiently contrived p2p scenarios.
>
> This is indeed for the DMA-buf, cause we are pinning the underlying backing
> store and not just one attachment.
You can't move the buffer either way, so from that point there's no
difference. I more meant from an account/api point of view of whether it's
ok to pin a buffer you haven't even attached to yet. From the code it
seems like first you always want to attach, hence it would make sense to
put the pin/unpin onto the attachment. That might also help with
debugging, we could check whether everyone balances their pin/unpin
correctly (instead of just counting for the overall dma-buf).
There's also a slight semantic difference between a pin on an attachment
(which would mean this attachment is always going to be mappable and wont
move around), whereas a pin on the entire dma-buf means the entire dma-buf
and all it's attachment must always be mappable. Otoh, global pin is
probably easier, you just need to check all current attachments again
whether they still work or whether you might need to move the buffer
around to a more suitable place (e.g. if you not all can do p2p it needs
to go into system ram before it's pinned).
For the backing storage you only want one per-bo ->pinned_count, that's
clear, my suggestion was more about whether having more information about
who's pinning is useful. Exporters can always just ignore that added
information.
> Key point is I want to call this in the exporter itself in the long run.
> E.g. that the exporter stops working with its internal special handling
> functions and uses this one instead.
Hm, not exactly following why the exporter needs to call
dma_buf_pin/unpin, instead of whatever is used to implement that. Or do
you mean that you want a ->pinned_count in dma_buf itself, so that there's
only one book-keeping for this?
> > Either way, docs need to clarify this.
>
> Going to add a bit more here.
>
> >
> > > +{
> > > + int ret = 0;
> > > +
> > > + reservation_object_assert_held(dmabuf->resv);
> > > +
> > > + if (dmabuf->ops->pin)
> > > + ret = dmabuf->ops->pin(dmabuf);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dma_buf_pin);
> > > +
> > > +/**
> > > + * dma_buf_unpin - Remove lock from DMA-buf
> > > + *
> > > + * @dmabuf: [in] DMA-buf to unlock.
> > > + */
> > > +void dma_buf_unpin(struct dma_buf *dmabuf)
> > > +{
> > > + reservation_object_assert_held(dmabuf->resv);
> > > +
> > > + if (dmabuf->ops->unpin)
> > > + dmabuf->ops->unpin(dmabuf);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dma_buf_unpin);
> > > +
> > > /**
> > > * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
> > > * calls attach() of dma_buf_ops to allow device-specific attach functionality
> > > @@ -548,7 +583,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
> > > * accessible to @dev, and cannot be moved to a more suitable place. This is
> > > * indicated with the error code -EBUSY.
> > > */
> > > -struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info)
> > > +struct dma_buf_attachment *
> > > +dma_buf_attach(const struct dma_buf_attach_info *info)
> > > {
> > > struct dma_buf *dmabuf = info->dmabuf;
> > > struct dma_buf_attachment *attach;
> > > @@ -625,12 +661,19 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> > > enum dma_data_direction direction)
> > > {
> > > struct sg_table *sg_table;
> > > + int r;
> > > might_sleep();
> > > if (WARN_ON(!attach || !attach->dmabuf))
> > > return ERR_PTR(-EINVAL);
> > > + reservation_object_lock(attach->dmabuf->resv, NULL);
> > > + r = dma_buf_pin(attach->dmabuf);
> > > + reservation_object_unlock(attach->dmabuf->resv);
> > This adds an unconditional reservat lock to map/unmap, which is think
> > pisses off drivers. This gets fixed later on with the caching, but means
> > the series is broken here.
> >
> > Also, that super-fine grained split-up makes it harder for me to review
> > the docs, since only until the very end are all the bits present for full
> > dynamic dma-buf mappings.
> >
> > I think it'd be best to squash all the patches from pin up to the one that
> > adds the invalidate callback into one patch. It's all changes to
> > dma-buf.[hc] only anyway. If that is too big we can think about how to
> > split it up again, but at least for me the current splitting doesn't make
> > sense at all.
>
> Fine with me, if you think that is easier to review. It just gave me a big
> headache to add all that logic at the same time.
I think the big headache is unavoidable, since it's all linked very
closely together. Hence why I think this is easier to review, defacto it's
dma-buf v2. Treating it as a clean-slate thing (but with backwards compat)
instead of as an evolution feels better (after the first few attempts of a
split up series).
Cheers, Daniel
>
> >
> > > + if (r)
> > > + return ERR_PTR(r);
> > > +
> > > sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> > > if (!sg_table)
> > > sg_table = ERR_PTR(-ENOMEM);
> > Leaks the pin if we fail here.
>
> Going to fix that.
>
> >
> > > @@ -660,6 +703,10 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> > > attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
> > > direction);
> > > +
> > > + reservation_object_lock(attach->dmabuf->resv, NULL);
> > > + dma_buf_unpin(attach->dmabuf);
> > > + reservation_object_unlock(attach->dmabuf->resv);
> > > }
> > > EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
> > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > > index 2c312dfd31a1..0321939b1c3d 100644
> > > --- a/include/linux/dma-buf.h
> > > +++ b/include/linux/dma-buf.h
> > > @@ -51,6 +51,34 @@ struct dma_buf_attachment;
> > > * @vunmap: [optional] unmaps a vmap from the buffer
> > > */
> > > struct dma_buf_ops {
> > > + /**
> > > + * @pin:
> > > + *
> > > + * This is called by dma_buf_pin and lets the exporter know that the
> > > + * DMA-buf can't be moved any more.
> > > + *
> > > + * This is called with the dmabuf->resv object locked.
> > > + *
> > > + * This callback is optional.
> > > + *
> > > + * Returns:
> > > + *
> > > + * 0 on success, negative error code on failure.
> > > + */
> > > + int (*pin)(struct dma_buf *);
> > > +
> > > + /**
> > > + * @unpin:
> > > + *
> > > + * This is called by dma_buf_unpin and lets the exporter know that the
> > > + * DMA-buf can be moved again.
> > > + *
> > > + * This is called with the dmabuf->resv object locked.
> > > + *
> > > + * This callback is optional.
> > "This callback is optional, but must be provided if @pin is." Same for
> > @pin I think, plus would be good to check in dma_buf_export that you have
> > both or neither with
> >
> > WARN_ON(!!exp_info->ops->pin == !!exp_info->ops->unpin);
> >
> > > + */
> > > + void (*unpin)(struct dma_buf *);
> > > +
> > > /**
> > > * @attach:
> > > *
> > > @@ -95,9 +123,7 @@ struct dma_buf_ops {
> > > *
> > > * This is called by dma_buf_map_attachment() and is used to map a
> > > * shared &dma_buf into device address space, and it is mandatory. It
> > > - * can only be called if @attach has been called successfully. This
> > > - * essentially pins the DMA buffer into place, and it cannot be moved
> > > - * any more
> > > + * can only be called if @attach has been called successfully.
> > I think dropping this outright isn't correct, since for all current
> > dma-buf exporters it's still what they should be doing. We just need to
> > make this conditional on @pin and @unpin not being present:
> >
> > "If @pin and @unpin are not provided this essentially pins the DMA
> > buffer into place, and it cannot be moved any more."
> >
> > > *
> > > * This call may sleep, e.g. when the backing storage first needs to be
> > > * allocated, or moved to a location suitable for all currently attached
> > > @@ -135,9 +161,6 @@ struct dma_buf_ops {
> > > *
> > > * This is called by dma_buf_unmap_attachment() and should unmap and
> > > * release the &sg_table allocated in @map_dma_buf, and it is mandatory.
> > Same here, add a "If @pin and @unpin are not provided this should ..."
> > qualifier instead of deleting.
> >
> > Cheers, Daniel
> >
> >
> > > - * It should also unpin the backing storage if this is the last mapping
> > > - * of the DMA buffer, it the exporter supports backing storage
> > > - * migration.
> > > */
> > > void (*unmap_dma_buf)(struct dma_buf_attachment *,
> > > struct sg_table *,
> > > @@ -386,6 +409,9 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
> > > get_file(dmabuf->file);
> > > }
> > > +int dma_buf_pin(struct dma_buf *dmabuf);
> > > +void dma_buf_unpin(struct dma_buf *dmabuf);
> > > +
> > > struct dma_buf_attachment *
> > > dma_buf_attach(const struct dma_buf_attach_info *info);
> > > void dma_buf_detach(struct dma_buf *dmabuf,
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list