dma-buf: refcount the attachment for cache_sgt_mapping

Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") added a support of caching the sgt pointer into an attach pointer to let users reuse the sgt pointer without another mapping. However, it might not totally work as most of dma-buf callers are doing attach() and map_attachment() back-to-back, using drm_prime.c for example:

     drm_gem_prime_import_dev() {
         attach = dma_buf_attach() {
             /* Allocating a new attach */
             attach = kzalloc();
             /* .... */
             return attach;
         }
         dma_buf_map_attachment(attach, direction) {
             /* attach->sgt would be always empty as attach is new */
             if (attach->sgt) {
                 /* Reuse attach->sgt */
             }
             /* Otherwise, map it */
             attach->sgt = map();
         }
     }

So, for a cache_sgt_mapping use case, it would need to get the same attachment pointer in order to reuse its sgt pointer. So this patch adds a refcount to the attach() function and lets it search for the existing attach pointer by matching the dev pointer.

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++
  include/linux/dma-buf.h   |  2 ++
  2 files changed, 25 insertions(+)

drivers/dma-buf/dma-buf.c +23 -0

index f4104a21b069..d0260553a31c 100644
@@ -559,6 +559,21 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
  	if (WARN_ON(!dmabuf || !dev))
  		return ERR_PTR(-EINVAL);
 
+	/* cache_sgt_mapping requires to reuse the same attachment pointer */
+	if (dmabuf->ops->cache_sgt_mapping) {
+		mutex_lock(&dmabuf->lock);
+
+		/* Search for existing attachment and increase its refcount */
+		list_for_each_entry(attach, &dmabuf->attachments, node) {
+			if (dev != attach->dev)
+				continue;
+			atomic_inc_not_zero(&attach->refcount);
+			goto unlock_attach;
+		}
+
+		mutex_unlock(&dmabuf->lock);
+	}
+
  	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
  	if (!attach)
  		return ERR_PTR(-ENOMEM);

@@ -575,6 +590,9 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
  	}
  	list_add(&attach->node, &dmabuf->attachments);
 
+	atomic_set(&attach->refcount, 1);
+
+unlock_attach:
  	mutex_unlock(&dmabuf->lock);
 
  	return attach;

@@ -599,6 +617,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
  	if (WARN_ON(!dmabuf || !attach))
  		return;
 
+	/* Decrease the refcount for cache_sgt_mapping use cases */
+	if (dmabuf->ops->cache_sgt_mapping &&
+	    atomic_dec_return(&attach->refcount))
+		return;
+
  	if (attach->sgt)
  		dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir);
 

include/linux/dma-buf.h +2 -0

