All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] dma-buf: add caching of sg_table
@ 2018-07-18 10:47 Christian König
  2018-07-18 10:47 ` [PATCH 2/4] drm: remove prime sg_table caching Christian König
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Christian König @ 2018-07-18 10:47 UTC (permalink / raw)
  To: intel-gfx

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 explicitly says to not do so.

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

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 13884474d158..0bea5eecf554 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -574,6 +574,20 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
 	list_add(&attach->node, &dmabuf->attachments);
 
 	mutex_unlock(&dmabuf->lock);
+
+	if (!dmabuf->ops->no_sgt_cache) {
+		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:
@@ -596,6 +610,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)
@@ -631,6 +649,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;
+
 	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
@@ -658,6 +679,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 58725f890b5b..6534a6769e17 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -51,6 +51,16 @@ struct dma_buf_attachment;
  * @vunmap: [optional] unmaps a vmap from the buffer
  */
 struct dma_buf_ops {
+	/**
+	 * @no_sgt_cache:
+	 *
+	 * Flag controlling the caching of the sg_table in the DMA-buf helpers.
+	 * If not set the sg_table is created during device attaching, if set
+	 * the sg_table is created dynamically when dma_buf_map_attachment() is
+	 * called.
+	 */
+	bool no_sgt_cache;
+
 	/**
 	 * @attach:
 	 *
@@ -323,6 +333,7 @@ struct dma_buf_attachment {
 	struct device *dev;
 	struct list_head node;
 	void *priv;
+	struct sg_table *sgt;
 };
 
 /**
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] drm: remove prime sg_table caching
  2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
@ 2018-07-18 10:47 ` Christian König
  2018-07-18 10:47 ` [PATCH 3/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v3 Christian König
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2018-07-18 10:47 UTC (permalink / raw)
  To: intel-gfx

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 | 78 +++++++++++----------------------------------
 1 file changed, 18 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 186db2e4c57a..f6bd37eb2a72 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,26 +183,17 @@ 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;
 	struct drm_device *dev = obj->dev;
 
-	prime_attach = kzalloc(sizeof(*prime_attach), GFP_KERNEL);
-	if (!prime_attach)
-		return -ENOMEM;
-
-	prime_attach->dir = DMA_NONE;
-	attach->priv = prime_attach;
-
 	if (!dev->driver->gem_prime_pin)
 		return 0;
 
@@ -226,27 +212,9 @@ 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;
 	struct drm_device *dev = obj->dev;
 
-	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;
-	}
-
 	if (dev->driver->gem_prime_unpin)
 		dev->driver->gem_prime_unpin(obj);
 }
@@ -292,36 +260,21 @@ 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);
-
 	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+	if (IS_ERR(sgt))
+		return sgt;
 
-	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;
@@ -334,14 +287,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.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v3
  2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
  2018-07-18 10:47 ` [PATCH 2/4] drm: remove prime sg_table caching Christian König
@ 2018-07-18 10:47 ` Christian König
  2018-07-18 10:47 ` [PATCH 4/4] dma-buf: lock the reservation object during (un)map_dma_buf v3 Christian König
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2018-07-18 10:47 UTC (permalink / raw)
  To: intel-gfx

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

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

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 0bea5eecf554..e68322c953a9 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -624,6 +624,43 @@ 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_locked(). 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;
+
+	might_sleep();
+	reservation_object_assert_held(attach->dmabuf->resv);
+
+	if (attach->sgt)
+		return attach->sgt;
+
+	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	if (!sg_table)
+		sg_table = 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
@@ -660,6 +697,32 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 }
 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);
