Hi all,
Adding piles more people.
For the first case of caching the iommu mapping there's different answers, depening upon the exact case:
1) You need to fix your userspace to not constantly re-establish the sharing.
2) We need to add streaming dma support for real to dma-bufs so that the mapping can be kept while we transfer ownership around. Thus far no one really needed this though since usually you don't actually do cpu access.
3) You need opportunistic caching of imported/exported buffer objects and their mappings. For this you need a) subsystem import/export support which guarantees you to hand out the same dma-buf/native object again upon re-export or re-import (drm has it) b) some opportunistic caching of buffer objects (pretty much are real gpu drm drivers have it). No need of any metadata scheme, and given how much fun I've had implemented this for drm I don't you can make your metadata scheme work in a sane or correct way wrt lifetimes.
For caching the iommu mapping if the iommu is the same for multiple devices:
1) We need some way to figure out which iommu serves which devices.
2) The exporter needs to consult this and might just hand out the same sg mapping out again if it wants to.
No need for importers to do fancy stuff, or attach any importer-visible metadata to dma-bufs. Of course duplicating this code all over the place is a but uncool, so I expect that eventually we'll have a generic exporter implementation, at least for non-swappable buffers. drm/gem is a bit special here ...
In general I don't like the idea of arbitrary metadata at all, sounds prone to abuse with crazy lifetime/refcounting rules for the objects involved. Also I think for a lot of your examples (like debugging) it would be much better if we have a standardized piece of metadata so that all drivers/platforms can use the same tooling.
And it feels like I'm writing such a mail every few months ...
Cheers, Daniel
On Mon, Mar 24, 2014 at 7:20 AM, Bin Wang binw@marvell.com wrote:
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:
- Try to reuse the first generated specific data in the buffer's life time, to avoid redundant map/unmap, or generate/free.
- Try to share the "reusable" data between devices, if the devices support compatible formats.
Regards, Bin Wang _______________________________________________ Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
Hi all,
On 2014년 03월 24일 16:35, Daniel Vetter wrote:
Hi all,
Adding piles more people.
For the first case of caching the iommu mapping there's different answers, depening upon the exact case:
You need to fix your userspace to not constantly re-establish the sharing.
We need to add streaming dma support for real to dma-bufs so that
the mapping can be kept while we transfer ownership around. Thus far no one really needed this though since usually you don't actually do cpu access.
- You need opportunistic caching of imported/exported buffer objects
and their mappings. For this you need a) subsystem import/export support which guarantees you to hand out the same dma-buf/native object again upon re-export or re-import (drm has it) b) some opportunistic caching of buffer objects (pretty much are real gpu drm drivers have it). No need of any metadata scheme, and given how much fun I've had implemented this for drm I don't you can make your metadata scheme work in a sane or correct way wrt lifetimes.
For caching the iommu mapping if the iommu is the same for multiple devices:
We need some way to figure out which iommu serves which devices.
The exporter needs to consult this and might just hand out the same
sg mapping out again if it wants to.
No need for importers to do fancy stuff, or attach any importer-visible metadata to dma-bufs. Of course duplicating this code all over the place is a but uncool, so I expect that eventually we'll have a generic exporter implementation, at least for non-swappable buffers. drm/gem is a bit special here ...
In general I don't like the idea of arbitrary metadata at all, sounds prone to abuse with crazy lifetime/refcounting rules for the objects involved. Also I think for a lot of your examples (like debugging) it would be much better if we have a standardized piece of metadata so that all drivers/platforms can use the same tooling.
And it feels like I'm writing such a mail every few months ...
I posted concept about importer priv of dma-buf, and it seems that this patch is partly from similar requirement - iommu map/unmap.
And at that time, Daniel agreed at least the issue, that unnecessary map/unmap can repeatedly called, is also in the drm gem. http://lists.freedesktop.org/archives/dri-devel/2013-June/039469.html
So I agree about the necessary of some data of dma-buf for general importer even though the data can be shared between different subsystems.
Cheers, Daniel
On Mon, Mar 24, 2014 at 7:20 AM, Bin Wang binw@marvell.com wrote:
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.
This is basic concept of my RFC patch. http://www.kernelhub.org/?p=2&msg=268056
Best Regards, - Seung-Woo Kim
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:
- Try to reuse the first generated specific data in the buffer's life time, to avoid redundant map/unmap, or generate/free.
- Try to share the "reusable" data between devices, if the devices support compatible formats.
Regards, Bin Wang _______________________________________________ Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig
On Tue, Mar 25, 2014 at 06:21:34PM +0900, Seung-Woo Kim wrote:
Hi all,
On 2014년 03월 24일 16:35, Daniel Vetter wrote:
Hi all,
Adding piles more people.
For the first case of caching the iommu mapping there's different answers, depening upon the exact case:
You need to fix your userspace to not constantly re-establish the sharing.
We need to add streaming dma support for real to dma-bufs so that
the mapping can be kept while we transfer ownership around. Thus far no one really needed this though since usually you don't actually do cpu access.
- You need opportunistic caching of imported/exported buffer objects
and their mappings. For this you need a) subsystem import/export support which guarantees you to hand out the same dma-buf/native object again upon re-export or re-import (drm has it) b) some opportunistic caching of buffer objects (pretty much are real gpu drm drivers have it). No need of any metadata scheme, and given how much fun I've had implemented this for drm I don't you can make your metadata scheme work in a sane or correct way wrt lifetimes.
For caching the iommu mapping if the iommu is the same for multiple devices:
We need some way to figure out which iommu serves which devices.
The exporter needs to consult this and might just hand out the same
sg mapping out again if it wants to.
No need for importers to do fancy stuff, or attach any importer-visible metadata to dma-bufs. Of course duplicating this code all over the place is a but uncool, so I expect that eventually we'll have a generic exporter implementation, at least for non-swappable buffers. drm/gem is a bit special here ...
In general I don't like the idea of arbitrary metadata at all, sounds prone to abuse with crazy lifetime/refcounting rules for the objects involved. Also I think for a lot of your examples (like debugging) it would be much better if we have a standardized piece of metadata so that all drivers/platforms can use the same tooling.
And it feels like I'm writing such a mail every few months ...
I posted concept about importer priv of dma-buf, and it seems that this patch is partly from similar requirement - iommu map/unmap.
And at that time, Daniel agreed at least the issue, that unnecessary map/unmap can repeatedly called, is also in the drm gem. http://lists.freedesktop.org/archives/dri-devel/2013-June/039469.html
So I agree about the necessary of some data of dma-buf for general importer even though the data can be shared between different subsystems.
Oh right, I guess someone has to implement this eventually ;-) -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Seung-Woo Kim