[PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Douglas Anderson]

This series of patches will speed up memory allocation in dma-mapping
quite a bit.

The first patch ("ARM: dma-mapping: Optimize allocation") is hopefully
not terribly controversial: it merely doesn't try as hard to allocate
big chunks once it gets the first failure.  Since it's unlikely that
further big chunks will help (they're not likely to be virtually aligned
anyway), this should give a big speedup with no real regression to speak
of.  Yes, things could be made better, but this seems like a sane start.

The second patch ("common: DMA-mapping: add DMA_ATTR_ALLOC_SINGLE_PAGES
attribute") models MADV_NOHUGEPAGE as I understand it.  Hopefully folks
are happy with following that lead.  It does nothing by itself.

The third patch ("ARM: dma-mapping: Use DMA_ATTR_ALLOC_SINGLE_PAGES hint to
optimize allocation") simply applies the 2nd patch.  Again it's pretty
simple.  ...and again it does nothing by itself.

Thue fourth patch ("[media] videobuf2-dc: Let drivers specify DMA
attrs") comes from the ChromeOS tree (authored by Tomasz Figa) and
allows the fifth patch.

The fifth patch ("[media] s5p-mfc: Set DMA_ATTR_ALLOC_SINGLE_PAGES") uses the
new attribute.  For a second user, you can see the out of tree patch for
rk3288 at <https://chromium-review.googlesource.com/#/c/320498/>.

All testing was done on the chromeos kernel-3.8 and kernel-3.14.
Sanity (compile / boot) testing was done on a v4.4-rc6-based kernel on
rk3288, though the video codec isn't there.  I don't have graphics / MFC
working well on exynos, so the MFC change was only compile-tested
upstream.  Hopefully someone upstream whose setup for MFC can give a
Tested-by for these?

Also note that v2 of this series had an extra patch
<https://patchwork.kernel.org/patch/7888861/> that would attempt to sort
the allocation results to opportunistically get some extra alignment.  I
dropped that, but it could be re-introduced if there was interest.  I
found that it did give a little extra alignment sometimes, but maybe not
enough to justify the extra complexity.  It also was a bit half-baked
since it really should have tried harder to ensure alignment.

Changes in v6:
- renamed DMA_ATTR_NO_HUGE_PAGE to DMA_ATTR_ALLOC_SINGLE_PAGES

Changes in v5:
- renamed DMA_ATTR_NOHUGEPAGE to DMA_ATTR_NO_HUGE_PAGE
- s/ping ping/ping pong/
- Let drivers specify DMA attrs new for v5
- s5p-mfc patch new for v5

Changes in v4:
- Added Marek's ack
- renamed DMA_ATTR_SEQUENTIAL to DMA_ATTR_NOHUGEPAGE

Changes in v3:
- add DMA_ATTR_SEQUENTIAL attribute new for v3
- Use DMA_ATTR_SEQUENTIAL hint patch new for v3.

Changes in v2:
- No longer just 1 page at a time, but gives up higher order quickly.
- Only tries important higher order allocations that might help us.

Douglas Anderson (4):
  ARM: dma-mapping: Optimize allocation
  common: DMA-mapping: add DMA_ATTR_ALLOC_SINGLE_PAGES attribute
  ARM: dma-mapping: Use DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize
    alloc
  s5p-mfc: Set DMA_ATTR_ALLOC_SINGLE_PAGES

Tomasz Figa (1):
  videobuf2-dc: Let drivers specify DMA attrs

 Documentation/DMA-attributes.txt               | 23 ++++++++++++++++
 arch/arm/mm/dma-mapping.c                      | 38 ++++++++++++++++----------
 drivers/media/platform/s5p-mfc/s5p_mfc.c       | 13 +++++++--
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 33 ++++++++++++++--------
 include/linux/dma-attrs.h                      |  1 +
 include/media/videobuf2-dma-contig.h           | 11 +++++++-
 6 files changed, 91 insertions(+), 28 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Douglas Anderson]

From: Tomasz Figa <tfiga@chromium.org>

DMA allocations might be subject to certain reqiurements specific to the
hardware using the buffers, such as availability of kernel mapping (for
contents fix-ups in the driver). The only entity that knows them is the
driver, so it must share this knowledge with vb2-dc.

This patch extends the alloc_ctx initialization interface to let the
driver specify DMA attrs, which are then stored inside the allocation
context and will be used for all allocations with that context.

As a side effect, all dma_*_coherent() calls are turned into
dma_*_attrs() calls, because the attributes need to be carried over
through all DMA operations.

Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v6: None
Changes in v5:
- Let drivers specify DMA attrs new for v5

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/media/v4l2-core/videobuf2-dma-contig.c | 33 +++++++++++++++++---------
 include/media/videobuf2-dma-contig.h           | 11 ++++++++-
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index c33127284cfe..5361197f3e57 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -23,13 +23,16 @@
 
 struct vb2_dc_conf {
 	struct device		*dev;
+	struct dma_attrs	attrs;
 };
 
 struct vb2_dc_buf {
 	struct device			*dev;
 	void				*vaddr;
 	unsigned long			size;
+	void				*cookie;
 	dma_addr_t			dma_addr;
+	struct dma_attrs		attrs;
 	enum dma_data_direction		dma_dir;
 	struct sg_table			*dma_sgt;
 	struct frame_vector		*vec;
@@ -131,7 +134,8 @@ static void vb2_dc_put(void *buf_priv)
 		sg_free_table(buf->sgt_base);
 		kfree(buf->sgt_base);
 	}
-	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
+	dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
+			&buf->attrs);
 	put_device(buf->dev);
 	kfree(buf);
 }