+}
+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
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 6534a6769e17..a40f8f586a95 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -395,8 +395,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.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] dma-buf: lock the reservation object during (un)map_dma_buf v3
  2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
  2018-07-18 10:47 ` [PATCH 2/4] drm: remove prime sg_table caching Christian König
  2018-07-18 10:47 ` [PATCH 3/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v3 Christian König
@ 2018-07-18 10:47 ` Christian König
  2018-07-18 11:46 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] dma-buf: add caching of sg_table Patchwork
  2018-07-18 12:07 ` ✗ Fi.CI.BAT: failure " Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Christian König @ 2018-07-18 10:47 UTC (permalink / raw)
  To: intel-gfx

First step towards unpinned DMA buf operation.

I've checked the DRM drivers to potential locking of the reservation
object, but essentially we need to audit all implementations of the
dma_buf _ops for this to work.

v2: reordered
v3: rebased on sgt caching

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

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index e68322c953a9..21608306349d 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -686,10 +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;
-
-	sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
+	reservation_object_lock(attach->dmabuf->resv, NULL);
+	sg_table = dma_buf_map_attachment_locked(attach, direction);
+	reservation_object_unlock(attach->dmabuf->resv);
 	if (!sg_table)
 		sg_table = ERR_PTR(-ENOMEM);
 
@@ -742,11 +741,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);
+	reservation_object_lock(attach->dmabuf->resv, NULL);
+	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 a40f8f586a95..e964077a7e7e 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -128,6 +128,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
+	 * no_sgt_cache is true.
+	 *
 	 * Returns:
 	 *
 	 * A &sg_table scatter list of or the backing storage of the DMA buffer,
@@ -148,6 +151,9 @@ struct dma_buf_ops {
 	 * It should also unpin the backing storage if this is the last mapping
 	 * of the DMA buffer, it the exporter supports backing storage
 	 * migration.
+	 *
+	 * This is always called with the dmabuf->resv object locked when
+	 * no_sgt_cache is true.
 	 */
 	void (*unmap_dma_buf)(struct dma_buf_attachment *,
 			      struct sg_table *,
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] dma-buf: add caching of sg_table
  2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
                   ` (2 preceding siblings ...)
  2018-07-18 10:47 ` [PATCH 4/4] dma-buf: lock the reservation object during (un)map_dma_buf v3 Christian König
@ 2018-07-18 11:46 ` Patchwork
  2018-07-18 12:07 ` ✗ Fi.CI.BAT: failure " Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-07-18 11:46 UTC (permalink / raw)
  To: Christian König; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] dma-buf: add caching of sg_table
URL   : https://patchwork.freedesktop.org/series/46778/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6172482908fe dma-buf: add caching of sg_table
5d400e5a92be drm: remove prime sg_table caching
0b5071f7f764 dma-buf: add dma_buf_(un)map_attachment_locked variants v3
-:106: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct dma_buf_attachment *' should also have an identifier name
#106: FILE: include/linux/dma-buf.h:398:
+struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,

-:110: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct dma_buf_attachment *' should also have an identifier name
#110: FILE: include/linux/dma-buf.h:402:
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,

-:110: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct sg_table *' should also have an identifier name
#110: FILE: include/linux/dma-buf.h:402:
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *,

