[PATCH 01/12] dma-buf: add struct dma_buf_attach_info

[PATCH 01/12] dma-buf: add struct dma_buf_attach_info

[Christian König]

Add a structure for the parameters of dma_buf_attach, this makes it much easier
to add new parameters later on.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c                       | 13 +++++++------
 drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
 drivers/gpu/drm/drm_prime.c                     |  6 +++++-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-
 drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
 drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
 .../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
 .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-
 drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-
 include/linux/dma-buf.h                         | 17 +++++++++++++++--
 10 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 7c858020d14b..50b4c6af04c7 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
 /**
  * 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
- * @dmabuf:	[in]	buffer to attach device to.
- * @dev:	[in]	device to be attached.
+ * @info:	[in]	holds all the attach related information provided
+ *			by the importer. see &struct dma_buf_attach_info
+ *			for further details.
  *
  * Returns struct dma_buf_attachment pointer for this attachment. Attachments
  * must be cleaned up by calling dma_buf_detach().
@@ -547,20 +548,20 @@ 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(struct dma_buf *dmabuf,
-					  struct device *dev)
+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;
 	int ret;
 
-	if (WARN_ON(!dmabuf || !dev))
+	if (WARN_ON(!dmabuf || !info->dev))
 		return ERR_PTR(-EINVAL);
 
 	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
 	if (!attach)
 		return ERR_PTR(-ENOMEM);
 
-	attach->dev = dev;
+	attach->dev = info->dev;
 	attach->dmabuf = dmabuf;
 
 	mutex_lock(&dmabuf->lock);
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 642d0e70d0f8..19c47821032f 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
 struct drm_gem_object *
 armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = buf
+	};
 	struct dma_buf_attachment *attach;
 	struct armada_gem_object *dobj;
 
@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 		}
 	}
 
-	attach = dma_buf_attach(buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 231e3f6d5f41..1fadf5d5ed33 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 					    struct dma_buf *dma_buf,
 					    struct device *attach_dev)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = attach_dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct sg_table *sgt;
 	struct drm_gem_object *obj;
@@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 	if (!dev->driver->gem_prime_import_sg_table)
 		return ERR_PTR(-EINVAL);
 
-	attach = dma_buf_attach(dma_buf, attach_dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 82e2ca17a441..aa7f685bd6ca 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
 struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 					     struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	}
 
 	/* need to attach */
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 4f80100ff5f3..8e6b6c879add 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,
 static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 					struct dma_buf *buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = drm->dev,
+		.dmabuf = buf
+	};
 	struct tegra_drm *tegra = drm->dev_private;
 	struct dma_buf_attachment *attach;
 	struct tegra_bo *bo;
@@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 	if (IS_ERR(bo))
 		return bo;
 
-	attach = dma_buf_attach(buf, drm->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach)) {
 		err = PTR_ERR(attach);
 		goto free;
diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
index 556f62662aa9..86b928f9742f 100644
--- a/drivers/gpu/drm/udl/udl_dmabuf.c
+++ b/drivers/gpu/drm/udl/udl_dmabuf.c
@@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev,
 struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 				struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct sg_table *sg;
 	struct udl_gem_object *uobj;
@@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 
 	/* need to attach */
 	get_device(dev->dev);
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach)) {
 		put_device(dev->dev);
 		return ERR_CAST(attach);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index aff0ab7bf83d..1f2687b5eb0e 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
 static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	unsigned long size, enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dbuf
+	};
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
 
@@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 
 	buf->dev = dev;
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach(&attach_info);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 015e737095cd..cbd626d2393a 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
 static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	unsigned long size, enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dbuf
+	};
 	struct vb2_dma_sg_buf *buf;
 	struct dma_buf_attachment *dba;
 
@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 
 	buf->dev = dev;
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach(&attach_info);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
index aa6c6bba961e..5a10c1facc27 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 				   size_t *size,
 				   enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dmabuf
+	};
 	struct dma_buf_attachment *attachment;
 	struct dma_buf *dmabuf;
 	struct sg_table *sgt;
@@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 		return -EINVAL;
 	}
 
-	attachment = dma_buf_attach(dmabuf, dev);
+	attachment = dma_buf_attach(&attach_info);
 	if (IS_ERR(attachment)) {
 		dev_err(dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(attachment);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 58725f890b5b..2c312dfd31a1 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -359,6 +359,19 @@ struct dma_buf_export_info {
 	struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \
 					 .owner = THIS_MODULE }
 
+/**
+ * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
+ * @dmabuf:	the exported dma_buf
+ * @dev:	the device which wants to import the attachment
+ *
+ * This structure holds the information required to attach to a buffer. Used
+ * with dma_buf_attach() only.
+ */
+struct dma_buf_attach_info {
+	struct dma_buf *dmabuf;
+	struct device *dev;
+};
+
 /**
  * get_dma_buf - convenience wrapper for get_file.
  * @dmabuf:	[in]	pointer to dma_buf
@@ -373,8 +386,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
 	get_file(dmabuf->file);
 }
 
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
-							struct device *dev);
+struct dma_buf_attachment *
+dma_buf_attach(const struct dma_buf_attach_info *info);
 void dma_buf_detach(struct dma_buf *dmabuf,
 				struct dma_buf_attachment *dmabuf_attach);
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Christian König]

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)
+{
+	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);
+	if (r)
+		return ERR_PTR(r);
+
 	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
@@ -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.
+	 */
+	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.
 	 *
 	 * 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.
-	 * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/12] dma-buf: start caching of sg_table objects

[Christian König]

To allow a smooth transition from pinning buffer objects to dynamic
invalidation we first start to cache the sg_table for an attachment
unless the driver has implemented the explicite pin/unpin callbacks.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++
 include/linux/dma-buf.h   |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 0656dcf289be..a18d10c4425a 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -610,6 +610,20 @@ dma_buf_attach(const struct dma_buf_attach_info *info)
 	list_add(&attach->node, &dmabuf->attachments);
 
 	mutex_unlock(&dmabuf->lock);
+
+	if (!dmabuf->ops->pin) {
+		struct sg_table *sgt;
+
+		sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
+		if (!sgt)
+			sgt = ERR_PTR(-ENOMEM);
+		if (IS_ERR(sgt)) {
+			dma_buf_detach(dmabuf, attach);
+			return ERR_CAST(sgt);
+		}
+		attach->sgt = sgt;
+	}
+
 	return attach;
 
 err_attach:
@@ -632,6 +646,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 	if (WARN_ON(!dmabuf || !attach))
 		return;
 
+	if (attach->sgt)
+		dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
+					   DMA_BIDIRECTIONAL);
+
 	mutex_lock(&dmabuf->lock);
 	list_del(&attach->node);
 	if (dmabuf->ops->detach)
@@ -668,6 +686,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 	if (WARN_ON(!attach || !attach->dmabuf))
 		return ERR_PTR(-EINVAL);
 
+	if (attach->sgt)
+		return attach->sgt;
+
 	reservation_object_lock(attach->dmabuf->resv, NULL);
 	r = dma_buf_pin(attach->dmabuf);
 	reservation_object_unlock(attach->dmabuf->resv);
@@ -701,6 +722,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
 		return;
 
+	if (attach->sgt == sg_table)
+		return;
+
 	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
 						direction);
 
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 0321939b1c3d..b9d0719581cd 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -345,6 +345,7 @@ struct dma_buf_attachment {
 	struct dma_buf *dmabuf;
 	struct device *dev;
 	struct list_head node;
+	struct sg_table *sgt;
 	void *priv;
 };
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/12] dma-buf: lock the reservation object during (un)map_dma_buf v4

[Christian König]

This way we always call the the map/unmap callbacks with the reservation
object held if pin/unpin is also implemented.

For static dma-buf exporters we still have the fallback of using cached sgt.

v2: reordered
v3: rebased on sgt caching
v4: use the cached sgt when possible
v5: cleanup further

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 10 +++++-----
 include/linux/dma-buf.h   |  3 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index a18d10c4425a..0aa97bb05636 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -691,13 +691,15 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 
 	reservation_object_lock(attach->dmabuf->resv, NULL);
 	r = dma_buf_pin(attach->dmabuf);
-	reservation_object_unlock(attach->dmabuf->resv);
-	if (r)
+	if (r) {
+		reservation_object_unlock(attach->dmabuf->resv);
 		return ERR_PTR(r);
+	}
 
 	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
+	reservation_object_unlock(attach->dmabuf->resv);
 
 	return sg_table;
 }
@@ -725,10 +727,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 	if (attach->sgt == sg_table)
 		return;
 
-	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
-						direction);
-
 	reservation_object_lock(attach->dmabuf->resv, NULL);
+	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
 	dma_buf_unpin(attach->dmabuf);
 	reservation_object_unlock(attach->dmabuf->resv);
 }
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index b9d0719581cd..425a771d229c 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -144,6 +144,9 @@ struct dma_buf_ops {
 	 * any other kind of sharing that the exporter might wish to make
 	 * available to buffer-users.
 	 *
+	 * This is always called with the dmabuf->resv object locked when
+	 * the pin/unpin callbacks are implemented.
+	 *
 	 * Returns:
 	 *
 	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/12] dma-buf: add dma_buf_(un)map_attachment_locked variants v4

[Christian König]

Add function variants which can be called with the reservation lock
already held.

v2: reordered, add lockdep asserts, fix kerneldoc
v3: rebased on sgt caching
v4: reorder once more

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 83 +++++++++++++++++++++++++++++++++------
 include/linux/dma-buf.h   |  5 +++
 2 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 0aa97bb05636..95bcd2ed795b 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -660,6 +660,48 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 }
 EXPORT_SYMBOL_GPL(dma_buf_detach);
 
+/**
+ * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
+ * with the reservation lock held. Is a wrapper for map_dma_buf() of the
+ *
+ * Returns the scatterlist table of the attachment;
+ * dma_buf_ops.
+ * @attach:	[in]	attachment whose scatterlist is to be returned
+ * @direction:	[in]	direction of DMA transfer
+ *
+ * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
+ * on error. May return -EINTR if it is interrupted by a signal.
+ *
+ * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
+ * the underlying backing storage is pinned for as long as a mapping exists,
+ * therefore users/importers should not hold onto a mapping for undue amounts of
+ * time.
+ */
+struct sg_table *
+dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
+			      enum dma_data_direction direction)
+{
+	struct sg_table *sg_table;
+	int r;
+
+	might_sleep();
+	reservation_object_assert_held(attach->dmabuf->resv);
+
+	if (attach->sgt)
+		return attach->sgt;
+
+	r = dma_buf_pin(attach->dmabuf);
+	if (r)
+		return ERR_PTR(r);
+
+	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	if (!sg_table)
+		return ERR_PTR(-ENOMEM);
+
+	return sg_table;
+}
+EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
+
 /**
  * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
  * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
@@ -679,7 +721,6 @@ 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();
 
@@ -690,21 +731,40 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 		return attach->sgt;
 
 	reservation_object_lock(attach->dmabuf->resv, NULL);
-	r = dma_buf_pin(attach->dmabuf);
-	if (r) {
-		reservation_object_unlock(attach->dmabuf->resv);
-		return ERR_PTR(r);
-	}
-
-	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
-	if (!sg_table)
-		sg_table = ERR_PTR(-ENOMEM);
+	sg_table = dma_buf_map_attachment(attach, direction);
 	reservation_object_unlock(attach->dmabuf->resv);
 
 	return sg_table;
 }
 EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
 
+/**
+ * dma_buf_unmap_attachment_locked - unmaps the buffer with reservation lock
+ * held, should deallocate the associated scatterlist. Is a wrapper for
+ * unmap_dma_buf() of dma_buf_ops.
+ * @attach:	[in]	attachment to unmap buffer from
+ * @sg_table:	[in]	scatterlist info of the buffer to unmap
+ * @direction:  [in]    direction of DMA transfer
+ *
+ * This unmaps a DMA mapping for @attached obtained by
+ * dma_buf_map_attachment_locked().
+ */
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
+				     struct sg_table *sg_table,
+				     enum dma_data_direction direction)
+{
+	might_sleep();
+	reservation_object_assert_held(attach->dmabuf->resv);
+
+	if (attach->sgt == sg_table)
+		return;
+
+	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
+						direction);
+	dma_buf_unpin(attach->dmabuf);
+}
+EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
+
 /**
  * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
  * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
@@ -728,8 +788,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 		return;
 
 	reservation_object_lock(attach->dmabuf->resv, NULL);
-	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);
-	dma_buf_unpin(attach->dmabuf);
+	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
 	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 425a771d229c..f7d6fc049b39 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -427,8 +427,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
 struct dma_buf *dma_buf_get(int fd);
 void dma_buf_put(struct dma_buf *dmabuf);
 
+struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
+					       enum dma_data_direction);
 struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
 					enum dma_data_direction);
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
+				     struct sg_table *,
+				     enum dma_data_direction);
 void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
 				enum dma_data_direction);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/12] dma-buf: add optional invalidate_mappings callback v5

[Christian König]

Each importer can now provide an invalidate_mappings callback.

This allows the exporter to provide the mappings without the need to pin
the backing store.

v2: don't try to invalidate mappings when the callback is NULL,
    lock the reservation obj while using the attachments,
    add helper to set the callback
v3: move flag for invalidation support into the DMA-buf,
    use new attach_info structure to set the callback
v4: use importer_priv field instead of mangling exporter priv.
v5: drop invalidation_supported flag

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c | 51 +++++++++++++++++++++++++++++++++------
 include/linux/dma-buf.h   | 30 +++++++++++++++++++++--
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 95bcd2ed795b..1b5cdae68cd6 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -607,7 +607,9 @@ dma_buf_attach(const struct dma_buf_attach_info *info)
 		if (ret)
 			goto err_attach;
 	}
+	reservation_object_lock(dmabuf->resv, NULL);
 	list_add(&attach->node, &dmabuf->attachments);
+	reservation_object_unlock(dmabuf->resv);
 
 	mutex_unlock(&dmabuf->lock);
 
@@ -651,7 +653,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 					   DMA_BIDIRECTIONAL);
 
 	mutex_lock(&dmabuf->lock);
+	reservation_object_lock(dmabuf->resv, NULL);
 	list_del(&attach->node);
+	reservation_object_unlock(dmabuf->resv);
 	if (dmabuf->ops->detach)
 		dmabuf->ops->detach(dmabuf, attach);
 
@@ -672,10 +676,7 @@ EXPORT_SYMBOL_GPL(dma_buf_detach);
  * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
  * on error. May return -EINTR if it is interrupted by a signal.
  *
- * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
- * the underlying backing storage is pinned for as long as a mapping exists,
- * therefore users/importers should not hold onto a mapping for undue amounts of
- * time.
+ * A mapping must be unmapped by using dma_buf_unmap_attachment().
  */
 struct sg_table *
 dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