@@ -147,14 +151,18 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size,
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
-						GFP_KERNEL | gfp_flags);
-	if (!buf->vaddr) {
+	buf->attrs = conf->attrs;
+	buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
+					GFP_KERNEL | gfp_flags, &buf->attrs);
+	if (!buf->cookie) {
 		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
 		kfree(buf);
 		return ERR_PTR(-ENOMEM);
 	}
 
+	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->attrs))
+		buf->vaddr = buf->cookie;
+
 	/* Prevent the device from being released while the buffer is used */
 	buf->dev = get_device(dev);
 	buf->size = size;
@@ -185,8 +193,8 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 	 */
 	vma->vm_pgoff = 0;
 
-	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
-		buf->dma_addr, buf->size);
+	ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
+		buf->dma_addr, buf->size, &buf->attrs);
 
 	if (ret) {
 		pr_err("Remapping memory failed, error: %d\n", ret);
@@ -329,7 +337,7 @@ static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum)
 {
 	struct vb2_dc_buf *buf = dbuf->priv;
 
-	return buf->vaddr + pgnum * PAGE_SIZE;
+	return buf->vaddr ? buf->vaddr + pgnum * PAGE_SIZE : NULL;
 }
 
 static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
@@ -368,8 +376,8 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf)
 		return NULL;
 	}
 
-	ret = dma_get_sgtable(buf->dev, sgt, buf->vaddr, buf->dma_addr,
-		buf->size);
+	ret = dma_get_sgtable_attrs(buf->dev, sgt, buf->cookie, buf->dma_addr,
+		buf->size, &buf->attrs);
 	if (ret < 0) {
 		dev_err(buf->dev, "failed to get scatterlist from DMA API\n");
 		kfree(sgt);
@@ -721,7 +729,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
 };
 EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
 
-void *vb2_dma_contig_init_ctx(struct device *dev)
+void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
+				    struct dma_attrs *attrs)
 {
 	struct vb2_dc_conf *conf;
 
@@ -730,10 +739,12 @@ void *vb2_dma_contig_init_ctx(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	conf->dev = dev;
+	if (attrs)
+		conf->attrs = *attrs;
 
 	return conf;
 }
-EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);
+EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx_attrs);
 
 void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
 {
diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
index c33dfa69d7ab..2087c9a68be3 100644
--- a/include/media/videobuf2-dma-contig.h
+++ b/include/media/videobuf2-dma-contig.h
@@ -16,6 +16,8 @@
 #include <media/videobuf2-v4l2.h>
 #include <linux/dma-mapping.h>
 
+struct dma_attrs;
+
 static inline dma_addr_t
 vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
 {
@@ -24,7 +26,14 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
 	return *addr;
 }
 
-void *vb2_dma_contig_init_ctx(struct device *dev);
+void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
+				    struct dma_attrs *attrs);
+
+static inline void *vb2_dma_contig_init_ctx(struct device *dev)
+{
+	return vb2_dma_contig_init_ctx_attrs(dev, NULL);
+}
+
 void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
 
 extern const struct vb2_mem_ops vb2_dma_contig_memops;
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v6 5/5] s5p-mfc: Set DMA_ATTR_ALLOC_SINGLE_PAGES

[Douglas Anderson]

We do video allocation all the time and we need it to be fast.  Plus TLB
efficiency isn't terribly important for video.

That means we want to set DMA_ATTR_ALLOC_SINGLE_PAGES.

See also the previous change ("ARM: dma-mapping: Use
DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize allocation").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v6:
- renamed DMA_ATTR_NO_HUGE_PAGE to DMA_ATTR_ALLOC_SINGLE_PAGES

Changes in v5:
- s5p-mfc patch new for v5

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/media/platform/s5p-mfc/s5p_mfc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 927ab4928779..421d25a1aec1 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1095,6 +1095,7 @@ static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev)
 /* MFC probe function */
 static int s5p_mfc_probe(struct platform_device *pdev)
 {
+	DEFINE_DMA_ATTRS(attrs);
 	struct s5p_mfc_dev *dev;
 	struct video_device *vfd;
 	struct resource *res;
@@ -1164,12 +1165,20 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);
+	/*
+	 * We'll do mostly sequential access, so sacrifice TLB efficiency for
+	 * faster allocation.
+	 */
+	dma_set_attr(DMA_ATTR_ALLOC_SINGLE_PAGES, &attrs);
+
+	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx_attrs(dev->mem_dev_l,
+							  &attrs);
 	if (IS_ERR(dev->alloc_ctx[0])) {
 		ret = PTR_ERR(dev->alloc_ctx[0]);
 		goto err_res;
 	}