total: 0 errors, 3 warnings, 0 checks, 88 lines checked
c16d15878445 dma-buf: lock the reservation object during (un)map_dma_buf v3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/4] dma-buf: add caching of sg_table
  2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
                   ` (3 preceding siblings ...)
  2018-07-18 11:46 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] dma-buf: add caching of sg_table Patchwork
@ 2018-07-18 12:07 ` Patchwork
  2018-07-18 13:24   ` Christian König
  4 siblings, 1 reply; 10+ messages in thread
From: Patchwork @ 2018-07-18 12:07 UTC (permalink / raw)
  To: Christian König; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/4] dma-buf: add caching of sg_table
URL   : https://patchwork.freedesktop.org/series/46778/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4503 -> Patchwork_9705 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9705 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9705, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46778/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9705:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_hangcheck:
      fi-cfl-guc:         PASS -> DMESG-FAIL

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      {fi-cfl-8109u}:     PASS -> DMESG-WARN
      fi-glk-dsi:         PASS -> DMESG-WARN
      fi-hsw-peppy:       PASS -> DMESG-WARN
      fi-bsw-n3050:       PASS -> DMESG-WARN
      fi-bxt-j4205:       PASS -> DMESG-WARN
      fi-skl-6700hq:      PASS -> DMESG-WARN
      fi-cfl-guc:         PASS -> DMESG-WARN
      fi-cfl-s3:          PASS -> DMESG-WARN
      fi-bxt-dsi:         PASS -> DMESG-WARN
      fi-ivb-3770:        PASS -> DMESG-WARN
      fi-skl-6700k2:      PASS -> DMESG-WARN
      fi-whl-u:           PASS -> DMESG-WARN
      fi-cfl-8700k:       PASS -> DMESG-WARN
      fi-byt-j1900:       PASS -> DMESG-WARN
      fi-hsw-4770:        PASS -> DMESG-WARN
      fi-kbl-7560u:       PASS -> DMESG-WARN
      fi-skl-6600u:       PASS -> DMESG-WARN
      fi-kbl-guc:         PASS -> DMESG-WARN
      {fi-kbl-8809g}:     NOTRUN -> DMESG-WARN
      fi-kbl-7567u:       PASS -> DMESG-WARN
      fi-skl-guc:         PASS -> DMESG-WARN
      fi-glk-j4005:       PASS -> DMESG-WARN
      fi-bdw-5557u:       PASS -> DMESG-WARN
      fi-bwr-2160:        PASS -> DMESG-WARN
      fi-kbl-r:           PASS -> DMESG-WARN
      fi-blb-e6850:       PASS -> DMESG-WARN
      fi-kbl-x1275:       PASS -> DMESG-WARN
      fi-skl-gvtdvm:      PASS -> DMESG-WARN
      fi-hsw-4770r:       PASS -> DMESG-WARN
      fi-skl-6260u:       PASS -> DMESG-WARN
      fi-snb-2600:        PASS -> DMESG-WARN
      fi-ilk-650:         PASS -> DMESG-WARN
      fi-elk-e7500:       PASS -> DMESG-WARN
      fi-skl-6770hq:      PASS -> DMESG-WARN
      fi-byt-n2820:       PASS -> DMESG-WARN
      fi-snb-2520m:       PASS -> DMESG-WARN
      fi-pnv-d510:        PASS -> DMESG-WARN
      fi-kbl-7500u:       PASS -> DMESG-WARN
      fi-ivb-3520m:       PASS -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9705 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-guc:         PASS -> DMESG-FAIL (fdo#106947)

    igt@gem_exec_suspend@basic-s4-devices:
      {fi-kbl-8809g}:     NOTRUN -> DMESG-WARN (fdo#107139, fdo#107222)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#102614, fdo#106103)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      {fi-cfl-8109u}:     DMESG-FAIL (fdo#106560) -> PASS

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     DMESG-FAIL -> PASS

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       FAIL (fdo#103841) -> PASS +1

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107222 https://bugs.freedesktop.org/show_bug.cgi?id=107222


== Participating hosts (46 -> 41) ==

  Additional (1): fi-kbl-8809g 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-bdw-gvtdvm fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4503 -> Patchwork_9705

  CI_DRM_4503: 4aa6797dfafaf527949bf55d3c8513c6902dfec2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4562: 8781fd89a63eabed9359d02b50583cca67ff3673 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9705: c16d158784451158cc11756c1325cd6d889dd103 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c16d15878445 dma-buf: lock the reservation object during (un)map_dma_buf v3
0b5071f7f764 dma-buf: add dma_buf_(un)map_attachment_locked variants v3
5d400e5a92be drm: remove prime sg_table caching
6172482908fe dma-buf: add caching of sg_table

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: ✗ Fi.CI.BAT: failure for series starting with [1/4] dma-buf: add caching of sg_table
  2018-07-18 12:07 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-07-18 13:24   ` Christian König
  2018-08-07 13:21     ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2018-07-18 13:24 UTC (permalink / raw)
  To: intel-gfx, Daniel Vetter