@@ -690,11 +691,22 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
 	if (attach->sgt)
 		return attach->sgt;
 
-	r = dma_buf_pin(attach->dmabuf);
-	if (r)
-		return ERR_PTR(r);
+	if (attach->invalidate) {
+		/*
+		 * Mapping a DMA-buf can trigger its invalidation, prevent
+		 * sending this event to the caller by temporary removing
+		 * this attachment from the list.
+		 */
+		list_del(&attach->node);
+	} else {
+		r = dma_buf_pin(attach->dmabuf);
+		if (r)
+			return ERR_PTR(r);
+	}
 
 	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	if (attach->invalidate)
+		list_add(&attach->node, &attach->dmabuf->attachments);
 	if (!sg_table)
 		return ERR_PTR(-ENOMEM);
 
@@ -761,7 +773,8 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
 
 	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
 						direction);
-	dma_buf_unpin(attach->dmabuf);
+	if (!attach->invalidate)
+		dma_buf_unpin(attach->dmabuf);
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
 
@@ -793,6 +806,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 }
 EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
 
+/**
+ * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
+ *
+ * @dmabuf:	[in]	buffer which mappings should be invalidated
+ *
+ * Informs all attachmenst that they need to destroy and recreated all their
+ * mappings.
+ */
+void dma_buf_invalidate_mappings(struct dma_buf *dmabuf)
+{
+	struct dma_buf_attachment *attach;
+
+	reservation_object_assert_held(dmabuf->resv);
+
+	list_for_each_entry(attach, &dmabuf->attachments, node)
+		if (attach->invalidate)
+			attach->invalidate(attach);
+}
+EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);
+
 /**
  * DOC: cpu access
  *
@@ -1205,10 +1238,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
 		seq_puts(s, "\tAttached Devices:\n");
 		attach_count = 0;
 
+		reservation_object_lock(buf_obj->resv, NULL);
 		list_for_each_entry(attach_obj, &buf_obj->attachments, node) {
 			seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
 			attach_count++;
 		}
+		reservation_object_unlock(buf_obj->resv);
 
 		seq_printf(s, "Total %d devices attached\n\n",
 				attach_count);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index f7d6fc049b39..d27e91bbd796 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -334,6 +334,9 @@ struct dma_buf {
  * @dev: device attached to the buffer.
  * @node: list of dma_buf_attachment.
  * @priv: exporter specific attachment data.
+ * @importer_priv: importer specific attachment data.
+ * @invalidate: invalidate callback, see &dma_buf_attach_info.invalidate for
+ *		more information.
  *
  * This structure holds the attachment information between the dma_buf buffer
  * and its user device(s). The list contains one attachment struct per device
@@ -350,6 +353,8 @@ struct dma_buf_attachment {
 	struct list_head node;
 	struct sg_table *sgt;
 	void *priv;
+	void *importer_priv;
+	void (*invalidate)(struct dma_buf_attachment *attach);
 };
 
 /**
@@ -388,15 +393,35 @@ struct dma_buf_export_info {
 
 /**
  * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
- * @dmabuf:	the exported dma_buf
- * @dev:	the device which wants to import the attachment
+ * @dmabuf:		the exported dma_buf
+ * @dev:		the device which wants to import the attachment
+ * @importer_priv:	private data of importer to this attachment
+ * @invalidate:		optional callback provided by the importer
  *
  * This structure holds the information required to attach to a buffer. Used
  * with dma_buf_attach() only.
+ *
+ * Only if the invalidate callback is provided the framework can avoid pinning
+ * the backing store while mappings exists.
+ *
+ * This callback is called with the lock of the reservation object
+ * associated with the dma_buf held and the mapping function must be
+ * called with this lock held as well. This makes sure that no mapping
+ * is created concurrently with an ongoing invalidation.
+ *
+ * After the callback all existing mappings are still valid until all
+ * fences in the dma_bufs reservation object are signaled. After getting an
+ * invalidation callback all mappings should be destroyed by the importer using
+ * the normal dma_buf_unmap_attachment() function as soon as possible.
+ *
+ * New mappings can be created immediately, but can't be used before the
+ * exclusive fence in the dma_bufs reservation object is signaled.
  */
 struct dma_buf_attach_info {
 	struct dma_buf *dmabuf;
 	struct device *dev;
+	void *importer_priv;
+	void (*invalidate)(struct dma_buf_attachment *attach);
 };
 
 /**
@@ -436,6 +461,7 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
 				     enum dma_data_direction);
 void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
 				enum dma_data_direction);
+void dma_buf_invalidate_mappings(struct dma_buf *dma_buf);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
 			     enum dma_data_direction dir);
 int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/12] drm: remove prime sg_table caching

[Christian König]

That is now done by the DMA-buf helpers instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_prime.c | 76 ++++++++-----------------------------
 1 file changed, 16 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1fadf5d5ed33..7e439ea3546a 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -86,11 +86,6 @@ struct drm_prime_member {
 	struct rb_node handle_rb;
 };
 
-struct drm_prime_attachment {
-	struct sg_table *sgt;
-	enum dma_data_direction dir;
-};
-
 static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
 				    struct dma_buf *dma_buf, uint32_t handle)
 {
@@ -188,25 +183,16 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri
  * @dma_buf: buffer to attach device to
  * @attach: buffer attachment data
  *
- * Allocates &drm_prime_attachment and calls &drm_driver.gem_prime_pin for
- * device specific attachment. This can be used as the &dma_buf_ops.attach
- * callback.
+ * Calls &drm_driver.gem_prime_pin for device specific handling. This can be
+ * used as the &dma_buf_ops.attach callback.
  *
  * Returns 0 on success, negative error code on failure.
  */
 int drm_gem_map_attach(struct dma_buf *dma_buf,
 		       struct dma_buf_attachment *attach)
 {
-	struct drm_prime_attachment *prime_attach;
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
-	if (!prime_attach)
-		return -ENOMEM;
-
-	prime_attach->dir = DMA_NONE;
-	attach->priv = prime_attach;
-
 	return drm_gem_pin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_attach);
@@ -222,26 +208,8 @@ EXPORT_SYMBOL(drm_gem_map_attach);
 void drm_gem_map_detach(struct dma_buf *dma_buf,
 			struct dma_buf_attachment *attach)
 {
-	struct drm_prime_attachment *prime_attach = attach->priv;
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	if (prime_attach) {
-		struct sg_table *sgt = prime_attach->sgt;
-
-		if (sgt) {
-			if (prime_attach->dir != DMA_NONE)
-				dma_unmap_sg_attrs(attach->dev, sgt->sgl,
-						   sgt->nents,
-						   prime_attach->dir,
-						   DMA_ATTR_SKIP_CPU_SYNC);
-			sg_free_table(sgt);
-		}
-
-		kfree(sgt);
-		kfree(prime_attach);
-		attach->priv = NULL;
-	}
-
 	drm_gem_unpin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_detach);
@@ -286,39 +254,22 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
 struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
 				     enum dma_data_direction dir)
 {
-	struct drm_prime_attachment *prime_attach = attach->priv;
 	struct drm_gem_object *obj = attach->dmabuf->priv;
 	struct sg_table *sgt;
 
-	if (WARN_ON(dir == DMA_NONE || !prime_attach))
+	if (WARN_ON(dir == DMA_NONE))
 		return ERR_PTR(-EINVAL);
 
-	/* return the cached mapping when possible */
-	if (prime_attach->dir == dir)
-		return prime_attach->sgt;
-
-	/*
-	 * two mappings with different directions for the same attachment are
-	 * not allowed
-	 */
-	if (WARN_ON(prime_attach->dir != DMA_NONE))
-		return ERR_PTR(-EBUSY);
-
 	if (obj->funcs)
 		sgt = obj->funcs->get_sg_table(obj);
 	else
 		sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
 
-	if (!IS_ERR(sgt)) {
-		if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
-				      DMA_ATTR_SKIP_CPU_SYNC)) {
-			sg_free_table(sgt);
-			kfree(sgt);
-			sgt = ERR_PTR(-ENOMEM);
-		} else {
-			prime_attach->sgt = sgt;
-			prime_attach->dir = dir;
-		}
+	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
+			      DMA_ATTR_SKIP_CPU_SYNC)) {
+		sg_free_table(sgt);
+		kfree(sgt);
+		sgt = ERR_PTR(-ENOMEM);
 	}
 
 	return sgt;
@@ -331,14 +282,19 @@ EXPORT_SYMBOL(drm_gem_map_dma_buf);
  * @sgt: scatterlist info of the buffer to unmap
  * @dir: direction of DMA transfer
  *
- * Not implemented. The unmap is done at drm_gem_map_detach().  This can be
- * used as the &dma_buf_ops.unmap_dma_buf callback.
+ * This can be used as the &dma_buf_ops.unmap_dma_buf callback.
  */
 void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
 			   struct sg_table *sgt,
 			   enum dma_data_direction dir)
 {
-	/* nothing to be done here */
+	if (!sgt)
+		return;
+
+	dma_unmap_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
+			   DMA_ATTR_SKIP_CPU_SYNC);
+	sg_free_table(sgt);
+	kfree(sgt);
 }
 EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/12] drm/ttm: remove the backing store if no placement is given

[Christian König]

Pipeline removal of the BOs backing store when no placement is given
during validation.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 2845fceb2fbd..8502b3ed2d88 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1160,6 +1160,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 	uint32_t new_flags;
 
 	reservation_object_assert_held(bo->resv);
+
+	/*
+	 * Remove the backing store if no placement is given.
+	 */
+	if (!placement->num_placement && !placement->num_busy_placement) {
+		ret = ttm_bo_pipeline_gutting(bo);
+		if (ret)
+			return ret;
+
+		return ttm_tt_create(bo, false);
+	}
+
 	/*
 	 * Check whether we need to move buffer.
 	 */
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/12] drm/ttm: use the parent resv for ghost objects

[Christian König]

