Re: [REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support
Hi Hans,
On Thursday 27 November 2014 10:02:14 Hans Verkuil wrote:
On 11/26/2014 10:00 PM, Laurent Pinchart wrote:
On Tuesday 18 November 2014 13:51:03 Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add support for importing dmabuf to videobuf2-dma-sg.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pawel Osciak pa...@osciak.com
---
drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 ---
1 file changed, 136 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c index f671fab..ad6d5c7
100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
[snip]
@@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
static void vb2_dma_sg_prepare(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
Is this actually true ? If you look at the export code in patch 08/12, I
don't see where the exporter would sync the buffer for the importer
device.
I think this was true at some point in the past. It ties in with my comment
for patch 06/12: cpu/device syncing for dma-buf is (and was) broken,
although nobody has noticed since none of the DMABUF-aware drivers that are
used as such today need CPU access to the buffer, or are only used on Intel
architectures where this is all moot. Patches 12-16 of my RFCv6 series
really fix this. This particular comment was copied from the dma-contig
version. The basic idea was that when the driver needs CPU access it will
call the vaddr memop, which will map the buffer for CPU access.
However, I am not sure whether dmabuf actually did the right thing there
originally. Later dmabuf was extended with begin/end_for_cpu_access ops to
make this explicit, but that was never implemented in vb2. That's what the
second part of RFCv6 does.
Right now dma-sg is bug-compatible with dma-contig.
I spend 1-2 hours with Pawel in Düsseldorf figuring this out, it is not
exactly trivial to understand.
I agree that the situation is a mess, but we'll need to fix it one way or
another :-/ The more we wait the more painful it will be. Please note that the
problem isn't specific to CPU access, we need to sync for the device even in
direct device to device transfers (although in practice that shouldn't be
strictly required with the platforms we target today).
dma_sync_sg_for_device(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support
On 12/01/2014 11:46 PM, Laurent Pinchart wrote:
Hi Hans,
On Thursday 27 November 2014 10:02:14 Hans Verkuil wrote:
On 11/26/2014 10:00 PM, Laurent Pinchart wrote:
On Tuesday 18 November 2014 13:51:03 Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add support for importing dmabuf to videobuf2-dma-sg.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pawel Osciak pa...@osciak.com
---
drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 ---
1 file changed, 136 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c index f671fab..ad6d5c7
100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
[snip]
@@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
static void vb2_dma_sg_prepare(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
Is this actually true ? If you look at the export code in patch 08/12, I
don't see where the exporter would sync the buffer for the importer
device.
I think this was true at some point in the past. It ties in with my comment
for patch 06/12: cpu/device syncing for dma-buf is (and was) broken,
although nobody has noticed since none of the DMABUF-aware drivers that are
used as such today need CPU access to the buffer, or are only used on Intel
architectures where this is all moot. Patches 12-16 of my RFCv6 series
really fix this. This particular comment was copied from the dma-contig
version. The basic idea was that when the driver needs CPU access it will
call the vaddr memop, which will map the buffer for CPU access.
However, I am not sure whether dmabuf actually did the right thing there
originally. Later dmabuf was extended with begin/end_for_cpu_access ops to
make this explicit, but that was never implemented in vb2. That's what the
second part of RFCv6 does.
Right now dma-sg is bug-compatible with dma-contig.
I spend 1-2 hours with Pawel in Düsseldorf figuring this out, it is not
exactly trivial to understand.
I agree that the situation is a mess, but we'll need to fix it one way or
another :-/ The more we wait the more painful it will be. Please note that
the
problem isn't specific to CPU access, we need to sync for the device even in
direct device to device transfers (although in practice that shouldn't be
strictly required with the platforms we target today).
As I mentioned, I do have fixes for this. But I still need to implement that
in two more drivers where it wasn't obvious how it should be done. I hope to
be able to tackle that soon.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support
Hi Laurent,
On 11/26/2014 10:00 PM, Laurent Pinchart wrote:
Hi Hans,
Thank you for the patch.
On Tuesday 18 November 2014 13:51:03 Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add support for importing dmabuf to videobuf2-dma-sg.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pawel Osciak pa...@osciak.com
---
drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 +++---
1 file changed, 136 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c index f671fab..ad6d5c7 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -41,11 +41,19 @@ struct vb2_dma_sg_buf {
int offset;
enum dma_data_direction dma_dir;
struct sg_table sg_table;
+/*
+ * This will point to sg_table when used with the MMAP or USERPTR
+ * memory model, and to the dma_buf sglist when used with the
+ * DMABUF memory model.
+ */
+struct sg_table *dma_sgt;
size_t size;
unsigned intnum_pages;
atomic_trefcount;
struct vb2_vmarea_handler handler;
struct vm_area_struct *vma;
+
+struct dma_buf_attachment *db_attach;
};
static void vb2_dma_sg_put(void *buf_priv);
@@ -112,6 +120,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size, buf-size = size;
/* size is already page aligned */
buf-num_pages = size PAGE_SHIFT;
+buf-dma_sgt = buf-sg_table;
buf-pages = kzalloc(buf-num_pages * sizeof(struct page *),
GFP_KERNEL);
@@ -122,7 +131,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size, if (ret)
goto fail_pages_alloc;
-ret = sg_alloc_table_from_pages(buf-sg_table, buf-pages,
+ret = sg_alloc_table_from_pages(buf-dma_sgt, buf-pages,
buf-num_pages, 0, size, GFP_KERNEL);
if (ret)
goto fail_table_alloc;
@@ -171,7 +180,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_unmap_sg(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
if (buf-vaddr)
vm_unmap_ram(buf-vaddr, buf-num_pages);
-sg_free_table(buf-sg_table);
+sg_free_table(buf-dma_sgt);
while (--i = 0)
__free_page(buf-pages[i]);
kfree(buf-pages);
@@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
static void vb2_dma_sg_prepare(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
-struct sg_table *sgt = buf-sg_table;
+struct sg_table *sgt = buf-dma_sgt;
+
+/* DMABUF exporter will flush the cache for us */
+if (buf-db_attach)
+return;
Is this actually true ? If you look at the export code in patch 08/12, I
don't
see where the exporter would sync the buffer for the importer device.
I think this was true at some point in the past. It ties in with my comment for
patch 06/12: cpu/device syncing for dma-buf is (and was) broken, although nobody
has noticed since none of the DMABUF-aware drivers that are used as such today
need CPU access to the buffer, or are only used on Intel architectures where
this is all moot. Patches 12-16 of my RFCv6 series really fix this. This
particular
comment was copied from the dma-contig version. The basic idea was that when the
driver needs CPU access it will call the vaddr memop, which will map the buffer
for CPU access.
However, I am not sure whether dmabuf actually did the right thing there
originally.
Later dmabuf was extended with begin/end_for_cpu_access ops to make this
explicit,
but that was never implemented in vb2. That's what the second part of RFCv6
does.
Right now dma-sg is bug-compatible with dma-contig.
I spend 1-2 hours with Pawel in Düsseldorf figuring this out, it is not exactly
trivial to understand.
Regards,
Hans
dma_sync_sg_for_device(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -191,7 +204,11 @@ static void vb2_dma_sg_prepare(void *buf_priv)
static void vb2_dma_sg_finish(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
-struct sg_table *sgt = buf-sg_table;
+struct sg_table *sgt = buf-dma_sgt;
+
+/* DMABUF exporter will flush the cache for us */
+if (buf-db_attach)
+return;
dma_sync_sg_for_cpu(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -219,6 +236,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx,
unsigned long vaddr, buf-dma_dir = dma_dir;
buf-offset = vaddr ~PAGE_MASK;
buf-size = size;
+buf-dma_sgt = buf-sg_table;
first = (vaddrPAGE_MASK) PAGE_SHIFT;
last = ((vaddr + size - 1) PAGE_MASK) PAGE_SHIFT;
@@ -271,7 +289,7 @@ static
Re: [REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support
Hi Hans,
Thank you for the patch.
On Tuesday 18 November 2014 13:51:03 Hans Verkuil wrote:
From: Hans Verkuil hans.verk...@cisco.com
Add support for importing dmabuf to videobuf2-dma-sg.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pawel Osciak pa...@osciak.com
---
drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 +++---
1 file changed, 136 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c index f671fab..ad6d5c7 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -41,11 +41,19 @@ struct vb2_dma_sg_buf {
int offset;
enum dma_data_direction dma_dir;
struct sg_table sg_table;
+ /*
+ * This will point to sg_table when used with the MMAP or USERPTR
+ * memory model, and to the dma_buf sglist when used with the
+ * DMABUF memory model.
+ */
+ struct sg_table *dma_sgt;
size_t size;
unsigned intnum_pages;
atomic_trefcount;
struct vb2_vmarea_handler handler;
struct vm_area_struct *vma;
+
+ struct dma_buf_attachment *db_attach;
};
static void vb2_dma_sg_put(void *buf_priv);
@@ -112,6 +120,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size, buf-size = size;
/* size is already page aligned */
buf-num_pages = size PAGE_SHIFT;
+ buf-dma_sgt = buf-sg_table;
buf-pages = kzalloc(buf-num_pages * sizeof(struct page *),
GFP_KERNEL);
@@ -122,7 +131,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size, if (ret)
goto fail_pages_alloc;
- ret = sg_alloc_table_from_pages(buf-sg_table, buf-pages,
+ ret = sg_alloc_table_from_pages(buf-dma_sgt, buf-pages,
buf-num_pages, 0, size, GFP_KERNEL);
if (ret)
goto fail_table_alloc;
@@ -171,7 +180,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_unmap_sg(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
if (buf-vaddr)
vm_unmap_ram(buf-vaddr, buf-num_pages);
- sg_free_table(buf-sg_table);
+ sg_free_table(buf-dma_sgt);
while (--i = 0)
__free_page(buf-pages[i]);
kfree(buf-pages);
@@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
static void vb2_dma_sg_prepare(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
Is this actually true ? If you look at the export code in patch 08/12, I don't
see where the exporter would sync the buffer for the importer device.
dma_sync_sg_for_device(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -191,7 +204,11 @@ static void vb2_dma_sg_prepare(void *buf_priv)
static void vb2_dma_sg_finish(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
dma_sync_sg_for_cpu(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -219,6 +236,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx,
unsigned long vaddr, buf-dma_dir = dma_dir;
buf-offset = vaddr ~PAGE_MASK;
buf-size = size;
+ buf-dma_sgt = buf-sg_table;
first = (vaddrPAGE_MASK) PAGE_SHIFT;
last = ((vaddr + size - 1) PAGE_MASK) PAGE_SHIFT;
@@ -271,7 +289,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx,
unsigned long vaddr, if (num_pages_from_user != buf-num_pages)
goto userptr_fail_get_user_pages;
- if (sg_alloc_table_from_pages(buf-sg_table, buf-pages,
+ if (sg_alloc_table_from_pages(buf-dma_sgt, buf-pages,
buf-num_pages, buf-offset, size, 0))
goto userptr_fail_alloc_table_from_pages;
@@ -313,7 +331,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
dma_unmap_sg(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
if (buf-vaddr)
vm_unmap_ram(buf-vaddr, buf-num_pages);
- sg_free_table(buf-sg_table);
+ sg_free_table(buf-dma_sgt);
while (--i = 0) {
if (buf-dma_dir == DMA_FROM_DEVICE)
set_page_dirty_lock(buf-pages[i]);
@@ -331,14 +349,16 @@ static void *vb2_dma_sg_vaddr(void *buf_priv)
BUG_ON(!buf);
- if (!buf-vaddr)
- buf-vaddr = vm_map_ram(buf-pages,
-
[REVIEWv7 PATCH 07/12] vb2-dma-sg: add dmabuf import support
From: Hans Verkuil hans.verk...@cisco.com
Add support for importing dmabuf to videobuf2-dma-sg.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pawel Osciak pa...@osciak.com
---
drivers/media/v4l2-core/videobuf2-dma-sg.c | 149 ++---
1 file changed, 136 insertions(+), 13 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index f671fab..ad6d5c7 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -41,11 +41,19 @@ struct vb2_dma_sg_buf {
int offset;
enum dma_data_direction dma_dir;
struct sg_table sg_table;
+ /*
+* This will point to sg_table when used with the MMAP or USERPTR
+* memory model, and to the dma_buf sglist when used with the
+* DMABUF memory model.
+*/
+ struct sg_table *dma_sgt;
size_t size;
unsigned intnum_pages;
atomic_trefcount;
struct vb2_vmarea_handler handler;
struct vm_area_struct *vma;
+
+ struct dma_buf_attachment *db_attach;
};
static void vb2_dma_sg_put(void *buf_priv);
@@ -112,6 +120,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size,
buf-size = size;
/* size is already page aligned */
buf-num_pages = size PAGE_SHIFT;
+ buf-dma_sgt = buf-sg_table;
buf-pages = kzalloc(buf-num_pages * sizeof(struct page *),
GFP_KERNEL);
@@ -122,7 +131,7 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned
long size,
if (ret)
goto fail_pages_alloc;
- ret = sg_alloc_table_from_pages(buf-sg_table, buf-pages,
+ ret = sg_alloc_table_from_pages(buf-dma_sgt, buf-pages,
buf-num_pages, 0, size, GFP_KERNEL);
if (ret)
goto fail_table_alloc;
@@ -171,7 +180,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_unmap_sg(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
if (buf-vaddr)
vm_unmap_ram(buf-vaddr, buf-num_pages);
- sg_free_table(buf-sg_table);
+ sg_free_table(buf-dma_sgt);
while (--i = 0)
__free_page(buf-pages[i]);
kfree(buf-pages);
@@ -183,7 +192,11 @@ static void vb2_dma_sg_put(void *buf_priv)
static void vb2_dma_sg_prepare(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
dma_sync_sg_for_device(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -191,7 +204,11 @@ static void vb2_dma_sg_prepare(void *buf_priv)
static void vb2_dma_sg_finish(void *buf_priv)
{
struct vb2_dma_sg_buf *buf = buf_priv;
- struct sg_table *sgt = buf-sg_table;
+ struct sg_table *sgt = buf-dma_sgt;
+
+ /* DMABUF exporter will flush the cache for us */
+ if (buf-db_attach)
+ return;
dma_sync_sg_for_cpu(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
}
@@ -219,6 +236,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx,
unsigned long vaddr,
buf-dma_dir = dma_dir;
buf-offset = vaddr ~PAGE_MASK;
buf-size = size;
+ buf-dma_sgt = buf-sg_table;
first = (vaddrPAGE_MASK) PAGE_SHIFT;
last = ((vaddr + size - 1) PAGE_MASK) PAGE_SHIFT;
@@ -271,7 +289,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx,
unsigned long vaddr,
if (num_pages_from_user != buf-num_pages)
goto userptr_fail_get_user_pages;
- if (sg_alloc_table_from_pages(buf-sg_table, buf-pages,
+ if (sg_alloc_table_from_pages(buf-dma_sgt, buf-pages,
buf-num_pages, buf-offset, size, 0))
goto userptr_fail_alloc_table_from_pages;
@@ -313,7 +331,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
dma_unmap_sg(buf-dev, sgt-sgl, sgt-nents, buf-dma_dir);
if (buf-vaddr)
vm_unmap_ram(buf-vaddr, buf-num_pages);
- sg_free_table(buf-sg_table);
+ sg_free_table(buf-dma_sgt);
while (--i = 0) {
if (buf-dma_dir == DMA_FROM_DEVICE)
set_page_dirty_lock(buf-pages[i]);
@@ -331,14 +349,16 @@ static void *vb2_dma_sg_vaddr(void *buf_priv)
BUG_ON(!buf);
- if (!buf-vaddr)
- buf-vaddr = vm_map_ram(buf-pages,
- buf-num_pages,
- -1,
- PAGE_KERNEL);
+ if (!buf-vaddr) {
+ if (buf-db_attach)
+