-	dev->alloc_ctx[1] = vb2_dma_contig_init_ctx(dev->mem_dev_r);
+	dev->alloc_ctx[1] = vb2_dma_contig_init_ctx_attrs(dev->mem_dev_r,
+							  &attrs);
 	if (IS_ERR(dev->alloc_ctx[1])) {
 		ret = PTR_ERR(dev->alloc_ctx[1]);
 		goto err_mem_init_ctx_1;
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Javier Martinez Canillas]

Hello Doug,

On Mon, Jan 11, 2016 at 2:30 PM, Douglas Anderson <dianders@chromium.org> wrote:

[snip]

>
> All testing was done on the chromeos kernel-3.8 and kernel-3.14.
> Sanity (compile / boot) testing was done on a v4.4-rc6-based kernel on
> rk3288, though the video codec isn't there.  I don't have graphics / MFC
> working well on exynos, so the MFC change was only compile-tested
> upstream.  Hopefully someone upstream whose setup for MFC can give a
> Tested-by for these?
>

I tested these patches on a Exynos5800 Peach Pi Chromebook. The
s5p-mfc driver probes correctly and the allocation succeeds.

I also tried to test actual video decoding using Gstreamer but ran
into issues (not related to this series) so testing that won't be
trivial for me.

It shouldn't block Doug's series though IMHO since he tested on his
platform and the patches speeds up allocation there, so is an
improvement.

Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Andrew Morton]

On Mon, 11 Jan 2016 09:30:22 -0800 Douglas Anderson <dianders@chromium.org> wrote:

> This series of patches will speed up memory allocation in dma-mapping
> quite a bit.

This is pretty much all ARM and driver stuff so I think I'll duck it. 
But I can merge it if nobody else feels a need to.

I saw a few acked-by/tested-by/etc from the v5 posting which weren't
carried over into v6 (might have been a timing race), so please fix
that up if there's an opportunity.

Regarding the new DMA_ATTR_ALLOC_SINGLE_PAGES hint: I suggest adding
"DMA_ATTR_ALLOC_SINGLE_PAGES is presently implemented only on ARM" to
the docs.  Or perhaps have a shot at implementing it elsewhere.

Typo in 4/5 changelog: "reqiurements"

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Doug Anderson]

Hi,

On Tue, Jan 26, 2016 at 3:59 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 11 Jan 2016 09:30:22 -0800 Douglas Anderson <dianders@chromium.org> wrote:
>
>> This series of patches will speed up memory allocation in dma-mapping
>> quite a bit.
>
> This is pretty much all ARM and driver stuff so I think I'll duck it.
> But I can merge it if nobody else feels a need to.

I was going to ask what the next steps were for this series.
Presumably I could post the patch to Russell's patch tracker if folks
want me to do that.  Alternatively it could go through the ARM-SOC
tree?


> I saw a few acked-by/tested-by/etc from the v5 posting which weren't
> carried over into v6 (might have been a timing race), so please fix
> that up if there's an opportunity.

Right.  Both Robin and Tomasz gave their Reviewed-by to Patch #1 in v5
even after v6 was posted.


> Regarding the new DMA_ATTR_ALLOC_SINGLE_PAGES hint: I suggest adding
> "DMA_ATTR_ALLOC_SINGLE_PAGES is presently implemented only on ARM" to
> the docs.  Or perhaps have a shot at implementing it elsewhere.

Warning sounds good.


> Typo in 4/5 changelog: "reqiurements"

Thanks for catching!


I'm happy to post up a v6 with these things fixed or I'm happy for
whoever is applying it to make these small fixes themselves.  Any
volunteers?  Olof, Arnd, or Russell: any of you want these patches?


-Doug

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Olof Johansson]