This way we can even pipeline imported BO evictions.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 895d77d799e4..97f35c4bda35 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -486,7 +486,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
 				      struct ttm_buffer_object **new_obj)
 {
 	struct ttm_transfer_obj *fbo;
-	int ret;
 
 	fbo = kmalloc(sizeof(*fbo), GFP_KERNEL);
 	if (!fbo)
@@ -517,10 +516,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
 	kref_init(&fbo->base.kref);
 	fbo->base.destroy = &ttm_transfered_destroy;
 	fbo->base.acc_size = 0;
-	fbo->base.resv = &fbo->base.ttm_resv;
-	reservation_object_init(fbo->base.resv);
-	ret = reservation_object_trylock(fbo->base.resv);
-	WARN_ON(!ret);
+	reservation_object_init(&fbo->base.ttm_resv);
 
 	*new_obj = &fbo->base;
 	return 0;
@@ -716,8 +712,6 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
 		if (ret)
 			return ret;
 
-		reservation_object_add_excl_fence(ghost_obj->resv, fence);
-
 		/**
 		 * If we're not moving to fixed memory, the TTM object
 		 * needs to stay alive. Otherwhise hang it on the ghost
@@ -729,7 +723,6 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
 		else
 			bo->ttm = NULL;
 
-		ttm_bo_unreserve(ghost_obj);
 		ttm_bo_put(ghost_obj);
 	}
 
@@ -772,8 +765,6 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
 		if (ret)
 			return ret;
 
-		reservation_object_add_excl_fence(ghost_obj->resv, fence);
-
 		/**
 		 * If we're not moving to fixed memory, the TTM object
 		 * needs to stay alive. Otherwhise hang it on the ghost
@@ -785,7 +776,6 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
 		else
 			bo->ttm = NULL;
 
-		ttm_bo_unreserve(ghost_obj);
 		ttm_bo_put(ghost_obj);
 
 	} else if (from->flags & TTM_MEMTYPE_FLAG_FIXED) {
@@ -841,16 +831,10 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
 	if (ret)
 		return ret;
 
-	ret = reservation_object_copy_fences(ghost->resv, bo->resv);
-	/* Last resort, wait for the BO to be idle when we are OOM */
-	if (ret)
-		ttm_bo_wait(bo, false, false);
-
 	memset(&bo->mem, 0, sizeof(bo->mem));
 	bo->mem.mem_type = TTM_PL_SYSTEM;
 	bo->ttm = NULL;
 
-	ttm_bo_unreserve(ghost);
 	ttm_bo_put(ghost);
 
 	return 0;
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Christian König]

The caching of SGT's is actually quite harmful and should probably removed
altogether when all drivers are audited.

Start by providing a separate DMA-buf export implementation in amdgpu. This is
also a prerequisite of unpinned DMA-buf handling.

v2: fix unintended recursion, remove debugging leftovers
v3: split out from unpinned DMA-buf work
v4: rebase on top of new no_sgt_cache flag

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |   1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h    |   1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |   5 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c  | 162 +++++++++++++--------
 4 files changed, 109 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 13a68f62bcc8..f1815223a1a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1254,7 +1254,6 @@ static struct drm_driver kms_driver = {
 	.gem_prime_export = amdgpu_gem_prime_export,
 	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
-	.gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
 	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
 	.gem_prime_vmap = amdgpu_gem_prime_vmap,
 	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
index f1ddfc50bcc7..0c50d14a9739 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
@@ -39,7 +39,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
 void amdgpu_gem_object_close(struct drm_gem_object *obj,
 				struct drm_file *file_priv);
 unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
 struct drm_gem_object *
 amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 				 struct dma_buf_attachment *attach,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 93b2c5a48a71..d26e2f0b88d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -31,6 +31,7 @@
  */
 #include <linux/list.h>
 #include <linux/slab.h>
+#include <linux/dma-buf.h>
 #include <drm/drmP.h>
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_cache.h>
@@ -1194,6 +1195,10 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 
 	amdgpu_bo_kunmap(abo);
 
+	if (abo->gem_base.dma_buf && !abo->gem_base.import_attach &&
+	    bo->mem.mem_type != TTM_PL_SYSTEM)
+		dma_buf_invalidate_mappings(abo->gem_base.dma_buf);
+
 	/* remember the eviction */
 	if (evict)
 		atomic64_inc(&adev->num_evictions);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index a38e0fb4a6fe..cdc3c1e63a68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -40,22 +40,6 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-fence-array.h>
 
-/**
- * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
- * implementation
- * @obj: GEM buffer object (BO)
- *
- * Returns:
- * A scatter/gather table for the pinned pages of the BO's memory.
- */
-struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
-{
-	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
-	int npages = bo->tbo.num_pages;
-
-	return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
-}
-
 /**
  * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
  * @obj: GEM BO
@@ -231,34 +215,84 @@ __reservation_object_make_exclusive(struct reservation_object *obj)
 }
 
 /**
- * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
- * @dma_buf: Shared DMA buffer
+ * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation
+ *
+ * @dma_buf: DMA-buf to pin in memory
+ *
+ * Pin the BO which is backing the DMA-buf so that it can't move any more.
+ */
+static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf)
+{
+	struct drm_gem_object *obj = dma_buf->priv;
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+	/* pin buffer into GTT */
+	return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
+}
+
+/**
+ * amdgpu_gem_unpin_dma_buf - &dma_buf_ops.unpin_dma_buf implementation
+ *
+ * @dma_buf: DMA-buf to unpin
+ *
+ * Unpin a previously pinned BO to make it movable again.
+ */
+static void amdgpu_gem_unpin_dma_buf(struct dma_buf *dma_buf)
+{
+	struct drm_gem_object *obj = dma_buf->priv;
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+	amdgpu_bo_unpin(bo);
+}
+
+/**
+ * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation
+ *
+ * @dma_buf: DMA-buf we attach to
  * @attach: DMA-buf attachment
  *
+ * Returns:
+ * Always zero for success.
+ */
+static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf,
+				     struct dma_buf_attachment *attach)
+{
+	struct drm_gem_object *obj = dma_buf->priv;
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+
+	/* Make sure the buffer is pinned when userspace didn't set GTT as
+	 * preferred domain. This avoid ping/pong situations with scan out BOs.
+	 */
+	if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT))
+		attach->invalidate = NULL;
+
+	return 0;
+}
+
+/**
+ * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
+ * @attach: DMA-buf attachment
+ * @dir: DMA direction
+ *
  * Makes sure that the shared DMA buffer can be accessed by the target device.
  * For now, simply pins it to the GTT domain, where it should be accessible by
  * all DMA devices.
  *
  * Returns:
- * 0 on success or a negative error code on failure.
+ * sg_table filled with the DMA addresses to use or ERR_PRT with negative error
+ * code.
  */
-static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
-				 struct dma_buf_attachment *attach)
+static struct sg_table *
+amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
+		       enum dma_data_direction dir)
 {
+	struct dma_buf *dma_buf = attach->dmabuf;
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+	struct sg_table *sgt;
 	long r;
 
-	r = drm_gem_map_attach(dma_buf, attach);
-	if (r)
-		return r;
-
-	r = amdgpu_bo_reserve(bo, false);
-	if (unlikely(r != 0))
-		goto error_detach;
-
-
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
 		 * We only create shared fences for internal use, but importers
@@ -270,53 +304,64 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 		 */
 		r = __reservation_object_make_exclusive(bo->tbo.resv);
 		if (r)
-			goto error_unreserve;
+			return ERR_PTR(r);
 	}
 
-	/* pin buffer into GTT */
-	r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
-	if (r)
-		goto error_unreserve;
+	if (attach->invalidate) {
+		/* move buffer into GTT */
+		struct ttm_operation_ctx ctx = { false, false };
+
+		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
+		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+		if (r)
+			return ERR_PTR(r);
+	}
+
+	sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
+	if (IS_ERR(sgt))
+		return sgt;
+
+	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
+			      DMA_ATTR_SKIP_CPU_SYNC))
+		goto error_free;
 
 	if (attach->dev->driver != adev->dev->driver)
 		bo->prime_shared_count++;
 
-error_unreserve:
-	amdgpu_bo_unreserve(bo);
+	return sgt;
 
-error_detach:
-	if (r)
-		drm_gem_map_detach(dma_buf, attach);
-	return r;
+error_free:
+	sg_free_table(sgt);
+	kfree(sgt);
+	return ERR_PTR(-ENOMEM);
 }
 
 /**
- * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
- * @dma_buf: Shared DMA buffer
+ * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
  * @attach: DMA-buf attachment
+ * @sgt: sg_table to unmap
+ * @dir: DMA direction
  *
  * This is called when a shared DMA buffer no longer needs to be accessible by
  * another device. For now, simply unpins the buffer from GTT.
  */
-static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
-				  struct dma_buf_attachment *attach)
+static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+				     struct sg_table *sgt,
+				     enum dma_data_direction dir)
 {
+	struct dma_buf *dma_buf = attach->dmabuf;
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
-	int ret = 0;
-
-	ret = amdgpu_bo_reserve(bo, true);
-	if (unlikely(ret != 0))
-		goto error;
 
-	amdgpu_bo_unpin(bo);
 	if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count)
 		bo->prime_shared_count--;
-	amdgpu_bo_unreserve(bo);
 
-error:
-	drm_gem_map_detach(dma_buf, attach);
+	if (sgt) {
+		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+		sg_free_table(sgt);
+		kfree(sgt);
+	}
 }
 
 /**
@@ -374,10 +419,11 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf,
 }
 
 const struct dma_buf_ops amdgpu_dmabuf_ops = {
-	.attach = amdgpu_gem_map_attach,
-	.detach = amdgpu_gem_map_detach,
-	.map_dma_buf = drm_gem_map_dma_buf,
-	.unmap_dma_buf = drm_gem_unmap_dma_buf,
+	.pin = amdgpu_gem_pin_dma_buf,
+	.unpin = amdgpu_gem_unpin_dma_buf,
+	.attach = amdgpu_gem_dma_buf_attach,
+	.map_dma_buf = amdgpu_gem_map_dma_buf,
+	.unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
 	.release = drm_gem_dmabuf_release,
 	.begin_cpu_access = amdgpu_gem_begin_cpu_access,
 	.mmap = drm_gem_dmabuf_mmap,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/12] drm/amdgpu: add independent DMA-buf import v4

[Christian König]

Instead of relying on the DRM functions just implement our own import
functions. This prepares support for taking care of unpinned DMA-buf.

v2: enable for all exporters, not just amdgpu, fix invalidation
    handling, lock reservation object while setting callback
v3: change to new dma_buf attach interface
v4: split out from unpinned DMA-buf work

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h   |  4 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 42 +++++++++++++++--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   | 34 +++++++++++++++---
 4 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index f1815223a1a1..95195c427e85 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1254,7 +1254,6 @@ static struct drm_driver kms_driver = {
 	.gem_prime_export = amdgpu_gem_prime_export,
 	.gem_prime_import = amdgpu_gem_prime_import,
 	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
-	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
 	.gem_prime_vmap = amdgpu_gem_prime_vmap,
 	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
 	.gem_prime_mmap = amdgpu_gem_prime_mmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
index 0c50d14a9739..01811d8aa8a9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
@@ -39,10 +39,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
 void amdgpu_gem_object_close(struct drm_gem_object *obj,
 				struct drm_file *file_priv);
 unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
-struct drm_gem_object *
-amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
-				 struct dma_buf_attachment *attach,
-				 struct sg_table *sg);
 struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 					struct drm_gem_object *gobj,
 					int flags);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index cdc3c1e63a68..662cd99980cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -122,31 +122,28 @@ int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma
 }
 
 /**
- * amdgpu_gem_prime_import_sg_table - &drm_driver.gem_prime_import_sg_table
- * implementation
+ * amdgpu_gem_prime_create_obj - create BO for DMA-buf import
+ *
  * @dev: DRM device
- * @attach: DMA-buf attachment
- * @sg: Scatter/gather table
+ * @dma_buf: DMA-buf
  *
- * Imports shared DMA buffer memory exported by another device.
+ * Creates an empty SG BO for DMA-buf import.
  *
  * Returns:
  * A new GEM BO of the given DRM device, representing the memory
  * described by the given DMA-buf attachment and scatter/gather table.
  */
-struct drm_gem_object *
-amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
-				 struct dma_buf_attachment *attach,
-				 struct sg_table *sg)
+static struct drm_gem_object *
+amdgpu_gem_prime_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)
 {
-	struct reservation_object *resv = attach->dmabuf->resv;
+	struct reservation_object *resv = dma_buf->resv;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_bo *bo;
 	struct amdgpu_bo_param bp;
 	int ret;
 
 	memset(&bp, 0, sizeof(bp));
-	bp.size = attach->dmabuf->size;
+	bp.size = dma_buf->size;
 	bp.byte_align = PAGE_SIZE;
 	bp.domain = AMDGPU_GEM_DOMAIN_CPU;
 	bp.flags = 0;
@@ -157,11 +154,9 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	if (ret)
 		goto error;
 
-	bo->tbo.sg = sg;
-	bo->tbo.ttm->sg = sg;
 	bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
 	bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
-	if (attach->dmabuf->ops != &amdgpu_dmabuf_ops)
+	if (dma_buf->ops != &amdgpu_dmabuf_ops)
 		bo->prime_shared_count = 1;
 
 	ww_mutex_unlock(&resv->lock);
@@ -477,6 +472,11 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf,
+	};
+	struct dma_buf_attachment *attach;
 	struct drm_gem_object *obj;
 
 	if (dma_buf->ops == &amdgpu_dmabuf_ops) {
@@ -491,5 +491,17 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 		}
 	}
 
