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@xxxxxxx>
---
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.
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.
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.
+ 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