On Tue, Jan 26, 2016 at 4:39 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Jan 26, 2016 at 3:59 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon, 11 Jan 2016 09:30:22 -0800 Douglas Anderson <dianders@chromium.org> wrote:
>>
>>> This series of patches will speed up memory allocation in dma-mapping
>>> quite a bit.
>>
>> This is pretty much all ARM and driver stuff so I think I'll duck it.
>> But I can merge it if nobody else feels a need to.
>
> I was going to ask what the next steps were for this series.
> Presumably I could post the patch to Russell's patch tracker if folks
> want me to do that.  Alternatively it could go through the ARM-SOC
> tree?
>
>
>> I saw a few acked-by/tested-by/etc from the v5 posting which weren't
>> carried over into v6 (might have been a timing race), so please fix
>> that up if there's an opportunity.
>
> Right.  Both Robin and Tomasz gave their Reviewed-by to Patch #1 in v5
> even after v6 was posted.
>
>
>> Regarding the new DMA_ATTR_ALLOC_SINGLE_PAGES hint: I suggest adding
>> "DMA_ATTR_ALLOC_SINGLE_PAGES is presently implemented only on ARM" to
>> the docs.  Or perhaps have a shot at implementing it elsewhere.
>
> Warning sounds good.
>
>
>> Typo in 4/5 changelog: "reqiurements"
>
> Thanks for catching!
>
>
> I'm happy to post up a v6 with these things fixed or I'm happy for
> whoever is applying it to make these small fixes themselves.  Any
> volunteers?  Olof, Arnd, or Russell: any of you want these patches?

I think it makes sense to send these through Russell's tracker for him
to merge, especially since I don't think there are any dependencies on
them for SoC-specific patches coming up.


-Olof

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Doug Anderson]

Hi,

On Fri, Jan 29, 2016 at 1:52 PM, Olof Johansson <olof@lixom.net> wrote:
> On Tue, Jan 26, 2016 at 4:39 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Tue, Jan 26, 2016 at 3:59 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>> On Mon, 11 Jan 2016 09:30:22 -0800 Douglas Anderson <dianders@chromium.org> wrote:
>>>
>>>> This series of patches will speed up memory allocation in dma-mapping
>>>> quite a bit.
>>>
>>> This is pretty much all ARM and driver stuff so I think I'll duck it.
>>> But I can merge it if nobody else feels a need to.
>>
>> I was going to ask what the next steps were for this series.
>> Presumably I could post the patch to Russell's patch tracker if folks
>> want me to do that.  Alternatively it could go through the ARM-SOC
>> tree?
>>
>>
>>> I saw a few acked-by/tested-by/etc from the v5 posting which weren't
>>> carried over into v6 (might have been a timing race), so please fix
>>> that up if there's an opportunity.
>>
>> Right.  Both Robin and Tomasz gave their Reviewed-by to Patch #1 in v5
>> even after v6 was posted.
>>
>>
>>> Regarding the new DMA_ATTR_ALLOC_SINGLE_PAGES hint: I suggest adding
>>> "DMA_ATTR_ALLOC_SINGLE_PAGES is presently implemented only on ARM" to
>>> the docs.  Or perhaps have a shot at implementing it elsewhere.
>>
>> Warning sounds good.
>>
>>
>>> Typo in 4/5 changelog: "reqiurements"
>>
>> Thanks for catching!
>>
>>
>> I'm happy to post up a v6 with these things fixed or I'm happy for
>> whoever is applying it to make these small fixes themselves.  Any
>> volunteers?  Olof, Arnd, or Russell: any of you want these patches?
>
> I think it makes sense to send these through Russell's tracker for him
> to merge, especially since I don't think there are any dependencies on
> them for SoC-specific patches coming up.

Sounds good.  I'll make the nitfixes and I'll post a v7 directly to
Russell's tracker.  I'll follow up here with a link to those patches.

-Doug

Re: [PATCH v6 0/5] dma-mapping: Patches for speeding up allocation

[Doug Anderson]

Hi,

On Fri, Jan 29, 2016 at 1:58 PM, Doug Anderson <dianders@chromium.org> wrote:
>> I think it makes sense to send these through Russell's tracker for him
>> to merge, especially since I don't think there are any dependencies on
>> them for SoC-specific patches coming up.
>
> Sounds good.  I'll make the nitfixes and I'll post a v7 directly to
> Russell's tracker.  I'll follow up here with a link to those patches.

For your viewing pleasure:

8505/1 dma-mapping: Optimize allocation
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8505/1

8506/1 common: DMA-mapping: add DMA_ATTR_ALLOC_SINGLE_PAGES attribute
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8506/1

8507/1 dma-mapping: Use DMA_ATTR_ALLOC_SINGLE_PAGES hint to optimize alloc
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8507/1

8508/1 videobuf2-dc: Let drivers specify DMA attrs
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8508/1

8509/1 s5p-mfc: Set DMA_ATTR_ALLOC_SINGLE_PAGES
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8509/1

---

Changes in v7 (AKA the above patches):
- Add Robin and Tomasz Reviewed-by.
- Add Javier Tested-by.
- Add note that this is only implemented on ARM (Andrew Morton).
- Typo in commit message "reqiurements" (Andrew Morton).


-Doug

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Mauro Carvalho Chehab]

Em Mon, 11 Jan 2016 09:30:26 -0800
Douglas Anderson <dianders@chromium.org> escreveu:

> From: Tomasz Figa <tfiga@chromium.org>
> 
> DMA allocations might be subject to certain reqiurements specific to the
> hardware using the buffers, such as availability of kernel mapping (for
> contents fix-ups in the driver). The only entity that knows them is the
> driver, so it must share this knowledge with vb2-dc.
> 
> This patch extends the alloc_ctx initialization interface to let the
> driver specify DMA attrs, which are then stored inside the allocation
> context and will be used for all allocations with that context.
> 
> As a side effect, all dma_*_coherent() calls are turned into
> dma_*_attrs() calls, because the attributes need to be carried over
> through all DMA operations.
> 
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Didn't test it, but provided that this is tested, I'm ok that such
patch would be merged via the ARM tree.

So, after testing with upstream Kernel:
	Acked-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

> ---
> Changes in v6: None
> Changes in v5:
> - Let drivers specify DMA attrs new for v5
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 33 +++++++++++++++++---------
>  include/media/videobuf2-dma-contig.h           | 11 ++++++++-
>  2 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index c33127284cfe..5361197f3e57 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -23,13 +23,16 @@
>  
>  struct vb2_dc_conf {
>  	struct device		*dev;
> +	struct dma_attrs	attrs;
>  };
>  
>  struct vb2_dc_buf {
>  	struct device			*dev;
>  	void				*vaddr;
>  	unsigned long			size;
> +	void				*cookie;
>  	dma_addr_t			dma_addr;
> +	struct dma_attrs		attrs;
>  	enum dma_data_direction		dma_dir;
>  	struct sg_table			*dma_sgt;
>  	struct frame_vector		*vec;
> @@ -131,7 +134,8 @@ static void vb2_dc_put(void *buf_priv)
>  		sg_free_table(buf->sgt_base);
>  		kfree(buf->sgt_base);
>  	}
> -	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> +	dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
> +			&buf->attrs);
>  	put_device(buf->dev);
>  	kfree(buf);
>  }
> @@ -147,14 +151,18 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size,
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
>  
> -	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
> -						GFP_KERNEL | gfp_flags);
> -	if (!buf->vaddr) {
> +	buf->attrs = conf->attrs;
> +	buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
> +					GFP_KERNEL | gfp_flags, &buf->attrs);
> +	if (!buf->cookie) {
>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
>  		kfree(buf);
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->attrs))
> +		buf->vaddr = buf->cookie;
> +
>  	/* Prevent the device from being released while the buffer is used */
>  	buf->dev = get_device(dev);
>  	buf->size = size;
> @@ -185,8 +193,8 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
>  	 */
>  	vma->vm_pgoff = 0;
>  
> -	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
> -		buf->dma_addr, buf->size);
> +	ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
> +		buf->dma_addr, buf->size, &buf->attrs);
>  
>  	if (ret) {
>  		pr_err("Remapping memory failed, error: %d\n", ret);
> @@ -329,7 +337,7 @@ static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum)
>  {
>  	struct vb2_dc_buf *buf = dbuf->priv;
>  
> -	return buf->vaddr + pgnum * PAGE_SIZE;
> +	return buf->vaddr ? buf->vaddr + pgnum * PAGE_SIZE : NULL;
>  }
>  
>  static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
> @@ -368,8 +376,8 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf)
>  		return NULL;
>  	}
>  
> -	ret = dma_get_sgtable(buf->dev, sgt, buf->vaddr, buf->dma_addr,
> -		buf->size);
> +	ret = dma_get_sgtable_attrs(buf->dev, sgt, buf->cookie, buf->dma_addr,
> +		buf->size, &buf->attrs);
>  	if (ret < 0) {
>  		dev_err(buf->dev, "failed to get scatterlist from DMA API\n");
>  		kfree(sgt);
> @@ -721,7 +729,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
>  };
>  EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>  
> -void *vb2_dma_contig_init_ctx(struct device *dev)
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs)
>  {
>  	struct vb2_dc_conf *conf;
>  
> @@ -730,10 +739,12 @@ void *vb2_dma_contig_init_ctx(struct device *dev)
>  		return ERR_PTR(-ENOMEM);
>  
>  	conf->dev = dev;
> +	if (attrs)
> +		conf->attrs = *attrs;
>  
>  	return conf;
>  }
> -EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx_attrs);
>  
>  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>  {
> diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
> index c33dfa69d7ab..2087c9a68be3 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -16,6 +16,8 @@
>  #include <media/videobuf2-v4l2.h>
>  #include <linux/dma-mapping.h>
>  
> +struct dma_attrs;
> +
>  static inline dma_addr_t
>  vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>  {
> @@ -24,7 +26,14 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>  	return *addr;
>  }
>  
> -void *vb2_dma_contig_init_ctx(struct device *dev);
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs);
> +
> +static inline void *vb2_dma_contig_init_ctx(struct device *dev)
> +{
> +	return vb2_dma_contig_init_ctx_attrs(dev, NULL);
> +}
> +
>  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
>  
>  extern const struct vb2_mem_ops vb2_dma_contig_memops;

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Marek Szyprowski]

Hello,