Hi Daniel,

Am 18.07.2018 um 14:07 schrieb Patchwork:
> == Series Details ==
>
> Series: series starting with [1/4] dma-buf: add caching of sg_table
> URL   : https://patchwork.freedesktop.org/series/46778/
> State : failure
> [SNIP]

it looks like I'm a step further understanding the problems which come 
with this change.

I've more or less audited all use cases and think that only i915 is left 
with the following lock inversion: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/fi-cfl-8700k/igt@gem_mmap_gtt@basic-small-bo-tiledx.html

Now my question is what is &obj->mm.lock used for and why do you guys 
call dma_buf_map_attachment() while holding it?

Thanks in advance,
Christian.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: ✗ Fi.CI.BAT: failure for series starting with [1/4] dma-buf: add caching of sg_table
  2018-07-18 13:24   ` Christian König
@ 2018-08-07 13:21     ` Daniel Vetter
  2018-08-07 15:13       ` Tvrtko Ursulin
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2018-08-07 13:21 UTC (permalink / raw)
  To: christian.koenig; +Cc: Daniel Vetter, intel-gfx

On Wed, Jul 18, 2018 at 03:24:26PM +0200, Christian König wrote:
> Hi Daniel,
> 
> Am 18.07.2018 um 14:07 schrieb Patchwork:
> > == Series Details ==
> > 
> > Series: series starting with [1/4] dma-buf: add caching of sg_table
> > URL   : https://patchwork.freedesktop.org/series/46778/
> > State : failure
> > [SNIP]
> 
> it looks like I'm a step further understanding the problems which come with
> this change.
> 
> I've more or less audited all use cases and think that only i915 is left
> with the following lock inversion: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/fi-cfl-8700k/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
> 
> Now my question is what is &obj->mm.lock used for and why do you guys call
> dma_buf_map_attachment() while holding it?

obj->mm.lock is the lock to protect getting at the backing storage.
i915_gem_object_get_pages and _put_pages are the relevant functions.

Existing paths want to pin the backing storage while holding the
reservation lock. And your new path needs to do the inverse since
dma_buf_map_attachment now also requires the reservation lock. And that is
obviously called from within the dma-buf importer version of get_pages.

I think there's 2 solutions:

- Merge obj->mm.lock and the reservation lock. Probably the cleaner
  solution, but likely more work.

- Make sure the obj->mm.lock always nests within the reservation lock, and
  grab the reservation lock anywhere it's not yet grabbed. Then you can
  use the dma_buf_map_attachment_locked variant in
  i915_gem_object_get_pages_dmabuf to avoid the locking inversion. This
  would essentially make the obj->mm.lock fully redundant.

Either way is going to be quite a bit of work. I expect that you need to
replace all the cases of dma_buf_map_attachment in i915 with
dma_buf_map_attachment_locked, and adjust the entire callchain to the new
locking scheme.

The real trouble here imo is that i915 CI is just the canary, I expect a
bunch of other drivers will also look at an inverted locking hierarchy if
dma_buf_map_attachment needs the reservation lock. And there's no
convenient CI for them, and code audit won't cut it really (at least I'm
too stupid to keep the locking hierarchy of an entire driver in my head).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: ✗ Fi.CI.BAT: failure for series starting with [1/4] dma-buf: add caching of sg_table
  2018-08-07 13:21     ` Daniel Vetter