-	return drm_gem_prime_import(dev, dma_buf);
+	obj = amdgpu_gem_prime_create_obj(dev, dma_buf);
+	if (IS_ERR(obj))
+		return obj;
+
+	attach = dma_buf_attach(&attach_info);
+	if (IS_ERR(attach)) {
+		drm_gem_object_put(obj);
+		return ERR_CAST(attach);
+	}
+
+	get_dma_buf(dma_buf);
+	obj->import_attach = attach;
+	return obj;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index c14198737dcd..afccca5b1f5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -44,6 +44,7 @@
 #include <linux/debugfs.h>
 #include <linux/iommu.h>
 #include <linux/hmm.h>
+#include <linux/dma-buf.h>
 #include "amdgpu.h"
 #include "amdgpu_object.h"
 #include "amdgpu_trace.h"
@@ -706,6 +707,7 @@ static unsigned long amdgpu_ttm_io_mem_pfn(struct ttm_buffer_object *bo,
  */
 struct amdgpu_ttm_tt {
 	struct ttm_dma_tt	ttm;
+	struct drm_gem_object	*gobj;
 	u64			offset;
 	uint64_t		userptr;
 	struct task_struct	*usertask;
@@ -1179,6 +1181,7 @@ static struct ttm_tt *amdgpu_ttm_tt_create(struct ttm_buffer_object *bo,
 		return NULL;
 	}
 	gtt->ttm.ttm.func = &amdgpu_backend_func;
+	gtt->gobj = &ttm_to_amdgpu_bo(bo)->gem_base;
 
 	/* allocate space for the uninitialized page entries */
 	if (ttm_sg_tt_init(&gtt->ttm, bo, page_flags)) {
@@ -1199,7 +1202,6 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(ttm->bdev);
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
-	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
 	/* user pages are bound by amdgpu_ttm_tt_pin_userptr() */
 	if (gtt && gtt->userptr) {
@@ -1212,7 +1214,20 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
 		return 0;
 	}
 
-	if (slave && ttm->sg) {
+	if (ttm->page_flags & TTM_PAGE_FLAG_SG) {
+		if (!ttm->sg) {
+			struct dma_buf_attachment *attach;
+			struct sg_table *sgt;
+
+			attach = gtt->gobj->import_attach;
+			sgt = dma_buf_map_attachment_locked(attach,
+							    DMA_BIDIRECTIONAL);
+			if (IS_ERR(sgt))
+				return PTR_ERR(sgt);
+
+			ttm->sg = sgt;
+		}
+
 		drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
 						 gtt->ttm.dma_address,
 						 ttm->num_pages);
@@ -1239,9 +1254,8 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
  */
 static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 {
-	struct amdgpu_device *adev;
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
-	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
+	struct amdgpu_device *adev;
 
 	if (gtt && gtt->userptr) {
 		amdgpu_ttm_tt_set_user_pages(ttm, NULL);
@@ -1250,7 +1264,17 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 		return;
 	}
 
-	if (slave)
+	if (ttm->sg && gtt->gobj->import_attach) {
+		struct dma_buf_attachment *attach;
+
+		attach = gtt->gobj->import_attach;
+		dma_buf_unmap_attachment_locked(attach, ttm->sg,
+						DMA_BIDIRECTIONAL);
+		ttm->sg = NULL;
+		return;
+	}
+
+	if (ttm->page_flags & TTM_PAGE_FLAG_SG)
 		return;
 
 	adev = amdgpu_ttm_adev(ttm->bdev);
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/12] drm/amdgpu: add DMA-buf invalidation callback v2

[Christian König]

Allow for invalidation of imported DMA-bufs.

v2: add dma_buf_pin/dma_buf_unpin support

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  6 ++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c  | 24 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d26e2f0b88d2..affde72b44db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -850,6 +850,9 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 		return 0;
 	}
 
+	if (bo->gem_base.import_attach)
+		dma_buf_pin(bo->gem_base.import_attach->dmabuf);
+
 	bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
 	/* force to pin into visible video ram */
 	if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
@@ -933,6 +936,9 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)
 
 	amdgpu_bo_subtract_pin_size(bo);
 
+	if (bo->gem_base.import_attach)
+		dma_buf_unpin(bo->gem_base.import_attach->dmabuf);
+
 	for (i = 0; i < bo->placement.num_placement; i++) {
 		bo->placements[i].lpfn = 0;
 		bo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 662cd99980cb..675375b2fb7b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -458,6 +458,28 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
 	return buf;
 }
 
+/**
+ * amdgpu_gem_prime_invalidate_mappings - &attach.invalidate implementation
+ *
+ * @attach: the DMA-buf attachment
+ *
+ * Invalidate the DMA-buf attachment, making sure that the we re-create the
+ * mapping before the next use.
+ */
+static void
+amdgpu_gem_prime_invalidate_mappings(struct dma_buf_attachment *attach)
+{
+	struct ttm_operation_ctx ctx = { false, false };
+	struct drm_gem_object *obj = attach->importer_priv;
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+	struct ttm_placement placement = {};
+	int r;
+
+	r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
+	if (r)
+		DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
+}
+
 /**
  * amdgpu_gem_prime_import - &drm_driver.gem_prime_import implementation
  * @dev: DRM device
@@ -475,6 +497,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 	struct dma_buf_attach_info attach_info = {
 		.dev = dev->dev,
 		.dmabuf = dma_buf,
+		.invalidate = amdgpu_gem_prime_invalidate_mappings
 	};
 	struct dma_buf_attachment *attach;
 	struct drm_gem_object *obj;
@@ -495,6 +518,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 	if (IS_ERR(obj))
 		return obj;
 
+	attach_info.importer_priv = obj;
 	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach)) {
 		drm_gem_object_put(obj);
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/12] dma-buf: add struct dma_buf_attach_info

[Daniel Vetter]

On Fri, Apr 26, 2019 at 02:36:27PM +0200, Christian König wrote:
> Add a structure for the parameters of dma_buf_attach, this makes it much easier
> to add new parameters later on.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c                       | 13 +++++++------
>  drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
>  drivers/gpu/drm/drm_prime.c                     |  6 +++++-
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-

omapdrm seems to be missing.

>  drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
>  drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
>  .../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-

videobuf2-dma-sg seems to be missing.

>  drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-

fastrpc and gntdev-dmabuf also missing.

I guess all just rebase fallout. With those all compiling:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Would also be good to cc: everyone on the next round per
get_mainteiners.pl for this patch.
-Daniel


>  include/linux/dma-buf.h                         | 17 +++++++++++++++--
>  10 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 7c858020d14b..50b4c6af04c7 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>  /**
>   * 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
> - * @dmabuf:	[in]	buffer to attach device to.
> - * @dev:	[in]	device to be attached.
> + * @info:	[in]	holds all the attach related information provided
> + *			by the importer. see &struct dma_buf_attach_info
> + *			for further details.
>   *
>   * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>   * must be cleaned up by calling dma_buf_detach().
> @@ -547,20 +548,20 @@ 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(struct dma_buf *dmabuf,
> -					  struct device *dev)
> +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;
>  	int ret;
>  
> -	if (WARN_ON(!dmabuf || !dev))
> +	if (WARN_ON(!dmabuf || !info->dev))
>  		return ERR_PTR(-EINVAL);
>  
>  	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
>  	if (!attach)
>  		return ERR_PTR(-ENOMEM);
>  
> -	attach->dev = dev;
> +	attach->dev = info->dev;
>  	attach->dmabuf = dmabuf;
>  
>  	mutex_lock(&dmabuf->lock);
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index 642d0e70d0f8..19c47821032f 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
>  struct drm_gem_object *
>  armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct armada_gem_object *dobj;
>  
> @@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>  		}
>  	}
>  
> -	attach = dma_buf_attach(buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 231e3f6d5f41..1fadf5d5ed33 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  					    struct dma_buf *dma_buf,
>  					    struct device *attach_dev)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = attach_dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sgt;
>  	struct drm_gem_object *obj;
> @@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  	if (!dev->driver->gem_prime_import_sg_table)
>  		return ERR_PTR(-EINVAL);
>  
> -	attach = dma_buf_attach(dma_buf, attach_dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 82e2ca17a441..aa7f685bd6ca 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
>  struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  					     struct dma_buf *dma_buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  	}
>  
>  	/* need to attach */
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 4f80100ff5f3..8e6b6c879add 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,
>  static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>  					struct dma_buf *buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = drm->dev,
> +		.dmabuf = buf
> +	};
>  	struct tegra_drm *tegra = drm->dev_private;
>  	struct dma_buf_attachment *attach;
>  	struct tegra_bo *bo;
> @@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>  	if (IS_ERR(bo))
>  		return bo;
>  
> -	attach = dma_buf_attach(buf, drm->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach)) {
>  		err = PTR_ERR(attach);
>  		goto free;
> diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
> index 556f62662aa9..86b928f9742f 100644
> --- a/drivers/gpu/drm/udl/udl_dmabuf.c
> +++ b/drivers/gpu/drm/udl/udl_dmabuf.c
> @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev,
>  struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>  				struct dma_buf *dma_buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sg;
>  	struct udl_gem_object *uobj;
> @@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>  
>  	/* need to attach */
>  	get_device(dev->dev);
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach)) {
>  		put_device(dev->dev);
>  		return ERR_CAST(attach);
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index aff0ab7bf83d..1f2687b5eb0e 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
>  static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  	unsigned long size, enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dbuf
> +	};
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
>  
> @@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  
>  	buf->dev = dev;
>  	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach(&attach_info);
>  	if (IS_ERR(dba)) {
>  		pr_err("failed to attach dmabuf\n");
>  		kfree(buf);
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 015e737095cd..cbd626d2393a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
>  static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  	unsigned long size, enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dbuf
> +	};
>  	struct vb2_dma_sg_buf *buf;
>  	struct dma_buf_attachment *dba;
>  
> @@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  
>  	buf->dev = dev;
>  	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach(&attach_info);
>  	if (IS_ERR(dba)) {
>  		pr_err("failed to attach dmabuf\n");
>  		kfree(buf);
> diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
> index aa6c6bba961e..5a10c1facc27 100644
> --- a/drivers/staging/media/tegra-vde/tegra-vde.c
> +++ b/drivers/staging/media/tegra-vde/tegra-vde.c
> @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>  				   size_t *size,
>  				   enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dmabuf
> +	};
>  	struct dma_buf_attachment *attachment;
>  	struct dma_buf *dmabuf;
>  	struct sg_table *sgt;
> @@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	attachment = dma_buf_attach(dmabuf, dev);
> +	attachment = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attachment)) {
>  		dev_err(dev, "Failed to attach dmabuf\n");
>  		err = PTR_ERR(attachment);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 58725f890b5b..2c312dfd31a1 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -359,6 +359,19 @@ struct dma_buf_export_info {
>  	struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \
>  					 .owner = THIS_MODULE }
>  
> +/**
> + * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
> + * @dmabuf:	the exported dma_buf
> + * @dev:	the device which wants to import the attachment
> + *
> + * This structure holds the information required to attach to a buffer. Used
> + * with dma_buf_attach() only.
> + */
> +struct dma_buf_attach_info {
> +	struct dma_buf *dmabuf;
> +	struct device *dev;
> +};
> +
>  /**
>   * get_dma_buf - convenience wrapper for get_file.
>   * @dmabuf:	[in]	pointer to dma_buf
> @@ -373,8 +386,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
>  	get_file(dmabuf->file);
>  }
>  
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -							struct device *dev);
> +struct dma_buf_attachment *
> +dma_buf_attach(const struct dma_buf_attach_info *info);
>  void dma_buf_detach(struct dma_buf *dmabuf,
>  				struct dma_buf_attachment *dmabuf_attach);
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[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@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
> -	 * 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/12] dma-buf: add optional invalidate_mappings callback v5

[Daniel Vetter]

On Fri, Apr 26, 2019 at 02:36:32PM +0200, Christian König wrote:
> Each importer can now provide an invalidate_mappings callback.
> 
> This allows the exporter to provide the mappings without the need to pin
> the backing store.
> 
> v2: don't try to invalidate mappings when the callback is NULL,
>     lock the reservation obj while using the attachments,
>     add helper to set the callback
> v3: move flag for invalidation support into the DMA-buf,
>     use new attach_info structure to set the callback
> v4: use importer_priv field instead of mangling exporter priv.
> v5: drop invalidation_supported flag
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c | 51 +++++++++++++++++++++++++++++++++------
>  include/linux/dma-buf.h   | 30 +++++++++++++++++++++--
>  2 files changed, 71 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 95bcd2ed795b..1b5cdae68cd6 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -607,7 +607,9 @@ dma_buf_attach(const struct dma_buf_attach_info *info)
>  		if (ret)
>  			goto err_attach;
>  	}
> +	reservation_object_lock(dmabuf->resv, NULL);
>  	list_add(&attach->node, &dmabuf->attachments);
> +	reservation_object_unlock(dmabuf->resv);
>  
>  	mutex_unlock(&dmabuf->lock);
>  
> @@ -651,7 +653,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  					   DMA_BIDIRECTIONAL);
>  
>  	mutex_lock(&dmabuf->lock);
> +	reservation_object_lock(dmabuf->resv, NULL);
>  	list_del(&attach->node);
> +	reservation_object_unlock(dmabuf->resv);
>  	if (dmabuf->ops->detach)
>  		dmabuf->ops->detach(dmabuf, attach);
>  
> @@ -672,10 +676,7 @@ EXPORT_SYMBOL_GPL(dma_buf_detach);
>   * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
>   * on error. May return -EINTR if it is interrupted by a signal.
>   *
> - * A mapping must be unmapped by using dma_buf_unmap_attachment(). Note that
> - * the underlying backing storage is pinned for as long as a mapping exists,
> - * therefore users/importers should not hold onto a mapping for undue amounts of
> - * time.
> + * A mapping must be unmapped by using dma_buf_unmap_attachment().
>   */
>  struct sg_table *
>  dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
> @@ -690,11 +691,22 @@ dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
>  	if (attach->sgt)
>  		return attach->sgt;
>  
> -	r = dma_buf_pin(attach->dmabuf);
> -	if (r)
> -		return ERR_PTR(r);
> +	if (attach->invalidate) {
> +		/*
> +		 * Mapping a DMA-buf can trigger its invalidation, prevent
> +		 * sending this event to the caller by temporary removing
> +		 * this attachment from the list.
> +		 */
> +		list_del(&attach->node);
> +	} else {
> +		r = dma_buf_pin(attach->dmabuf);
> +		if (r)
> +			return ERR_PTR(r);
> +	}
>  
>  	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	if (attach->invalidate)
> +		list_add(&attach->node, &attach->dmabuf->attachments);
>  	if (!sg_table)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -761,7 +773,8 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
>  
>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>  						direction);
> -	dma_buf_unpin(attach->dmabuf);
> +	if (!attach->invalidate)
> +		dma_buf_unpin(attach->dmabuf);
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked);
>  
> @@ -793,6 +806,26 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  }
>  EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>  
> +/**
> + * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
> + *
> + * @dmabuf:	[in]	buffer which mappings should be invalidated
> + *
> + * Informs all attachmenst that they need to destroy and recreated all their
> + * mappings.
> + */
> +void dma_buf_invalidate_mappings(struct dma_buf *dmabuf)
> +{
> +	struct dma_buf_attachment *attach;
> +
> +	reservation_object_assert_held(dmabuf->resv);
> +
> +	list_for_each_entry(attach, &dmabuf->attachments, node)
> +		if (attach->invalidate)
> +			attach->invalidate(attach);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_invalidate_mappings);
> +
>  /**
>   * DOC: cpu access
>   *
> @@ -1205,10 +1238,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
>  		seq_puts(s, "\tAttached Devices:\n");
>  		attach_count = 0;
>  
> +		reservation_object_lock(buf_obj->resv, NULL);
>  		list_for_each_entry(attach_obj, &buf_obj->attachments, node) {
>  			seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
>  			attach_count++;
>  		}
> +		reservation_object_unlock(buf_obj->resv);
>  
>  		seq_printf(s, "Total %d devices attached\n\n",
>  				attach_count);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index f7d6fc049b39..d27e91bbd796 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -334,6 +334,9 @@ struct dma_buf {
>   * @dev: device attached to the buffer.
>   * @node: list of dma_buf_attachment.
>   * @priv: exporter specific attachment data.
> + * @importer_priv: importer specific attachment data.
> + * @invalidate: invalidate callback, see &dma_buf_attach_info.invalidate for
> + *		more information.
>   *
>   * This structure holds the attachment information between the dma_buf buffer
>   * and its user device(s). The list contains one attachment struct per device
> @@ -350,6 +353,8 @@ struct dma_buf_attachment {
>  	struct list_head node;
>  	struct sg_table *sgt;
>  	void *priv;
> +	void *importer_priv;
> +	void (*invalidate)(struct dma_buf_attachment *attach);
>  };
>  
>  /**
> @@ -388,15 +393,35 @@ struct dma_buf_export_info {
>  
>  /**
>   * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
> - * @dmabuf:	the exported dma_buf
> - * @dev:	the device which wants to import the attachment
> + * @dmabuf:		the exported dma_buf
> + * @dev:		the device which wants to import the attachment
> + * @importer_priv:	private data of importer to this attachment
> + * @invalidate:		optional callback provided by the importer
>   *
>   * This structure holds the information required to attach to a buffer. Used
>   * with dma_buf_attach() only.
> + *
> + * Only if the invalidate callback is provided the framework can avoid pinning
> + * the backing store while mappings exists.
> + *
> + * This callback is called with the lock of the reservation object
> + * associated with the dma_buf held and the mapping function must be
> + * called with this lock held as well. This makes sure that no mapping
> + * is created concurrently with an ongoing invalidation.
> + *
> + * After the callback all existing mappings are still valid until all
> + * fences in the dma_bufs reservation object are signaled. After getting an
> + * invalidation callback all mappings should be destroyed by the importer using
> + * the normal dma_buf_unmap_attachment() function as soon as possible.
> + *
> + * New mappings can be created immediately, but can't be used before the
> + * exclusive fence in the dma_bufs reservation object is signaled.

Imo this is worse than the old place, since now the invalidate specific
stuff is lost in the general help text. Please move to an inline comment
(yes you can mix this in the same struct with non-inline member comments).

I think in the end we also need a DOC: overview section for dynamic dma
buf mmaps, that explains the overall flows. But that's easier to type with
all the different bits in one patch.
-Daniel

>   */
>  struct dma_buf_attach_info {
>  	struct dma_buf *dmabuf;
>  	struct device *dev;
> +	void *importer_priv;
> +	void (*invalidate)(struct dma_buf_attachment *attach);
>  };
>  
>  /**
> @@ -436,6 +461,7 @@ void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,
>  				     enum dma_data_direction);
>  void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
>  				enum dma_data_direction);
> +void dma_buf_invalidate_mappings(struct dma_buf *dma_buf);
>  int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>  			     enum dma_data_direction dir);
>  int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/12] dma-buf: start caching of sg_table objects

[Daniel Vetter]

On Fri, Apr 26, 2019 at 02:36:29PM +0200, Christian König wrote:
> To allow a smooth transition from pinning buffer objects to dynamic
> invalidation we first start to cache the sg_table for an attachment
> unless the driver has implemented the explicite pin/unpin callbacks.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Correction on my "squash everything" suggestion: I think this patch here
still needs to be separate, so that "why did this cause a regression" is
easier to understand. But with a small nit.

> ---
>  drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++
>  include/linux/dma-buf.h   |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 0656dcf289be..a18d10c4425a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -610,6 +610,20 @@ dma_buf_attach(const struct dma_buf_attach_info *info)
>  	list_add(&attach->node, &dmabuf->attachments);
>  
>  	mutex_unlock(&dmabuf->lock);
> +
> +	if (!dmabuf->ops->pin) {

I think a static inline dma_buf_is_dynamic_attachment() would be good
here, which in this patch (since it's ealier) would always return false.

Aside: I think only checking pin here (i.e. can the exporter do dynamic
mappings) isn't good enough, we also need to check for the importers
->invalidate. Hence _is_dynamic_attachment, not _is_dynamic_dma_buf. If we
don't also check for the importer we again have a reservation lock in
dma_buf_map/unmap, which we know will anger the lockdep gods ...
-Daniel

> +		struct sg_table *sgt;
> +
> +		sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> +		if (!sgt)
> +			sgt = ERR_PTR(-ENOMEM);
> +		if (IS_ERR(sgt)) {
> +			dma_buf_detach(dmabuf, attach);
> +			return ERR_CAST(sgt);
> +		}
> +		attach->sgt = sgt;
> +	}
> +
>  	return attach;
>  
>  err_attach:
> @@ -632,6 +646,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  	if (WARN_ON(!dmabuf || !attach))
>  		return;
>  
> +	if (attach->sgt)
> +		dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> +					   DMA_BIDIRECTIONAL);
> +
>  	mutex_lock(&dmabuf->lock);
>  	list_del(&attach->node);
>  	if (dmabuf->ops->detach)
> @@ -668,6 +686,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf))
>  		return ERR_PTR(-EINVAL);
>  
> +	if (attach->sgt)
> +		return attach->sgt;
> +
>  	reservation_object_lock(attach->dmabuf->resv, NULL);
>  	r = dma_buf_pin(attach->dmabuf);
>  	reservation_object_unlock(attach->dmabuf->resv);
> @@ -701,6 +722,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>  		return;
>  
> +	if (attach->sgt == sg_table)
> +		return;
> +
>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>  						direction);
>  
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 0321939b1c3d..b9d0719581cd 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -345,6 +345,7 @@ struct dma_buf_attachment {
>  	struct dma_buf *dmabuf;
>  	struct device *dev;
>  	struct list_head node;
> +	struct sg_table *sgt;
>  	void *priv;
>  };
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/12] dma-buf: add struct dma_buf_attach_info

[Christian König]

Am 29.04.19 um 10:24 schrieb Daniel Vetter:
> On Fri, Apr 26, 2019 at 02:36:27PM +0200, Christian König wrote:
>> Add a structure for the parameters of dma_buf_attach, this makes it much easier
>> to add new parameters later on.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-buf.c                       | 13 +++++++------
>>   drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
>>   drivers/gpu/drm/drm_prime.c                     |  6 +++++-
>>   drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-
> omapdrm seems to be missing.
>
>>   drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
>>   drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
>>   .../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
>>   .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-
> videobuf2-dma-sg seems to be missing.
>
>>   drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-
> fastrpc and gntdev-dmabuf also missing.
>
> I guess all just rebase fallout. With those all compiling:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks.

>
> Would also be good to cc: everyone on the next round per
> get_mainteiners.pl for this patch.

All fixed and send out, let's see if I get any response.

Christian.

> -Daniel
>
>
>>   include/linux/dma-buf.h                         | 17 +++++++++++++++--
>>   10 files changed, 62 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 7c858020d14b..50b4c6af04c7 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -532,8 +532,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>>   /**
>>    * 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
>> - * @dmabuf:	[in]	buffer to attach device to.
>> - * @dev:	[in]	device to be attached.
>> + * @info:	[in]	holds all the attach related information provided
>> + *			by the importer. see &struct dma_buf_attach_info
>> + *			for further details.
>>    *
>>    * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>>    * must be cleaned up by calling dma_buf_detach().
>> @@ -547,20 +548,20 @@ 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(struct dma_buf *dmabuf,
>> -					  struct device *dev)
>> +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;
>>   	int ret;
>>   
>> -	if (WARN_ON(!dmabuf || !dev))
>> +	if (WARN_ON(!dmabuf || !info->dev))
>>   		return ERR_PTR(-EINVAL);
>>   
>>   	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
>>   	if (!attach)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> -	attach->dev = dev;
>> +	attach->dev = info->dev;
>>   	attach->dmabuf = dmabuf;
>>   
>>   	mutex_lock(&dmabuf->lock);
>> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
>> index 642d0e70d0f8..19c47821032f 100644
>> --- a/drivers/gpu/drm/armada/armada_gem.c
>> +++ b/drivers/gpu/drm/armada/armada_gem.c
>> @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
>>   struct drm_gem_object *
>>   armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev->dev,
>> +		.dmabuf = buf
>> +	};
>>   	struct dma_buf_attachment *attach;
>>   	struct armada_gem_object *dobj;
>>   
>> @@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>>   		}
>>   	}
>>   
>> -	attach = dma_buf_attach(buf, dev->dev);
>> +	attach = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attach))
>>   		return ERR_CAST(attach);
>>   
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 231e3f6d5f41..1fadf5d5ed33 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -709,6 +709,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>>   					    struct dma_buf *dma_buf,
>>   					    struct device *attach_dev)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = attach_dev,
>> +		.dmabuf = dma_buf
>> +	};
>>   	struct dma_buf_attachment *attach;
>>   	struct sg_table *sgt;
>>   	struct drm_gem_object *obj;
>> @@ -729,7 +733,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>>   	if (!dev->driver->gem_prime_import_sg_table)
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	attach = dma_buf_attach(dma_buf, attach_dev);
>> +	attach = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attach))
>>   		return ERR_CAST(attach);
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> index 82e2ca17a441..aa7f685bd6ca 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
>>   struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>>   					     struct dma_buf *dma_buf)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev->dev,
>> +		.dmabuf = dma_buf
>> +	};
>>   	struct dma_buf_attachment *attach;
>>   	struct drm_i915_gem_object *obj;
>>   	int ret;
>> @@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>>   	}
>>   
>>   	/* need to attach */
>> -	attach = dma_buf_attach(dma_buf, dev->dev);
>> +	attach = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attach))
>>   		return ERR_CAST(attach);
>>   
>> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
>> index 4f80100ff5f3..8e6b6c879add 100644
>> --- a/drivers/gpu/drm/tegra/gem.c
>> +++ b/drivers/gpu/drm/tegra/gem.c
>> @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,
>>   static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>>   					struct dma_buf *buf)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = drm->dev,
>> +		.dmabuf = buf
>> +	};
>>   	struct tegra_drm *tegra = drm->dev_private;
>>   	struct dma_buf_attachment *attach;
>>   	struct tegra_bo *bo;
>> @@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>>   	if (IS_ERR(bo))
>>   		return bo;
>>   
>> -	attach = dma_buf_attach(buf, drm->dev);
>> +	attach = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attach)) {
>>   		err = PTR_ERR(attach);
>>   		goto free;
>> diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
>> index 556f62662aa9..86b928f9742f 100644
>> --- a/drivers/gpu/drm/udl/udl_dmabuf.c
>> +++ b/drivers/gpu/drm/udl/udl_dmabuf.c
>> @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev,
>>   struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>>   				struct dma_buf *dma_buf)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev->dev,
>> +		.dmabuf = dma_buf
>> +	};
>>   	struct dma_buf_attachment *attach;
>>   	struct sg_table *sg;
>>   	struct udl_gem_object *uobj;
>> @@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>>   
>>   	/* need to attach */
>>   	get_device(dev->dev);
>> -	attach = dma_buf_attach(dma_buf, dev->dev);
>> +	attach = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attach)) {
>>   		put_device(dev->dev);
>>   		return ERR_CAST(attach);
>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
>> index aff0ab7bf83d..1f2687b5eb0e 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
>> @@ -676,6 +676,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
>>   static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   	unsigned long size, enum dma_data_direction dma_dir)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev,
>> +		.dmabuf = dbuf
>> +	};
>>   	struct vb2_dc_buf *buf;
>>   	struct dma_buf_attachment *dba;
>>   
>> @@ -691,7 +695,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   
>>   	buf->dev = dev;
>>   	/* create attachment for the dmabuf with the user device */
>> -	dba = dma_buf_attach(dbuf, buf->dev);
>> +	dba = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(dba)) {
>>   		pr_err("failed to attach dmabuf\n");
>>   		kfree(buf);
>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> index 015e737095cd..cbd626d2393a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
>>   static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   	unsigned long size, enum dma_data_direction dma_dir)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev,
>> +		.dmabuf = dbuf
>> +	};
>>   	struct vb2_dma_sg_buf *buf;
>>   	struct dma_buf_attachment *dba;
>>   
>> @@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   
>>   	buf->dev = dev;
>>   	/* create attachment for the dmabuf with the user device */
>> -	dba = dma_buf_attach(dbuf, buf->dev);
>> +	dba = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(dba)) {
>>   		pr_err("failed to attach dmabuf\n");
>>   		kfree(buf);
>> diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
>> index aa6c6bba961e..5a10c1facc27 100644
>> --- a/drivers/staging/media/tegra-vde/tegra-vde.c
>> +++ b/drivers/staging/media/tegra-vde/tegra-vde.c
>> @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>>   				   size_t *size,
>>   				   enum dma_data_direction dma_dir)
>>   {
>> +	struct dma_buf_attach_info attach_info = {
>> +		.dev = dev,
>> +		.dmabuf = dmabuf
>> +	};
>>   	struct dma_buf_attachment *attachment;
>>   	struct dma_buf *dmabuf;
>>   	struct sg_table *sgt;
>> @@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>>   		return -EINVAL;
>>   	}
>>   
>> -	attachment = dma_buf_attach(dmabuf, dev);
>> +	attachment = dma_buf_attach(&attach_info);
>>   	if (IS_ERR(attachment)) {
>>   		dev_err(dev, "Failed to attach dmabuf\n");
>>   		err = PTR_ERR(attachment);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> index 58725f890b5b..2c312dfd31a1 100644
>> --- a/include/linux/dma-buf.h
>> +++ b/include/linux/dma-buf.h
>> @@ -359,6 +359,19 @@ struct dma_buf_export_info {
>>   	struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \
>>   					 .owner = THIS_MODULE }
>>   
>> +/**
>> + * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
>> + * @dmabuf:	the exported dma_buf
>> + * @dev:	the device which wants to import the attachment
>> + *
>> + * This structure holds the information required to attach to a buffer. Used
>> + * with dma_buf_attach() only.
>> + */
>> +struct dma_buf_attach_info {
>> +	struct dma_buf *dmabuf;
>> +	struct device *dev;
>> +};
>> +
>>   /**
>>    * get_dma_buf - convenience wrapper for get_file.
>>    * @dmabuf:	[in]	pointer to dma_buf
>> @@ -373,8 +386,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
>>   	get_file(dmabuf->file);
>>   }
>>   
>> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> -							struct device *dev);
>> +struct dma_buf_attachment *
>> +dma_buf_attach(const struct dma_buf_attach_info *info);
>>   void dma_buf_detach(struct dma_buf *dmabuf,
>>   				struct dma_buf_attachment *dmabuf_attach);
>>   
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Christian König]

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@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.

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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Daniel Vetter]

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@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Daniel Vetter]

On Fri, Apr 26, 2019 at 02:36:36PM +0200, Christian König wrote:
> The caching of SGT's is actually quite harmful and should probably removed
> altogether when all drivers are audited.
> 
> Start by providing a separate DMA-buf export implementation in amdgpu. This is
> also a prerequisite of unpinned DMA-buf handling.
> 
> v2: fix unintended recursion, remove debugging leftovers
> v3: split out from unpinned DMA-buf work
> v4: rebase on top of new no_sgt_cache flag
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

For my own understanding figured it'd be good to read the amdgpu patches
in more detail too. Some questions below.
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |   1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h    |   1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |   5 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c  | 162 +++++++++++++--------
>  4 files changed, 109 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 13a68f62bcc8..f1815223a1a1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -1254,7 +1254,6 @@ static struct drm_driver kms_driver = {
>  	.gem_prime_export = amdgpu_gem_prime_export,
>  	.gem_prime_import = amdgpu_gem_prime_import,
>  	.gem_prime_res_obj = amdgpu_gem_prime_res_obj,
> -	.gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table,
>  	.gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table,
>  	.gem_prime_vmap = amdgpu_gem_prime_vmap,
>  	.gem_prime_vunmap = amdgpu_gem_prime_vunmap,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
> index f1ddfc50bcc7..0c50d14a9739 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
> @@ -39,7 +39,6 @@ int amdgpu_gem_object_open(struct drm_gem_object *obj,
>  void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  				struct drm_file *file_priv);
>  unsigned long amdgpu_gem_timeout(uint64_t timeout_ns);
> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
>  struct drm_gem_object *
>  amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>  				 struct dma_buf_attachment *attach,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 93b2c5a48a71..d26e2f0b88d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -31,6 +31,7 @@
>   */
>  #include <linux/list.h>
>  #include <linux/slab.h>
> +#include <linux/dma-buf.h>
>  #include <drm/drmP.h>
>  #include <drm/amdgpu_drm.h>
>  #include <drm/drm_cache.h>
> @@ -1194,6 +1195,10 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
>  
>  	amdgpu_bo_kunmap(abo);
>  
> +	if (abo->gem_base.dma_buf && !abo->gem_base.import_attach &&
> +	    bo->mem.mem_type != TTM_PL_SYSTEM)
> +		dma_buf_invalidate_mappings(abo->gem_base.dma_buf);
> +
>  	/* remember the eviction */
>  	if (evict)
>  		atomic64_inc(&adev->num_evictions);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index a38e0fb4a6fe..cdc3c1e63a68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -40,22 +40,6 @@
>  #include <linux/dma-buf.h>
>  #include <linux/dma-fence-array.h>
>  
> -/**
> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table
> - * implementation
> - * @obj: GEM buffer object (BO)
> - *
> - * Returns:
> - * A scatter/gather table for the pinned pages of the BO's memory.
> - */
> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
> -{
> -	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> -	int npages = bo->tbo.num_pages;
> -
> -	return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
> -}
> -
>  /**
>   * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
>   * @obj: GEM BO
> @@ -231,34 +215,84 @@ __reservation_object_make_exclusive(struct reservation_object *obj)
>  }
>  
>  /**
> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> - * @dma_buf: Shared DMA buffer
> + * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation
> + *
> + * @dma_buf: DMA-buf to pin in memory
> + *
> + * Pin the BO which is backing the DMA-buf so that it can't move any more.
> + */
> +static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf)
> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +	/* pin buffer into GTT */
> +	return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);