index 8a327566d7f4..65f12212ca2e 100644
@@ -333,6 +333,7 @@ struct dma_buf {
   * @dev: device attached to the buffer.
   * @node: list of dma_buf_attachment.
   * @sgt: cached mapping.
+ * @refcount: refcount of the attachment for the same device.
   * @dir: direction of cached mapping.
   * @priv: exporter specific attachment data.
   *

@@ -350,6 +351,7 @@ struct dma_buf_attachment {
  	struct device *dev;
  	struct list_head node;
  	struct sg_table *sgt;
+	atomic_t refcount;
  	enum dma_data_direction dir;
  	void *priv;
  };

Am 12.06.19 um 03:22 schrieb Nicolin Chen:

Quoted 22 lines

Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") added a support of caching the sgt pointer into an attach pointer to let users reuse the sgt pointer without another mapping. However, it might not totally work as most of dma-buf callers are doing attach() and map_attachment() back-to-back, using drm_prime.c for example:

     drm_gem_prime_import_dev() {
         attach = dma_buf_attach() {
             /* Allocating a new attach */
             attach = kzalloc();
             /* .... */
             return attach;
         }
         dma_buf_map_attachment(attach, direction) {
             /* attach->sgt would be always empty as attach is new */
             if (attach->sgt) {
                 /* Reuse attach->sgt */
             }
             /* Otherwise, map it */
             attach->sgt = map();
         }
     }

Quoted 4 lines

So, for a cache_sgt_mapping use case, it would need to get the same attachment pointer in order to reuse its sgt pointer. So this patch adds a refcount to the attach() function and lets it search for the existing attach pointer by matching the dev pointer.

I don't think that this is a good idea.

We use sgt caching as workaround for locking order problems and want to remove it again in the long term.

So what is the actual use case of this?

Hi Christian,

Thanks for the quick reply.

Quoted 23 lines · Christian

Am 12.06.19 um 03:22 schrieb Nicolin Chen:

Quoted 22 lines

Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") added a support of caching the sgt pointer into an attach pointer to let users reuse the sgt pointer without another mapping. However, it might not totally work as most of dma-buf callers are doing attach() and map_attachment() back-to-back, using drm_prime.c for example:

      drm_gem_prime_import_dev() {
          attach = dma_buf_attach() {
              /* Allocating a new attach */
              attach = kzalloc();
              /* .... */
              return attach;
          }
          dma_buf_map_attachment(attach, direction) {
              /* attach->sgt would be always empty as attach is new */
              if (attach->sgt) {
                  /* Reuse attach->sgt */
              }
              /* Otherwise, map it */
              attach->sgt = map();
          }
      }

Quoted 8 lines

Quoted 4 lines

So, for a cache_sgt_mapping use case, it would need to get the same attachment pointer in order to reuse its sgt pointer. So this patch adds a refcount to the attach() function and lets it search for the existing attach pointer by matching the dev pointer.

I don't think that this is a good idea.

We use sgt caching as workaround for locking order problems and want to remove it again in the long term.

Oh. I thought it was for a performance improving purpose. It may be a misunderstanding then.

Quoted 1 line

So what is the actual use case of this?

We have some similar downstream changes at dma_buf to reduce the overhead from multiple clients of the same device doing attach() and map_attachment() calls for the same dma_buf.

We haven't used DRM/GRM_PRIME yet but I am also curious would it benefit DRM also if we reduce this overhead in the dma_buf?

Thanks
Nicolin

Am 12.06.19 um 10:02 schrieb Nicolin Chen:

Quoted 36 lines

Hi Christian,

Thanks for the quick reply.

Quoted 23 lines · Christian

Am 12.06.19 um 03:22 schrieb Nicolin Chen:

Quoted 22 lines

Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2") added a support of caching the sgt pointer into an attach pointer to let users reuse the sgt pointer without another mapping. However, it might not totally work as most of dma-buf callers are doing attach() and map_attachment() back-to-back, using drm_prime.c for example:

      drm_gem_prime_import_dev() {
          attach = dma_buf_attach() {
              /* Allocating a new attach */
              attach = kzalloc();
              /* .... */
              return attach;
          }
          dma_buf_map_attachment(attach, direction) {
              /* attach->sgt would be always empty as attach is new */
              if (attach->sgt) {
                  /* Reuse attach->sgt */
              }
              /* Otherwise, map it */
              attach->sgt = map();
          }
      }

Quoted 8 lines

Quoted 4 lines

So, for a cache_sgt_mapping use case, it would need to get the same attachment pointer in order to reuse its sgt pointer. So this patch adds a refcount to the attach() function and lets it search for the existing attach pointer by matching the dev pointer.

I don't think that this is a good idea.

We use sgt caching as workaround for locking order problems and want to remove it again in the long term.

Quoted 7 lines

Oh. I thought it was for a performance improving purpose. It may be a misunderstanding then.

Quoted 1 line

So what is the actual use case of this?

We have some similar downstream changes at dma_buf to reduce the overhead from multiple clients of the same device doing attach() and map_attachment() calls for the same dma_buf.

I don't think that this is a good idea over all. A driver calling attach for the same buffer is doing something wrong in the first place and we should not work around this in the DMA-buf handling.

Quoted 2 lines

We haven't used DRM/GRM_PRIME yet but I am also curious would it benefit DRM also if we reduce this overhead in the dma_buf?

No, not at all.

From you replies, in a summary, does it means that there won't be a case

Hi Christian,

Quoted 5 lines · Christian

Am 12.06.19 um 10:02 schrieb Nicolin Chen:
[SNIP]

Quoted 2 lines

We haven't used DRM/GRM_PRIME yet but I am also curious would it benefit DRM also if we reduce this overhead in the dma_buf?

No, not at all.

From you replies, in a summary, does it means that there won't be a case
of DRM having a dma_buf attaching to the same device, i.e. multiple calls of drm_gem_prime_import() function with same parameters of dev + dma_buf?

If so, we can just ignore/drop this patch. Sorry for the misunderstanding.

Thanks
Nicolin

Am 12.06.19 um 10:15 schrieb Nicolin Chen:

Quoted 7 lines

Hi Christian,

Quoted 4 lines · Christian

Am 12.06.19 um 10:02 schrieb Nicolin Chen:
[SNIP]

Quoted 2 lines

We haven't used DRM/GRM_PRIME yet but I am also curious would it benefit DRM also if we reduce this overhead in the dma_buf?

Quoted 4 lines

Quoted 1 line

No, not at all.

From you replies, in a summary, does it means that there won't be a case
of DRM having a dma_buf attaching to the same device, i.e. multiple calls of drm_gem_prime_import() function with same parameters of dev + dma_buf?

Well, there are some cases where this happens. But in those cases we intentionally want to get a new attachment :)

So thinking more about it you would actually break those and that is not something we can do.

Quoted 1 line

If so, we can just ignore/drop this patch. Sorry for the misunderstanding.

It might be interesting for things like P2P, but even then it might be better to just cache the P2P settings instead of the full attachment.