@ 2018-08-07 15:13       ` Tvrtko Ursulin
  2018-08-14  9:42         ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2018-08-07 15:13 UTC (permalink / raw)
  To: Daniel Vetter, christian.koenig; +Cc: Daniel Vetter, intel-gfx


On 07/08/2018 14:21, Daniel Vetter wrote:
> On Wed, Jul 18, 2018 at 03:24:26PM +0200, Christian König wrote:
>> Hi Daniel,
>>
>> Am 18.07.2018 um 14:07 schrieb Patchwork:
>>> == Series Details ==
>>>
>>> Series: series starting with [1/4] dma-buf: add caching of sg_table
>>> URL   : https://patchwork.freedesktop.org/series/46778/
>>> State : failure
>>> [SNIP]
>>
>> it looks like I'm a step further understanding the problems which come with
>> this change.
>>
>> I've more or less audited all use cases and think that only i915 is left
>> with the following lock inversion: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/fi-cfl-8700k/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
>>
>> Now my question is what is &obj->mm.lock used for and why do you guys call
>> dma_buf_map_attachment() while holding it?
> 
> obj->mm.lock is the lock to protect getting at the backing storage.
> i915_gem_object_get_pages and _put_pages are the relevant functions.
> 
> Existing paths want to pin the backing storage while holding the
> reservation lock. And your new path needs to do the inverse since
> dma_buf_map_attachment now also requires the reservation lock. And that is
> obviously called from within the dma-buf importer version of get_pages.
> 
> I think there's 2 solutions:
> 
> - Merge obj->mm.lock and the reservation lock. Probably the cleaner
>    solution, but likely more work.
> 
> - Make sure the obj->mm.lock always nests within the reservation lock, and
>    grab the reservation lock anywhere it's not yet grabbed. Then you can
>    use the dma_buf_map_attachment_locked variant in
>    i915_gem_object_get_pages_dmabuf to avoid the locking inversion. This
>    would essentially make the obj->mm.lock fully redundant.
> 
> Either way is going to be quite a bit of work. I expect that you need to
> replace all the cases of dma_buf_map_attachment in i915 with
> dma_buf_map_attachment_locked, and adjust the entire callchain to the new
> locking scheme.
> 
> The real trouble here imo is that i915 CI is just the canary, I expect a
> bunch of other drivers will also look at an inverted locking hierarchy if
> dma_buf_map_attachment needs the reservation lock. And there's no
> convenient CI for them, and code audit won't cut it really (at least I'm
> too stupid to keep the locking hierarchy of an entire driver in my head).

We chatted about this on #intel-gfx and concluded that either solution 
derives to replacing the obj->mm.lock with the reservation lock. And 
that is problematic for i915, both from the reason of a general 
direction towards more fine-grained locking, and also issue that 
reservation lock needs to be avoided under the shrinker path (we lock 
obj->mm.lock when dropping the backing store there).

I proposed that maybe we could re-jig how we use obj->mm.lock a bit, to 
ensure backing store vfunc (get_pages) is not called under it (although 
I haven't thought it fully through it may be possible without too 
significant drawbacks), but Chris also has some patches which may work 
around this in a different way. So I'll wait to see those first.

On whether or not reservation lock is the right lock to use from dma-buf 
for this purpose I'll leave other guys to comment - I am not fully into 
the details of dma-buf design.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: ✗ Fi.CI.BAT: failure for series starting with [1/4] dma-buf: add caching of sg_table
  2018-08-07 15:13       ` Tvrtko Ursulin
@ 2018-08-14  9:42         ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-08-14  9:42 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Daniel Vetter, intel-gfx, christian.koenig