This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
this can fail is someone already pinned the buffer into vram. And that
kind of checking is supposed to happen in the buffer attachment.

Also will p2p pin into VRAM if all attachments are p2p capable? Or is your
plan to require dynamic invalidate to avoid fragmenting vram badly with
pinned stuff you can't move?

> +}
> +
> +/**
> + * amdgpu_gem_unpin_dma_buf - &dma_buf_ops.unpin_dma_buf implementation
> + *
> + * @dma_buf: DMA-buf to unpin
> + *
> + * Unpin a previously pinned BO to make it movable again.
> + */
> +static void amdgpu_gem_unpin_dma_buf(struct dma_buf *dma_buf)
> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +	amdgpu_bo_unpin(bo);
> +}
> +
> +/**
> + * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation
> + *
> + * @dma_buf: DMA-buf we attach to
>   * @attach: DMA-buf attachment
>   *
> + * Returns:
> + * Always zero for success.
> + */
> +static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf,
> +				     struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +	/* Make sure the buffer is pinned when userspace didn't set GTT as
> +	 * preferred domain. This avoid ping/pong situations with scan out BOs.
> +	 */
> +	if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT))
> +		attach->invalidate = NULL;

Not following here at all. If the BO can't be in GTT I'd guess you should
reject the attach outright, since the pinning/map later on will fail I
guess? At least I'm not making the connection with why dynamic dma-buf
won't work anymore, since dynamic dma-buf is to make p2p of bo in vram
work better, which is exactly what this here seems to check for.