On 2016-01-11 18:30, Douglas Anderson wrote:
> From: Tomasz Figa <tfiga@chromium.org>
>
> DMA allocations might be subject to certain reqiurements specific to the
> hardware using the buffers, such as availability of kernel mapping (for
> contents fix-ups in the driver). The only entity that knows them is the
> driver, so it must share this knowledge with vb2-dc.
>
> This patch extends the alloc_ctx initialization interface to let the
> driver specify DMA attrs, which are then stored inside the allocation
> context and will be used for all allocations with that context.
>
> As a side effect, all dma_*_coherent() calls are turned into
> dma_*_attrs() calls, because the attributes need to be carried over
> through all DMA operations.
>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Changes in v6: None
> Changes in v5:
> - Let drivers specify DMA attrs new for v5
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 33 +++++++++++++++++---------
>   include/media/videobuf2-dma-contig.h           | 11 ++++++++-
>   2 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index c33127284cfe..5361197f3e57 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -23,13 +23,16 @@
>   
>   struct vb2_dc_conf {
>   	struct device		*dev;
> +	struct dma_attrs	attrs;
>   };
>   
>   struct vb2_dc_buf {
>   	struct device			*dev;
>   	void				*vaddr;
>   	unsigned long			size;
> +	void				*cookie;
>   	dma_addr_t			dma_addr;
> +	struct dma_attrs		attrs;
>   	enum dma_data_direction		dma_dir;
>   	struct sg_table			*dma_sgt;
>   	struct frame_vector		*vec;
> @@ -131,7 +134,8 @@ static void vb2_dc_put(void *buf_priv)
>   		sg_free_table(buf->sgt_base);
>   		kfree(buf->sgt_base);
>   	}
> -	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> +	dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
> +			&buf->attrs);
>   	put_device(buf->dev);
>   	kfree(buf);
>   }
> @@ -147,14 +151,18 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size,
>   	if (!buf)
>   		return ERR_PTR(-ENOMEM);
>   
> -	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
> -						GFP_KERNEL | gfp_flags);
> -	if (!buf->vaddr) {
> +	buf->attrs = conf->attrs;
> +	buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
> +					GFP_KERNEL | gfp_flags, &buf->attrs);
> +	if (!buf->cookie) {
>   		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
>   		kfree(buf);
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
> +	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->attrs))
> +		buf->vaddr = buf->cookie;
> +
>   	/* Prevent the device from being released while the buffer is used */
>   	buf->dev = get_device(dev);
>   	buf->size = size;
> @@ -185,8 +193,8 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
>   	 */
>   	vma->vm_pgoff = 0;
>   
> -	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
> -		buf->dma_addr, buf->size);
> +	ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
> +		buf->dma_addr, buf->size, &buf->attrs);
>   
>   	if (ret) {
>   		pr_err("Remapping memory failed, error: %d\n", ret);
> @@ -329,7 +337,7 @@ static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum)
>   {
>   	struct vb2_dc_buf *buf = dbuf->priv;
>   
> -	return buf->vaddr + pgnum * PAGE_SIZE;
> +	return buf->vaddr ? buf->vaddr + pgnum * PAGE_SIZE : NULL;
>   }
>   
>   static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
> @@ -368,8 +376,8 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf)
>   		return NULL;
>   	}
>   
> -	ret = dma_get_sgtable(buf->dev, sgt, buf->vaddr, buf->dma_addr,
> -		buf->size);
> +	ret = dma_get_sgtable_attrs(buf->dev, sgt, buf->cookie, buf->dma_addr,
> +		buf->size, &buf->attrs);
>   	if (ret < 0) {
>   		dev_err(buf->dev, "failed to get scatterlist from DMA API\n");
>   		kfree(sgt);
> @@ -721,7 +729,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
>   };
>   EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>   
> -void *vb2_dma_contig_init_ctx(struct device *dev)
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs)
>   {
>   	struct vb2_dc_conf *conf;
>   
> @@ -730,10 +739,12 @@ void *vb2_dma_contig_init_ctx(struct device *dev)
>   		return ERR_PTR(-ENOMEM);
>   
>   	conf->dev = dev;
> +	if (attrs)
> +		conf->attrs = *attrs;
>   
>   	return conf;
>   }
> -EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx_attrs);
>   
>   void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>   {
> diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
> index c33dfa69d7ab..2087c9a68be3 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -16,6 +16,8 @@
>   #include <media/videobuf2-v4l2.h>
>   #include <linux/dma-mapping.h>
>   
> +struct dma_attrs;
> +
>   static inline dma_addr_t
>   vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>   {
> @@ -24,7 +26,14 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>   	return *addr;
>   }
>   
> -void *vb2_dma_contig_init_ctx(struct device *dev);
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs);
> +
> +static inline void *vb2_dma_contig_init_ctx(struct device *dev)
> +{
> +	return vb2_dma_contig_init_ctx_attrs(dev, NULL);
> +}
> +
>   void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
>   
>   extern const struct vb2_mem_ops vb2_dma_contig_memops;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Doug Anderson]