On Tue, Aug 07, 2018 at 04:13:53PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/08/2018 14:21, Daniel Vetter wrote:
> > On Wed, Jul 18, 2018 at 03:24:26PM +0200, Christian König wrote:
> > > Hi Daniel,
> > > 
> > > Am 18.07.2018 um 14:07 schrieb Patchwork:
> > > > == Series Details ==
> > > > 
> > > > Series: series starting with [1/4] dma-buf: add caching of sg_table
> > > > URL   : https://patchwork.freedesktop.org/series/46778/
> > > > State : failure
> > > > [SNIP]
> > > 
> > > it looks like I'm a step further understanding the problems which come with
> > > this change.
> > > 
> > > I've more or less audited all use cases and think that only i915 is left
> > > with the following lock inversion: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/fi-cfl-8700k/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
> > > 
> > > Now my question is what is &obj->mm.lock used for and why do you guys call
> > > dma_buf_map_attachment() while holding it?
> > 
> > obj->mm.lock is the lock to protect getting at the backing storage.
> > i915_gem_object_get_pages and _put_pages are the relevant functions.
> > 
> > Existing paths want to pin the backing storage while holding the
> > reservation lock. And your new path needs to do the inverse since
> > dma_buf_map_attachment now also requires the reservation lock. And that is
> > obviously called from within the dma-buf importer version of get_pages.
> > 
> > I think there's 2 solutions:
> > 
> > - Merge obj->mm.lock and the reservation lock. Probably the cleaner
> >    solution, but likely more work.
> > 
> > - Make sure the obj->mm.lock always nests within the reservation lock, and
> >    grab the reservation lock anywhere it's not yet grabbed. Then you can
> >    use the dma_buf_map_attachment_locked variant in
> >    i915_gem_object_get_pages_dmabuf to avoid the locking inversion. This
> >    would essentially make the obj->mm.lock fully redundant.
> > 
> > Either way is going to be quite a bit of work. I expect that you need to
> > replace all the cases of dma_buf_map_attachment in i915 with
> > dma_buf_map_attachment_locked, and adjust the entire callchain to the new
> > locking scheme.
> > 
> > The real trouble here imo is that i915 CI is just the canary, I expect a
> > bunch of other drivers will also look at an inverted locking hierarchy if
> > dma_buf_map_attachment needs the reservation lock. And there's no
> > convenient CI for them, and code audit won't cut it really (at least I'm
> > too stupid to keep the locking hierarchy of an entire driver in my head).
> 
> We chatted about this on #intel-gfx and concluded that either solution
> derives to replacing the obj->mm.lock with the reservation lock. And that is
> problematic for i915, both from the reason of a general direction towards
> more fine-grained locking, and also issue that reservation lock needs to be
> avoided under the shrinker path (we lock obj->mm.lock when dropping the
> backing store there).

So the way this works for other drivers (well atm there's only ttm ones)
is that they try-lock the reservation lock when trying to evict stuff from
the shrinker. Would be good to elaborate on why we can't use that
approach ..

> I proposed that maybe we could re-jig how we use obj->mm.lock a bit, to
> ensure backing store vfunc (get_pages) is not called under it (although I
> haven't thought it fully through it may be possible without too significant
> drawbacks), but Chris also has some patches which may work around this in a
> different way. So I'll wait to see those first.

.. or is that Chris' work?

Honest question since I'm entirely out of the loop here on the i915 side.

> On whether or not reservation lock is the right lock to use from dma-buf for
> this purpose I'll leave other guys to comment - I am not fully into the
> details of dma-buf design.

If we want to do dynamic cross-device buffer management, which is
Christian's goal here, then we must somehow harmonize the locking schemes
used for buffer management, at least for backing storage handling. That's
the entire challenge of this undertaking, and the big trouble is that we
don't have a surplus on people who understand more than their own drivers
buffer management code and locking scheme. That's the information I'm
trying to help extract from all involved parties here.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-08-14  9:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-18 10:47 [PATCH 1/4] dma-buf: add caching of sg_table Christian König
2018-07-18 10:47 ` [PATCH 2/4] drm: remove prime sg_table caching Christian König
2018-07-18 10:47 ` [PATCH 3/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v3 Christian König
2018-07-18 10:47 ` [PATCH 4/4] dma-buf: lock the reservation object during (un)map_dma_buf v3 Christian König
2018-07-18 11:46 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] dma-buf: add caching of sg_table Patchwork
2018-07-18 12:07 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-07-18 13:24   ` Christian König
2018-08-07 13:21     ` Daniel Vetter
2018-08-07 15:13       ` Tvrtko Ursulin
2018-08-14  9:42         ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.