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@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.
Either way, docs need to clarify this.
+{
- 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.
- 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.
@@ -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
*/ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *,* migration.
@@ -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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel