Re: [Linaro-mm-sig] [PATCH] dma-buf: add meta data attachment

Hi Sumit,

On 03/21/2014 07:26 PM, Sumit Semwal wrote:

Hi Bin Wang,

On 21 March 2014 10:34, Bin Wang binw@marvell.com wrote:

...

we found there'd be varied specific data connected to a dmabuf, like the iommu mapping of buffer, the dma descriptors (that can be shared between several components). Though these info might be able to be generated every time before dma operations, for performance sake, it's better to be kept before really invalid.

Thanks for your patch! I think we'd need to have more specific details on what does 'meta data' mean here; more specific comments inline.

...

Change-Id: I89d43dc3fe1ee3da91c42074da5df71b968e6d3c Signed-off-by: Bin Wang binw@marvell.com

---

drivers/base/dma-buf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 22 ++++++++++ 2 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c index 08fe897..5c82e60 100644 --- a/drivers/base/dma-buf.c +++ b/drivers/base/dma-buf.c @@ -50,6 +50,7 @@ static int dma_buf_release(struct inode *inode, struct file *file)

     BUG_ON(dmabuf->vmapping_counter);

•   dma_buf_meta_release(dmabuf);
     dmabuf->ops->release(dmabuf);
  
     mutex_lock(&db_list.lock);
  

@@ -138,6 +139,7 @@ struct dma_buf *dma_buf_export_named(void *priv, const struct dma_buf_ops *ops,

     mutex_init(&dmabuf->lock);
     INIT_LIST_HEAD(&dmabuf->attachments);

•   INIT_LIST_HEAD(&dmabuf->metas);
  
  

I am not sure I understand the relationship of 'meta data' with 'dma-buf', or 'attachments' - do you mean to have a list of some meta-data that is unrelated to the attachments to the dma-buf? I think it'd help to explain whether meta-data is specific to each dma-buf, and there's a list of them, or this meta-data is related to each importer device attachment? If it's related to each importer, it should really be added as part of the dma_buf_attachment, and not separately.

The "meta-data" here can be any kind of data related to the dmabuf, , it's specific for each dmabuf.

For example, we have iommu for a VPU device, VPU driver has to map the dmabuf to get an IOVA address before it can access the buffer. This dmabuf would be reused many times during the video playback, for performance concern we don't want this dmabuf be map/unmaped by iommu driver every time of VPU HW data transfer, since the physical pages not changed and iommu IOVA plenty.

Another example, in our SoC, device A and device B use the same dma chain format, and these drivers share dmabuf between each other, thus the "dma chain" array doesn't need to be generated every timer for each device.

So here, with the help from the "meta-data", only the "first time" we need to generate the "meta-data" from scratch, and later we can just reuse it until the buffer freed.

The "meta-data" is related to the importer, or importers. However, the "dma_buf_attachment" is per "devices", holding device specific attributes, while "meta-data" is per "dmabuf', holding buffer specific attributes, like the example above.

...

     mutex_lock(&db_list.lock);
     list_add(&dmabuf->list_node, &db_list.head);

@@ -570,6 +572,104 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr) } EXPORT_SYMBOL_GPL(dma_buf_vunmap);

+/**

• • dma_buf_meta_attach - Attach additional meta data to the dmabuf
• • @dmabuf: [in] the dmabuf to attach to
• • @id: [in] the id of the meta data
• • @pdata: [in] the raw data to be attached
• • @release: [in] the callback to release the meta data
• */

+int dma_buf_meta_attach(struct dma_buf *dmabuf, int id, void *pdata,

•   int (*release)(void *))
  

+{

•   struct dma_buf_meta *pmeta;
  

•   pmeta = kmalloc(sizeof(struct dma_buf_meta), GFP_KERNEL);
  

•   if (pmeta == NULL)
  

•           return -ENOMEM;
  

•   pmeta->id = id;
  

What does 'id' mean here? Also there is no check on any duplicity on the 'id', so if any device adds more meta data with the same ID, you would always work only on the first one? It would also help if you could explain the relevance of id.

The "id" here means a kind of specific "meta-data", for example we use "#define VPU_DMABUF_META_ID 0x10000" for the VPU iommu related data.

Each importer would try to "dma_buf_meta_fetch()" the "meta-data" first, if not found, it would generate and attach it. If found, it would just reuse it.

I agree with you that it's better to add the check of id in this function as well.

...

•   pmeta->pdata = pdata;
  

•   pmeta->release = release;
  

•   mutex_lock(&dmabuf->lock);
  

•   list_add(&pmeta->node, &dmabuf->metas);
  

•   mutex_unlock(&dmabuf->lock);
  

•   return 0;
  

+} +EXPORT_SYMBOL_GPL(dma_buf_meta_attach);