On Mon, Feb 1, 2016 at 5:29 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On 2016-01-11 18:30, Douglas Anderson wrote:
>>
>> From: Tomasz Figa <tfiga@chromium.org>
>>
>> DMA allocations might be subject to certain reqiurements specific to the
>> hardware using the buffers, such as availability of kernel mapping (for
>> contents fix-ups in the driver). The only entity that knows them is the
>> driver, so it must share this knowledge with vb2-dc.
>>
>> This patch extends the alloc_ctx initialization interface to let the
>> driver specify DMA attrs, which are then stored inside the allocation
>> context and will be used for all allocations with that context.
>>
>> As a side effect, all dma_*_coherent() calls are turned into
>> dma_*_attrs() calls, because the attributes need to be carried over
>> through all DMA operations.
>>
>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

Added both your ans Mauro's acks to the current patch in RMK's patch
tracker.  You can see the patch at
<http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8508/2>

Note that Javier has tested this series upstream on a Samsung
Chromebook and validated that the allocations are working as intended,
even if MFC is a bit tricky to get working properly upstream.


-Doug

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Hans Verkuil]

Hi Doug,

Is there any reason to think that different planes will need different
DMA attrs? I ask because this patch series of mine:

http://www.spinics.net/lists/linux-media/msg97522.html

does away with allocating allocation contexts (struct vb2_dc_conf).

For dma_attr this would mean that struct dma_attrs would probably be implemented
as a struct dma_attrs pointer in the vb2_queue struct once I rebase that patch series
on top of this patch. In other words, the same dma_attrs struct would be used for all
planes.

Second question: would specifying dma_attrs also make sense for videobuf2-dma-sg.c?

Regards,

	Hans

On 01/11/2016 06:30 PM, Douglas Anderson wrote:
> From: Tomasz Figa <tfiga@chromium.org>
> 
> DMA allocations might be subject to certain reqiurements specific to the
> hardware using the buffers, such as availability of kernel mapping (for
> contents fix-ups in the driver). The only entity that knows them is the
> driver, so it must share this knowledge with vb2-dc.
> 
> This patch extends the alloc_ctx initialization interface to let the
> driver specify DMA attrs, which are then stored inside the allocation
> context and will be used for all allocations with that context.
> 
> As a side effect, all dma_*_coherent() calls are turned into
> dma_*_attrs() calls, because the attributes need to be carried over
> through all DMA operations.
> 
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v6: None
> Changes in v5:
> - Let drivers specify DMA attrs new for v5
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 33 +++++++++++++++++---------
>  include/media/videobuf2-dma-contig.h           | 11 ++++++++-
>  2 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index c33127284cfe..5361197f3e57 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -23,13 +23,16 @@
>  
>  struct vb2_dc_conf {
>  	struct device		*dev;
> +	struct dma_attrs	attrs;
>  };
>  
>  struct vb2_dc_buf {
>  	struct device			*dev;
>  	void				*vaddr;
>  	unsigned long			size;
> +	void				*cookie;
>  	dma_addr_t			dma_addr;
> +	struct dma_attrs		attrs;
>  	enum dma_data_direction		dma_dir;
>  	struct sg_table			*dma_sgt;
>  	struct frame_vector		*vec;
> @@ -131,7 +134,8 @@ static void vb2_dc_put(void *buf_priv)
>  		sg_free_table(buf->sgt_base);
>  		kfree(buf->sgt_base);
>  	}
> -	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
> +	dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
> +			&buf->attrs);
>  	put_device(buf->dev);
>  	kfree(buf);
>  }
> @@ -147,14 +151,18 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size,
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
>  
> -	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
> -						GFP_KERNEL | gfp_flags);
> -	if (!buf->vaddr) {
> +	buf->attrs = conf->attrs;
> +	buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
> +					GFP_KERNEL | gfp_flags, &buf->attrs);
> +	if (!buf->cookie) {
>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
>  		kfree(buf);
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->attrs))
> +		buf->vaddr = buf->cookie;
> +
>  	/* Prevent the device from being released while the buffer is used */
>  	buf->dev = get_device(dev);
>  	buf->size = size;
> @@ -185,8 +193,8 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
>  	 */
>  	vma->vm_pgoff = 0;
>  
> -	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
> -		buf->dma_addr, buf->size);
> +	ret = dma_mmap_attrs(buf->dev, vma, buf->cookie,
> +		buf->dma_addr, buf->size, &buf->attrs);
>  
>  	if (ret) {
>  		pr_err("Remapping memory failed, error: %d\n", ret);
> @@ -329,7 +337,7 @@ static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum)
>  {
>  	struct vb2_dc_buf *buf = dbuf->priv;
>  
> -	return buf->vaddr + pgnum * PAGE_SIZE;
> +	return buf->vaddr ? buf->vaddr + pgnum * PAGE_SIZE : NULL;
>  }
>  
>  static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
> @@ -368,8 +376,8 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf)
>  		return NULL;
>  	}
>  
> -	ret = dma_get_sgtable(buf->dev, sgt, buf->vaddr, buf->dma_addr,
> -		buf->size);
> +	ret = dma_get_sgtable_attrs(buf->dev, sgt, buf->cookie, buf->dma_addr,
> +		buf->size, &buf->attrs);
>  	if (ret < 0) {
>  		dev_err(buf->dev, "failed to get scatterlist from DMA API\n");
>  		kfree(sgt);
> @@ -721,7 +729,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
>  };
>  EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>  
> -void *vb2_dma_contig_init_ctx(struct device *dev)
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs)
>  {
>  	struct vb2_dc_conf *conf;
>  
> @@ -730,10 +739,12 @@ void *vb2_dma_contig_init_ctx(struct device *dev)
>  		return ERR_PTR(-ENOMEM);
>  
>  	conf->dev = dev;
> +	if (attrs)
> +		conf->attrs = *attrs;
>  
>  	return conf;
>  }
> -EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx_attrs);
>  
>  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>  {
> diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
> index c33dfa69d7ab..2087c9a68be3 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -16,6 +16,8 @@
>  #include <media/videobuf2-v4l2.h>
>  #include <linux/dma-mapping.h>
>  
> +struct dma_attrs;
> +
>  static inline dma_addr_t
>  vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>  {
> @@ -24,7 +26,14 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
>  	return *addr;
>  }
>  
> -void *vb2_dma_contig_init_ctx(struct device *dev);
> +void *vb2_dma_contig_init_ctx_attrs(struct device *dev,
> +				    struct dma_attrs *attrs);
> +
> +static inline void *vb2_dma_contig_init_ctx(struct device *dev)
> +{
> +	return vb2_dma_contig_init_ctx_attrs(dev, NULL);
> +}
> +
>  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
>  
>  extern const struct vb2_mem_ops vb2_dma_contig_memops;
> 

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Doug Anderson]