Or is this just a quick check until you add full p2p support?

Count me confused ...

> +
> +	return 0;
> +}
> +
> +/**
> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation
> + * @attach: DMA-buf attachment
> + * @dir: DMA direction
> + *
>   * Makes sure that the shared DMA buffer can be accessed by the target device.
>   * For now, simply pins it to the GTT domain, where it should be accessible by
>   * all DMA devices.
>   *
>   * Returns:
> - * 0 on success or a negative error code on failure.
> + * sg_table filled with the DMA addresses to use or ERR_PRT with negative error
> + * code.
>   */
> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> -				 struct dma_buf_attachment *attach)
> +static struct sg_table *
> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach,
> +		       enum dma_data_direction dir)
>  {
> +	struct dma_buf *dma_buf = attach->dmabuf;
>  	struct drm_gem_object *obj = dma_buf->priv;
>  	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> +	struct sg_table *sgt;
>  	long r;
>  
> -	r = drm_gem_map_attach(dma_buf, attach);
> -	if (r)
> -		return r;
> -
> -	r = amdgpu_bo_reserve(bo, false);
> -	if (unlikely(r != 0))
> -		goto error_detach;
> -
> -
>  	if (attach->dev->driver != adev->dev->driver) {
>  		/*
>  		 * We only create shared fences for internal use, but importers
> @@ -270,53 +304,64 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>  		 */
>  		r = __reservation_object_make_exclusive(bo->tbo.resv);
>  		if (r)
> -			goto error_unreserve;
> +			return ERR_PTR(r);
>  	}
>  
> -	/* pin buffer into GTT */
> -	r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
> -	if (r)
> -		goto error_unreserve;
> +	if (attach->invalidate) {
> +		/* move buffer into GTT */
> +		struct ttm_operation_ctx ctx = { false, false };
> +
> +		amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
> +		r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> +		if (r)
> +			return ERR_PTR(r);
> +	}
> +
> +	sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages);
> +	if (IS_ERR(sgt))
> +		return sgt;
> +
> +	if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
> +			      DMA_ATTR_SKIP_CPU_SYNC))
> +		goto error_free;
>  
>  	if (attach->dev->driver != adev->dev->driver)
>  		bo->prime_shared_count++;
>  
> -error_unreserve:
> -	amdgpu_bo_unreserve(bo);
> +	return sgt;
>  
> -error_detach:
> -	if (r)
> -		drm_gem_map_detach(dma_buf, attach);
> -	return r;
> +error_free:
> +	sg_free_table(sgt);
> +	kfree(sgt);
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  /**
> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation
> - * @dma_buf: Shared DMA buffer
> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation
>   * @attach: DMA-buf attachment
> + * @sgt: sg_table to unmap
> + * @dir: DMA direction
>   *
>   * This is called when a shared DMA buffer no longer needs to be accessible by
>   * another device. For now, simply unpins the buffer from GTT.
>   */
> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf,
> -				  struct dma_buf_attachment *attach)
> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> +				     struct sg_table *sgt,
> +				     enum dma_data_direction dir)
>  {
> +	struct dma_buf *dma_buf = attach->dmabuf;
>  	struct drm_gem_object *obj = dma_buf->priv;
>  	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> -	int ret = 0;
> -
> -	ret = amdgpu_bo_reserve(bo, true);
> -	if (unlikely(ret != 0))
> -		goto error;
>  
> -	amdgpu_bo_unpin(bo);
>  	if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count)
>  		bo->prime_shared_count--;
> -	amdgpu_bo_unreserve(bo);
>  
> -error:
> -	drm_gem_map_detach(dma_buf, attach);
> +	if (sgt) {

This check seems to be leftovers from the sgt caching in drm_prime.c. With
the new code organization I don't think you need this check, looks like it
would paper over dma-buf.c or importer bugs.

Cheers, Daniel

> +		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
> +		sg_free_table(sgt);
> +		kfree(sgt);
> +	}
>  }
>  
>  /**
> @@ -374,10 +419,11 @@ static int amdgpu_gem_begin_cpu_access(struct dma_buf *dma_buf,
>  }
>  
>  const struct dma_buf_ops amdgpu_dmabuf_ops = {
> -	.attach = amdgpu_gem_map_attach,
> -	.detach = amdgpu_gem_map_detach,
> -	.map_dma_buf = drm_gem_map_dma_buf,
> -	.unmap_dma_buf = drm_gem_unmap_dma_buf,
> +	.pin = amdgpu_gem_pin_dma_buf,
> +	.unpin = amdgpu_gem_unpin_dma_buf,
> +	.attach = amdgpu_gem_dma_buf_attach,
> +	.map_dma_buf = amdgpu_gem_map_dma_buf,
> +	.unmap_dma_buf = amdgpu_gem_unmap_dma_buf,
>  	.release = drm_gem_dmabuf_release,
>  	.begin_cpu_access = amdgpu_gem_begin_cpu_access,
>  	.mmap = drm_gem_dmabuf_mmap,
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/12] dma-buf: start caching of sg_table objects

[Daniel Vetter]

On Fri, Apr 26, 2019 at 02:36:29PM +0200, Christian König wrote:
> To allow a smooth transition from pinning buffer objects to dynamic
> invalidation we first start to cache the sg_table for an attachment
> unless the driver has implemented the explicite pin/unpin callbacks.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

btw given how huge a change this is the doc update is rather thin :-)

Probably better to revise this all together, so we have a coherent
documentation story between static and dynamic dma-buf attachments and
what exactly that means in each case.

> ---
>  drivers/dma-buf/dma-buf.c | 24 ++++++++++++++++++++++++
>  include/linux/dma-buf.h   |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 0656dcf289be..a18d10c4425a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -610,6 +610,20 @@ dma_buf_attach(const struct dma_buf_attach_info *info)
>  	list_add(&attach->node, &dmabuf->attachments);
>  
>  	mutex_unlock(&dmabuf->lock);
> +
> +	if (!dmabuf->ops->pin) {
> +		struct sg_table *sgt;
> +
> +		sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> +		if (!sgt)
> +			sgt = ERR_PTR(-ENOMEM);
> +		if (IS_ERR(sgt)) {
> +			dma_buf_detach(dmabuf, attach);
> +			return ERR_CAST(sgt);
> +		}
> +		attach->sgt = sgt;
> +	}
> +
>  	return attach;
>  
>  err_attach:
> @@ -632,6 +646,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>  	if (WARN_ON(!dmabuf || !attach))
>  		return;
>  
> +	if (attach->sgt)
> +		dmabuf->ops->unmap_dma_buf(attach, attach->sgt,
> +					   DMA_BIDIRECTIONAL);
> +
>  	mutex_lock(&dmabuf->lock);
>  	list_del(&attach->node);
>  	if (dmabuf->ops->detach)
> @@ -668,6 +686,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf))
>  		return ERR_PTR(-EINVAL);
>  
> +	if (attach->sgt)
> +		return attach->sgt;
> +
>  	reservation_object_lock(attach->dmabuf->resv, NULL);
>  	r = dma_buf_pin(attach->dmabuf);
>  	reservation_object_unlock(attach->dmabuf->resv);
> @@ -701,6 +722,9 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>  	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
>  		return;
>  
> +	if (attach->sgt == sg_table)
> +		return;
> +
>  	attach->dmabuf->ops->unmap_dma_buf(attach, sg_table,
>  						direction);
>  
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 0321939b1c3d..b9d0719581cd 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -345,6 +345,7 @@ struct dma_buf_attachment {
>  	struct dma_buf *dmabuf;
>  	struct device *dev;
>  	struct list_head node;
> +	struct sg_table *sgt;
>  	void *priv;
>  };
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Christian König]

Am 30.04.19 um 15:59 schrieb Daniel Vetter:
> 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:
>>>> [SNIP]
>>>> +/**
>>>> + * 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?

Yes, exactly that is one of the final goals of this.

We could of course use the attachment here, but that would disqualify 
the exporter calling this directly without an attachment.

Regards,
Christian.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Daniel Vetter]

On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote:
> Am 30.04.19 um 15:59 schrieb Daniel Vetter:
> > 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:
> > > > > [SNIP]
> > > > > +/**
> > > > > + * 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?
> 
> Yes, exactly that is one of the final goals of this.
> 
> We could of course use the attachment here, but that would disqualify the
> exporter calling this directly without an attachment.

Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted
lasagna :-)

I do understand the goal, all these imported/exporter special cases in
code are a bit silly, but I think the proper fix would be if you just
always import a buffer, even the private ones, allocated against some
exporter-only thing. Then it's always the same function to call.

But I'm not sure this is a good reasons to guide the dma-buf interfaces
for everyone else. Moving pin to the attachment sounds like a better idea
(if the above is the only reason to keep it on the dma-buf).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Koenig, Christian]

Am 30.04.19 um 16:34 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote:
>> Am 30.04.19 um 15:59 schrieb Daniel Vetter:
>>> 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:
>>>>>> [SNIP]
>>>>>> +/**
>>>>>> + * 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?
>> Yes, exactly that is one of the final goals of this.
>>
>> We could of course use the attachment here, but that would disqualify the
>> exporter calling this directly without an attachment.
> Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted
> lasagna :-)
>
> I do understand the goal, all these imported/exporter special cases in
> code are a bit silly, but I think the proper fix would be if you just
> always import a buffer, even the private ones, allocated against some
> exporter-only thing. Then it's always the same function to call.
>
> But I'm not sure this is a good reasons to guide the dma-buf interfaces
> for everyone else. Moving pin to the attachment sounds like a better idea
> (if the above is the only reason to keep it on the dma-buf).

Yeah, that's on my mind as well. But I'm running into a chicken and egg 
problem here again.

Basically we need to convert the drivers to do their internal operation 
using the DMA-buf as top level object first and then we can switch all 
internal operation to using a "normal" attachment.

Additional to that it simply doesn't looks like the right idea to use 
the attachment as parameter here. After all we pin the underlying 
DMA-buf and NOT the attachment.

Christian.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] dma-buf: add explicit buffer pinning v2

[Daniel Vetter]

On Tue, Apr 30, 2019 at 4:41 PM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 30.04.19 um 16:34 schrieb Daniel Vetter:
> > [CAUTION: External Email]
> >
> > On Tue, Apr 30, 2019 at 04:26:32PM +0200, Christian König wrote:
> >> Am 30.04.19 um 15:59 schrieb Daniel Vetter:
> >>> 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:
> >>>>>> [SNIP]
> >>>>>> +/**
> >>>>>> + * 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?
> >> Yes, exactly that is one of the final goals of this.
> >>
> >> We could of course use the attachment here, but that would disqualify the
> >> exporter calling this directly without an attachment.
> > Hm exporter calling dma_buf_pin/unpin sounds like one seriously inverted
> > lasagna :-)
> >
> > I do understand the goal, all these imported/exporter special cases in
> > code are a bit silly, but I think the proper fix would be if you just
> > always import a buffer, even the private ones, allocated against some
> > exporter-only thing. Then it's always the same function to call.
> >
> > But I'm not sure this is a good reasons to guide the dma-buf interfaces
> > for everyone else. Moving pin to the attachment sounds like a better idea
> > (if the above is the only reason to keep it on the dma-buf).
>
> Yeah, that's on my mind as well. But I'm running into a chicken and egg
> problem here again.

Yeah the usual refactor story :-/

> Basically we need to convert the drivers to do their internal operation
> using the DMA-buf as top level object first and then we can switch all
> internal operation to using a "normal" attachment.
>
> Additional to that it simply doesn't looks like the right idea to use
> the attachment as parameter here. After all we pin the underlying
> DMA-buf and NOT the attachment.

We pin both imo - I'd be really surprised as an importer if I attach,
pin, and then the exporter tells me I can't map the attachment I just
made anymore because the buffer is in the wrong place. That's imo what
pin really should make sure is assured - we already require this for
the static importers to be true (which is also why caching makes
sense). Hence for me global pin = pin all the attachments.

I also dropped some questions on the amdgpu patches to hopefully
better understand all this with actual implementations.

btw totally different thought: For dynamic exporters, should we drop
the cached sgt on the floor if it's not in use? the drm_prime.c
helpers don't need this since they map all the time, but afaiui at
least v4l and i915 don't hang onto a mapping forever, only when
actually needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Christian König]

Am 30.04.19 um 16:16 schrieb Daniel Vetter:
> [SNIP]
>>   /**
>> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>> - * @dma_buf: Shared DMA buffer
>> + * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation
>> + *
>> + * @dma_buf: DMA-buf to pin in memory
>> + *
>> + * Pin the BO which is backing the DMA-buf so that it can't move any more.
>> + */
>> +static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf)
>> +{
>> +	struct drm_gem_object *obj = dma_buf->priv;
>> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>> +
>> +	/* pin buffer into GTT */
>> +	return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
> This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
> this can fail is someone already pinned the buffer into vram. And that
> kind of checking is supposed to happen in the buffer attachment.

Why is that supposed to happen on the attachment? I mean it could be 
nice to have for debugging, but I still don't see any practical reason 
for this.

> Also will p2p pin into VRAM if all attachments are p2p capable? Or is your
> plan to require dynamic invalidate to avoid fragmenting vram badly with
> pinned stuff you can't move?

My plan was to make dynamic invalidation a must have for P2P, exactly 
for the reason you noted.

> +/**
> + * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation
> + *
> + * @dma_buf: DMA-buf we attach to
>    * @attach: DMA-buf attachment
>    *
> + * Returns:
> + * Always zero for success.
> + */
> +static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf,
> +				     struct dma_buf_attachment *attach)
> +{
> +	struct drm_gem_object *obj = dma_buf->priv;
> +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> +
> +	/* Make sure the buffer is pinned when userspace didn't set GTT as
> +	 * preferred domain. This avoid ping/pong situations with scan out BOs.
> +	 */
> +	if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT))
> +		attach->invalidate = NULL;
> Not following here at all. If the BO can't be in GTT I'd guess you should
> reject the attach outright, since the pinning/map later on will fail I
> guess? At least I'm not making the connection with why dynamic dma-buf
> won't work anymore, since dynamic dma-buf is to make p2p of bo in vram
> work better, which is exactly what this here seems to check for.
>
> Or is this just a quick check until you add full p2p support?
>
> Count me confused ...