+/**

• • dma_buf_meta_dettach - Dettach the meta data from dmabuf by id
• • @dmabuf: [in] the dmabuf including the meta data
• • @id: [in] the id of the meta data
• */

+int dma_buf_meta_dettach(struct dma_buf *dmabuf, int id) +{

•   struct dma_buf_meta *pmeta, *tmp;
  

•   int ret = -ENOENT;
  

•   mutex_lock(&dmabuf->lock);
  

•   list_for_each_entry_safe(pmeta, tmp, &dmabuf->metas, node) {
  

•           if (pmeta->id == id) {
  

•                   if (pmeta->release)
  

•                           pmeta->release(pmeta->pdata);
  

•                   list_del(&pmeta->node);
  

•                   kfree(pmeta);
  

•                   ret = 0;
  

•                   break;
  

•           }
  

•   }
  

•   mutex_unlock(&dmabuf->lock);
  

•   return ret;
  

+} +EXPORT_SYMBOL_GPL(dma_buf_meta_dettach);

+/**

• • dma_buf_meta_fetch - Get the meta data from dmabuf by id
• • @dmabuf: [in] the dmabuf including the meta data
• • @id: [in] the id of the meta data
• */

+void *dma_buf_meta_fetch(struct dma_buf *dmabuf, int id) +{

•   struct dma_buf_meta *pmeta;
  

•   void *pdata = NULL;
  

•   mutex_lock(&dmabuf->lock);
  

•   list_for_each_entry(pmeta, &dmabuf->metas, node) {
  

•           if (pmeta->id == id) {
  

•                   pdata = pmeta->pdata;
  

•                   break;
  

•           }
  

•   }
  

•   mutex_unlock(&dmabuf->lock);
  

•   return pdata;
  

+} +EXPORT_SYMBOL_GPL(dma_buf_meta_fetch);

+/**

• • dma_buf_meta_release - Release all the meta data attached to the dmabuf
• • @dmabuf: [in] the dmabuf including the meta data
• */

+void dma_buf_meta_release(struct dma_buf *dmabuf) +{

•   struct dma_buf_meta *pmeta, *tmp;
  

•   mutex_lock(&dmabuf->lock);
  

•   list_for_each_entry_safe(pmeta, tmp, &dmabuf->metas, node) {
  

•           if (pmeta->release)
  

•                   pmeta->release(pmeta->pdata);
  

•           list_del(&pmeta->node);
  

•           kfree(pmeta);
  

•   }
  

•   mutex_unlock(&dmabuf->lock);
  

•   return;
  

+}

• #ifdef CONFIG_DEBUG_FS static int dma_buf_describe(struct seq_file *s) {

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index dfac5ed..369d032 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -120,6 +120,7 @@ struct dma_buf { size_t size; struct file *file; struct list_head attachments;

•   struct list_head metas;
     const struct dma_buf_ops *ops;
     /* mutex to serialize list manipulation, attach/detach and vmap/unmap */
     struct mutex lock;
  

@@ -149,6 +150,20 @@ struct dma_buf_attachment { };

Like I said above, I don't think I completely understand the need of 'a list of metas' - a sample usage would be most helpful to realise the relevance here.

As the examples above, in the list of "metas", there can be, VPU iommu mapping info, camera/display dma chain info, or other debugging/tracking info etc.

...

/**

• • struct dma_buf_meta - holds varied meta data attached to the buffer
• • @id: the identification of the meta data
• • @dmabuf: buffer for this attachment.
• • @node: list of dma_buf_meta.
• • @pdata: specific meta data.
• */

+struct dma_buf_meta {

•   int id;
  

•   struct list_head node;
  

•   int (*release)(void *pdata);
  

•   void *pdata;
  

+};

+/**

• get_dma_buf - convenience wrapper for get_file.
• @dmabuf: [in] pointer to dma_buf

@@ -194,6 +209,13 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, unsigned long); void *dma_buf_vmap(struct dma_buf *); void dma_buf_vunmap(struct dma_buf *, void *vaddr);

+int dma_buf_meta_attach(struct dma_buf *dmabuf, int id, void *pdata,

•   int (*release)(void *));
  

+int dma_buf_meta_dettach(struct dma_buf *dmabuf, int id); +void *dma_buf_meta_fetch(struct dma_buf *dmabuf, int id); +void dma_buf_meta_release(struct dma_buf *dmabuf);

• int dma_buf_debugfs_create_file(const char *name, int (*write)(struct seq_file *)); #endif /* __DMA_BUF_H__ */

-- 1.7.0.4

---

Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

The purpose of this extension mainly is: 1. Try to reuse the first generated specific data in the buffer's life time, to avoid redundant map/unmap, or generate/free. 2. Try to share the "reusable" data between devices, if the devices support compatible formats.

Regards, Bin Wang