Hans,

On Wed, Feb 17, 2016 at 9:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Doug,
>
> Is there any reason to think that different planes will need different
> DMA attrs? I ask because this patch series of mine:
>
> http://www.spinics.net/lists/linux-media/msg97522.html
>
> does away with allocating allocation contexts (struct vb2_dc_conf).
>
> For dma_attr this would mean that struct dma_attrs would probably be implemented
> as a struct dma_attrs pointer in the vb2_queue struct once I rebase that patch series
> on top of this patch. In other words, the same dma_attrs struct would be used for all
> planes.
>
> Second question: would specifying dma_attrs also make sense for videobuf2-dma-sg.c?

Those are all probably better questions for someone like Tomasz, who
authored ${SUBJECT} patch.  I mostly ended up touching this codepath
by going down the rabbit hole chasing a bug.  In my particular case I
was seeing that video was eating up all the large chunks in the system
needlessly and starving other memory users.  Using DMA attrs was the
logical way to indicate that we didn't need large chunks, so I used
it.  Other than that I'm totally unfamiliar with the video subsystem.


-Doug

Re: [PATCH v6 4/5] videobuf2-dc: Let drivers specify DMA attrs

[Tomasz Figa]

Hi Hans,

On Thu, Feb 18, 2016 at 2:00 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi Doug,
>
> Is there any reason to think that different planes will need different
> DMA attrs? I ask because this patch series of mine:
>
> http://www.spinics.net/lists/linux-media/msg97522.html
>
> does away with allocating allocation contexts (struct vb2_dc_conf).
>
> For dma_attr this would mean that struct dma_attrs would probably be implemented
> as a struct dma_attrs pointer in the vb2_queue struct once I rebase that patch series
> on top of this patch. In other words, the same dma_attrs struct would be used for all
> planes.

I could think of some format consisting of video and metadata planes
(such as V4L2_PIX_FMT_S5C_UYVY_JPG) and some hypothetical hardware,
which generates the metadata in a way that requires patching in
.buf_finish(). In this case, we can allocate video plane without
kernel mapping, but for metadata plane kernel mapping is necessary to
do the patching.

However the above is only a hypothetical "what if" of mine, since
personally I haven't seen such case yet. Our real use case is
allocating raw video planes without kernel mapping, while keeping
kernel mapping available for encoded bitstream, which needs some extra
patching. The reason for disabling kernel mapping is that vmalloc
space can be easily exhausted when processing high resolution video
with long buffer queues (e.g. high resolution H264 decode/encode).

>
> Second question: would specifying dma_attrs also make sense for videobuf2-dma-sg.c?

For our particular use case, probably not, because I don't see kernel
mapping being implicitly created at videobuf2-dma-sg level for
allocated MMAP buffers. AFAICT only if vb2_plane_vaddr() or respective
DMA-BUF op is called then the mapping is created, which is unavoidable
because the caller apparently needs it for something.

Best regards,
Tomasz