Well completely amdgpu internal handling here. Key point is we have both 
preferred_domains as well as allowed_domains.

During command submission we always try to move a BO to the 
preferred_domains again.

Now what could happen if we don't have this check is the following:

1. BO is allocate in VRAM. And preferred_domains says only VRAM please, 
but allowed_domains says VRAM or GTT.

2. DMA-buf Importer comes along and moves the BO to GTT, which is 
perfectly valid because of the allowed_domains.

3. Command submission is made and moves the BO to VRAM again.

4. Importer comes along and moves the BO to GTT.
....

E.g. a nice ping/pong situation which just eats up memory bandwidth.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Daniel Vetter]

On Fri, May 03, 2019 at 02:35:02PM +0200, Christian König wrote:
> Am 30.04.19 um 16:16 schrieb Daniel Vetter:
> > [SNIP]
> > >   /**
> > > - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> > > - * @dma_buf: Shared DMA buffer
> > > + * amdgpu_gem_pin_dma_buf - &dma_buf_ops.pin_dma_buf implementation
> > > + *
> > > + * @dma_buf: DMA-buf to pin in memory
> > > + *
> > > + * Pin the BO which is backing the DMA-buf so that it can't move any more.
> > > + */
> > > +static int amdgpu_gem_pin_dma_buf(struct dma_buf *dma_buf)
> > > +{
> > > +	struct drm_gem_object *obj = dma_buf->priv;
> > > +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> > > +
> > > +	/* pin buffer into GTT */
> > > +	return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
> > This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
> > this can fail is someone already pinned the buffer into vram. And that
> > kind of checking is supposed to happen in the buffer attachment.
> 
> Why is that supposed to happen on the attachment? I mean it could be nice to
> have for debugging, but I still don't see any practical reason for this.

dma_buf_attach is supposed to make sure the buffer won't land somewhere
where you can't get at it anymore. Wrt pin that means the exporter needs
to make sure it can't get pinned into a wrong place, and also isn't pinned
into a wrong place anymore. That's why I think pinning ties in with
dma_buf_attach and not the overall buffer.

In a way there's two pieces of a pin:
- Do not move the buffer anymore.
- Make sure I can still get at it.

Internally the 2nd part is encoded in the domain parameter you pass to
amdgpu_bo_pin. When going through dma-buf that information is derived
from the attachment (e.g. if it's a p2p one, then you can put it wherever
you feel like, if it's a normal one it must be in system ram). The dma-buf
alone doesn't tell you _where_ to pin something.

> > Also will p2p pin into VRAM if all attachments are p2p capable? Or is your
> > plan to require dynamic invalidate to avoid fragmenting vram badly with
> > pinned stuff you can't move?
> 
> My plan was to make dynamic invalidation a must have for P2P, exactly for
> the reason you noted.
> 
> > +/**
> > + * amdgpu_gem_dma_buf_attach - &dma_buf_ops.attach implementation
> > + *
> > + * @dma_buf: DMA-buf we attach to
> >    * @attach: DMA-buf attachment
> >    *
> > + * Returns:
> > + * Always zero for success.
> > + */
> > +static int amdgpu_gem_dma_buf_attach(struct dma_buf *dma_buf,
> > +				     struct dma_buf_attachment *attach)
> > +{
> > +	struct drm_gem_object *obj = dma_buf->priv;
> > +	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
> > +
> > +	/* Make sure the buffer is pinned when userspace didn't set GTT as
> > +	 * preferred domain. This avoid ping/pong situations with scan out BOs.
> > +	 */
> > +	if (!(bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT))
> > +		attach->invalidate = NULL;
> > Not following here at all. If the BO can't be in GTT I'd guess you should
> > reject the attach outright, since the pinning/map later on will fail I
> > guess? At least I'm not making the connection with why dynamic dma-buf
> > won't work anymore, since dynamic dma-buf is to make p2p of bo in vram
> > work better, which is exactly what this here seems to check for.
> > 
> > Or is this just a quick check until you add full p2p support?
> > 
> > Count me confused ...
> 
> Well completely amdgpu internal handling here. Key point is we have both
> preferred_domains as well as allowed_domains.
> 
> During command submission we always try to move a BO to the
> preferred_domains again.
> 
> Now what could happen if we don't have this check is the following:
> 
> 1. BO is allocate in VRAM. And preferred_domains says only VRAM please, but
> allowed_domains says VRAM or GTT.
> 
> 2. DMA-buf Importer comes along and moves the BO to GTT, which is perfectly
> valid because of the allowed_domains.
> 
> 3. Command submission is made and moves the BO to VRAM again.
> 
> 4. Importer comes along and moves the BO to GTT.
> ....
> 
> E.g. a nice ping/pong situation which just eats up memory bandwidth.

Hm yeah the ping/pong is bad, but I figure you have to already handle that
(with some bias or whatever). Outright disabling invalidate/dynamic
dma-buf seems like overkill.

What about upgradging preferred_domains to include GTT here? Defacto what
you do is forcing GTT, so just adding GTT as a possible domain seems like
the better choice. Bonus points for undoing that when the last importer
disappears.

In general I think dynamic dma-buf needs to be able to handle this
somehow, or it won't really work. Simplest is probably to just stop moving
buffers around so much for buffers that are dynamically exported (so maybe
could also change that in the CS ioctl to not move exported buffers
anymore, would achieve the same).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Koenig, Christian]

Am 06.05.19 um 10:04 schrieb Daniel Vetter:
> [SNIP]
>>>> + /* pin buffer into GTT */
>>>> + return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
>>> This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
>>> this can fail is someone already pinned the buffer into vram. And that
>>> kind of checking is supposed to happen in the buffer attachment.
>> Why is that supposed to happen on the attachment? I mean it could be nice to
>> have for debugging, but I still don't see any practical reason for this.
> dma_buf_attach is supposed to make sure the buffer won't land somewhere
> where you can't get at it anymore. Wrt pin that means the exporter needs
> to make sure it can't get pinned into a wrong place, and also isn't pinned
> into a wrong place anymore. That's why I think pinning ties in with
> dma_buf_attach and not the overall buffer.
>
> In a way there's two pieces of a pin:
> - Do not move the buffer anymore.
> - Make sure I can still get at it.
>
> Internally the 2nd part is encoded in the domain parameter you pass to
> amdgpu_bo_pin. When going through dma-buf that information is derived
> from the attachment (e.g. if it's a p2p one, then you can put it wherever
> you feel like, if it's a normal one it must be in system ram). The dma-buf
> alone doesn't tell you _where_ to pin something.

Ok, that finally makes some sense. So the attachment describes where the 
buffer needs to be for the attaching device/use case to be able to 
access it.

Going to change it to use an attachment instead.

>> Well completely amdgpu internal handling here. Key point is we have both
>> preferred_domains as well as allowed_domains.
>>
>> During command submission we always try to move a BO to the
>> preferred_domains again.
>>
>> Now what could happen if we don't have this check is the following:
>>
>> 1. BO is allocate in VRAM. And preferred_domains says only VRAM please, but
>> allowed_domains says VRAM or GTT.
>>
>> 2. DMA-buf Importer comes along and moves the BO to GTT, which is perfectly
>> valid because of the allowed_domains.
>>
>> 3. Command submission is made and moves the BO to VRAM again.
>>
>> 4. Importer comes along and moves the BO to GTT.
>> ....
>>
>> E.g. a nice ping/pong situation which just eats up memory bandwidth.
> Hm yeah the ping/pong is bad, but I figure you have to already handle that
> (with some bias or whatever). Outright disabling invalidate/dynamic
> dma-buf seems like overkill.
>
> What about upgradging preferred_domains to include GTT here? Defacto what
> you do is forcing GTT, so just adding GTT as a possible domain seems like
> the better choice. Bonus points for undoing that when the last importer
> disappears.

Well that's exactly what we want to avoid here.

The preferred_domains is where userspace likes the buffer to be and 
should never be changed by the kernel.

The allowed_domains is where the buffer should be based on the hardware 
restrictions and is usually updated by the kernel driver.

> In general I think dynamic dma-buf needs to be able to handle this
> somehow, or it won't really work. Simplest is probably to just stop moving
> buffers around so much for buffers that are dynamically exported (so maybe
> could also change that in the CS ioctl to not move exported buffers
> anymore, would achieve the same).

Yeah, that's the obvious alternative. But I didn't wanted to add even 
more complexity to the patch right now.

Cleaning this up is pure amdgpu internally, e.g. we need to make sure to 
not move buffers around so much on command submission.

Christian.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/12] drm/amdgpu: add independent DMA-buf export v3

[Daniel Vetter]

On Mon, May 06, 2019 at 10:05:07AM +0000, Koenig, Christian wrote:
> Am 06.05.19 um 10:04 schrieb Daniel Vetter:
> > [SNIP]
> >>>> + /* pin buffer into GTT */
> >>>> + return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT);
> >>> This is kinda what I mean with "shouldn't we pin the attachment" - afaiui
> >>> this can fail is someone already pinned the buffer into vram. And that
> >>> kind of checking is supposed to happen in the buffer attachment.
> >> Why is that supposed to happen on the attachment? I mean it could be nice to
> >> have for debugging, but I still don't see any practical reason for this.
> > dma_buf_attach is supposed to make sure the buffer won't land somewhere
> > where you can't get at it anymore. Wrt pin that means the exporter needs
> > to make sure it can't get pinned into a wrong place, and also isn't pinned
> > into a wrong place anymore. That's why I think pinning ties in with
> > dma_buf_attach and not the overall buffer.
> >
> > In a way there's two pieces of a pin:
> > - Do not move the buffer anymore.
> > - Make sure I can still get at it.
> >
> > Internally the 2nd part is encoded in the domain parameter you pass to
> > amdgpu_bo_pin. When going through dma-buf that information is derived
> > from the attachment (e.g. if it's a p2p one, then you can put it wherever
> > you feel like, if it's a normal one it must be in system ram). The dma-buf
> > alone doesn't tell you _where_ to pin something.
> 
> Ok, that finally makes some sense. So the attachment describes where the 
> buffer needs to be for the attaching device/use case to be able to 
> access it.
> 
> Going to change it to use an attachment instead.

btw one thing we've discussed, and much easier to implement on most socs
is telling apart the render vs display attachment. Most socs have
different devices for that, and different pin requirements. We could do
something similarly-ish on pci too, where you attach to some sub-device
representing scanout. Or maybe a flag on the attachment. Either way, that
would indicate an attachment where the pin indicates "must be in vram".
Might be useful for SLI rendered scanout (in case that ever happens).

> >> Well completely amdgpu internal handling here. Key point is we have both
> >> preferred_domains as well as allowed_domains.
> >>
> >> During command submission we always try to move a BO to the
> >> preferred_domains again.
> >>
> >> Now what could happen if we don't have this check is the following:
> >>
> >> 1. BO is allocate in VRAM. And preferred_domains says only VRAM please, but
> >> allowed_domains says VRAM or GTT.
> >>
> >> 2. DMA-buf Importer comes along and moves the BO to GTT, which is perfectly
> >> valid because of the allowed_domains.
> >>
> >> 3. Command submission is made and moves the BO to VRAM again.
> >>
> >> 4. Importer comes along and moves the BO to GTT.
> >> ....
> >>
> >> E.g. a nice ping/pong situation which just eats up memory bandwidth.
> > Hm yeah the ping/pong is bad, but I figure you have to already handle that
> > (with some bias or whatever). Outright disabling invalidate/dynamic
> > dma-buf seems like overkill.
> >
> > What about upgradging preferred_domains to include GTT here? Defacto what
> > you do is forcing GTT, so just adding GTT as a possible domain seems like
> > the better choice. Bonus points for undoing that when the last importer
> > disappears.
> 
> Well that's exactly what we want to avoid here.
> 
> The preferred_domains is where userspace likes the buffer to be and 
> should never be changed by the kernel.
> 
> The allowed_domains is where the buffer should be based on the hardware 
> restrictions and is usually updated by the kernel driver.
> 
> > In general I think dynamic dma-buf needs to be able to handle this
> > somehow, or it won't really work. Simplest is probably to just stop moving
> > buffers around so much for buffers that are dynamically exported (so maybe
> > could also change that in the CS ioctl to not move exported buffers
> > anymore, would achieve the same).
> 
> Yeah, that's the obvious alternative. But I didn't wanted to add even 
> more complexity to the patch right now.
> 
> Cleaning this up is pure amdgpu internally, e.g. we need to make sure to 
> not move buffers around so much on command submission.

Ok if the comment is clear enough for you folks that's all fine. Maybe add
a FIXME about how this is a bit pessimistic and what could/should be done
instead.

I agree that it's probably not a use-case you really care about, since the
big piles of dynamic shared buffers will be p2p. Dynamic, but not p2p is a
bit an edge case really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel