Re: [PATCH RESEND 1/2] media: vb2-dma-contig: add helper for setting dma max seg size

[Hans Verkuil]

On 04/27/2016 03:14 PM, Marek Szyprowski wrote:
> Hello,
> 
> 
> On 2016-04-27 14:58, Hans Verkuil wrote:
>> On 04/27/2016 02:23 PM, Marek Szyprowski wrote:
>>> Hello,
>>>
>>>
>>> On 2016-04-27 14:10, Hans Verkuil wrote:
 On 04/27/2016 02:00 PM, Marek Szyprowski wrote:
> Add a helper function for device drivers to set DMA's max_seg_size.
> Setting it to largest possible value lets DMA-mapping API always create
> contiguous mappings in DMA address space. This is essential for all
> devices, which use dma-contig videobuf2 memory allocator and shared
> buffers.
>
> Signed-off-by: Marek Szyprowski 
> ---
> This patch was posted earlier as a part of
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> thread, but applying it is really needed to get all Exynos multimedia
> drivers working with IOMMU enabled.
>
> Best regards,
> Marek Szyprowski
> ---
>drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
>include/media/videobuf2-dma-contig.h   |  1 +
>2 files changed, 16 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5361197..f611456 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -753,6 +753,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>}
>EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
>+int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int 
> size)
> +{
> +if (!dev->dma_parms) {
> +dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
> +  GFP_KERNEL);
 The v3 patch from December uses kzalloc here. Is this perhaps on old 
 version?
>>> Right, my fault. I will do another resend (and fix the typo in the second 
>>> patch).
>>>
> +if (!dev->dma_parms)
> +return -ENOMEM;
> +}
> +if (dma_get_max_seg_size(dev) < size)
> +return dma_set_max_seg_size(dev, size);
> +
> +return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
 Admittedly I haven't looked closely at this, but is this something that you
 want for all dma-contig devices? Or to rephrase this question: what type of
 devices will need this?
>>> This is needed for all devices using vb2-dc, iommu and user-ptr mode, 
>>> however
>>> in the previous discussions (see https://patchwork.linuxtv.org/patch/30870/
>>> ) it has been suggested to make it via common helper instead of forcing it
>>> in vb2-dc.
>> This certainly will need to be carefully documented in 
>> videobuf2-dma-contig.h.
>>
>> What happens if it is called when you don't have an iommu? Does something 
>> break?
> 
> Nope, nothing breaks in such case. When no iommu is available this parameter 
> is
> ignored by dma-mapping layer. Due to some other issues, it cannot be set by
> generic platform init code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html

OK.

Is there any reason this cannot be done in vb2-dma-contig.c itself? If there is
no iommu, then this does nothing, and if there is one, don't you always need it?

Requiring drivers to set this fairly obscure setting is asking for problems.
This is guaranteed to be forgotten.

As Lars-Peter said, ideally this is done by the DMA provider, but apparently
that is (for now at least) a no-go. But the next best thing is vb2 IMHO.

I gather that this is specific to userptr mode, so placing this code in
vb2_dc_get_userptr() would make sense. You can even check the size here and
do nothing if it is < 64 kB. Note sure if that is useful, though.

Don't put it in vb2_dma_contig_init_ctx though as I am trying to get rid of
that: http://www.mail-archive.com/linux-media@vger.kernel.org/msg96741.html

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: [PATCH RESEND 1/2] media: vb2-dma-contig: add helper for setting dma max seg size

[Marek Szyprowski]

Hello,


On 2016-04-27 14:58, Hans Verkuil wrote:

On 04/27/2016 02:23 PM, Marek Szyprowski wrote:

Hello,


On 2016-04-27 14:10, Hans Verkuil wrote:

On 04/27/2016 02:00 PM, Marek Szyprowski wrote:

Add a helper function for device drivers to set DMA's max_seg_size.
Setting it to largest possible value lets DMA-mapping API always create
contiguous mappings in DMA address space. This is essential for all
devices, which use dma-contig videobuf2 memory allocator and shared
buffers.

Signed-off-by: Marek Szyprowski 
---
This patch was posted earlier as a part of
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
thread, but applying it is really needed to get all Exynos multimedia
drivers working with IOMMU enabled.

Best regards,
Marek Szyprowski
---
   drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
   include/media/videobuf2-dma-contig.h   |  1 +
   2 files changed, 16 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197..f611456 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -753,6 +753,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
   }
   EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
   +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
+{
+if (!dev->dma_parms) {
+dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
+  GFP_KERNEL);

The v3 patch from December uses kzalloc here. Is this perhaps on old version?

Right, my fault. I will do another resend (and fix the typo in the second 
patch).


+if (!dev->dma_parms)
+return -ENOMEM;
+}
+if (dma_get_max_seg_size(dev) < size)
+return dma_set_max_seg_size(dev, size);
+
+return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);

Admittedly I haven't looked closely at this, but is this something that you
want for all dma-contig devices? Or to rephrase this question: what type of
devices will need this?

This is needed for all devices using vb2-dc, iommu and user-ptr mode, however
in the previous discussions (see https://patchwork.linuxtv.org/patch/30870/
) it has been suggested to make it via common helper instead of forcing it
in vb2-dc.

This certainly will need to be carefully documented in videobuf2-dma-contig.h.

What happens if it is called when you don't have an iommu? Does something break?


Nope, nothing breaks in such case. When no iommu is available this 
parameter is

ignored by dma-mapping layer. Due to some other issues, it cannot be set by
generic platform init code:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html 



Best regards

--
Marek Szyprowski, PhD
Samsung R Institute Poland

--
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: [PATCH RESEND 1/2] media: vb2-dma-contig: add helper for setting dma max seg size

[Hans Verkuil]

On 04/27/2016 02:23 PM, Marek Szyprowski wrote:
> Hello,
> 
> 
> On 2016-04-27 14:10, Hans Verkuil wrote:
>> On 04/27/2016 02:00 PM, Marek Szyprowski wrote:
>>> Add a helper function for device drivers to set DMA's max_seg_size.
>>> Setting it to largest possible value lets DMA-mapping API always create
>>> contiguous mappings in DMA address space. This is essential for all
>>> devices, which use dma-contig videobuf2 memory allocator and shared
>>> buffers.
>>>
>>> Signed-off-by: Marek Szyprowski 
>>> ---
>>> This patch was posted earlier as a part of
>>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>>> thread, but applying it is really needed to get all Exynos multimedia
>>> drivers working with IOMMU enabled.
>>>
>>> Best regards,
>>> Marek Szyprowski
>>> ---
>>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
>>>   include/media/videobuf2-dma-contig.h   |  1 +
>>>   2 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
>>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> index 5361197..f611456 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> @@ -753,6 +753,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>>>   }
>>>   EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
>>>   +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int 
>>> size)
>>> +{
>>> +if (!dev->dma_parms) {
>>> +dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
>>> +  GFP_KERNEL);
>> The v3 patch from December uses kzalloc here. Is this perhaps on old version?
> 
> Right, my fault. I will do another resend (and fix the typo in the second 
> patch).
> 
>>> +if (!dev->dma_parms)
>>> +return -ENOMEM;
>>> +}
>>> +if (dma_get_max_seg_size(dev) < size)
>>> +return dma_set_max_seg_size(dev, size);
>>> +
>>> +return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);
>> Admittedly I haven't looked closely at this, but is this something that you
>> want for all dma-contig devices? Or to rephrase this question: what type of
>> devices will need this?
> 
> This is needed for all devices using vb2-dc, iommu and user-ptr mode, however
> in the previous discussions (see https://patchwork.linuxtv.org/patch/30870/
> ) it has been suggested to make it via common helper instead of forcing it
> in vb2-dc.

This certainly will need to be carefully documented in videobuf2-dma-contig.h.

What happens if it is called when you don't have an iommu? Does something break?

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: [PATCH RESEND 1/2] media: vb2-dma-contig: add helper for setting dma max seg size

[Marek Szyprowski]

Hello,


On 2016-04-27 14:10, Hans Verkuil wrote:

On 04/27/2016 02:00 PM, Marek Szyprowski wrote:

Add a helper function for device drivers to set DMA's max_seg_size.
Setting it to largest possible value lets DMA-mapping API always create
contiguous mappings in DMA address space. This is essential for all
devices, which use dma-contig videobuf2 memory allocator and shared
buffers.

Signed-off-by: Marek Szyprowski 
---
This patch was posted earlier as a part of
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
thread, but applying it is really needed to get all Exynos multimedia
drivers working with IOMMU enabled.

Best regards,
Marek Szyprowski
---
  drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
  include/media/videobuf2-dma-contig.h   |  1 +
  2 files changed, 16 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197..f611456 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -753,6 +753,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
  }
  EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
  
+int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)

+{
+   if (!dev->dma_parms) {
+   dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
+ GFP_KERNEL);

The v3 patch from December uses kzalloc here. Is this perhaps on old version?


Right, my fault. I will do another resend (and fix the typo in the 
second patch).



+   if (!dev->dma_parms)
+   return -ENOMEM;
+   }
+   if (dma_get_max_seg_size(dev) < size)
+   return dma_set_max_seg_size(dev, size);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);

Admittedly I haven't looked closely at this, but is this something that you
want for all dma-contig devices? Or to rephrase this question: what type of
devices will need this?


This is needed for all devices using vb2-dc, iommu and user-ptr mode, 
however

in the previous discussions (see https://patchwork.linuxtv.org/patch/30870/
) it has been suggested to make it via common helper instead of forcing it
in vb2-dc.

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

--
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: [PATCH RESEND 1/2] media: vb2-dma-contig: add helper for setting dma max seg size

[Hans Verkuil]

On 04/27/2016 02:00 PM, Marek Szyprowski wrote:
> Add a helper function for device drivers to set DMA's max_seg_size.
> Setting it to largest possible value lets DMA-mapping API always create
> contiguous mappings in DMA address space. This is essential for all
> devices, which use dma-contig videobuf2 memory allocator and shared
> buffers.
> 
> Signed-off-by: Marek Szyprowski 
> ---
> This patch was posted earlier as a part of
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> thread, but applying it is really needed to get all Exynos multimedia
> drivers working with IOMMU enabled.
> 
> Best regards,
> Marek Szyprowski
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++
>  include/media/videobuf2-dma-contig.h   |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5361197..f611456 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -753,6 +753,21 @@ void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
>  }
>  EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
>  
> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> + if (!dev->dma_parms) {
> + dev->dma_parms = devm_kzalloc(dev, sizeof(dev->dma_parms),
> +   GFP_KERNEL);

The v3 patch from December uses kzalloc here. Is this perhaps on old version?

> + if (!dev->dma_parms)
> + return -ENOMEM;
> + }
> + if (dma_get_max_seg_size(dev) < size)
> + return dma_set_max_seg_size(dev, size);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_dma_contig_set_max_seg_size);

Admittedly I haven't looked closely at this, but is this something that you
want for all dma-contig devices? Or to rephrase this question: what type of
devices will need this?

Regards,

Hans

> +
>  MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
>  MODULE_AUTHOR("Pawel Osciak ");
>  MODULE_LICENSE("GPL");
> diff --git a/include/media/videobuf2-dma-contig.h 
> b/include/media/videobuf2-dma-contig.h
> index 2087c9a..98857b5 100644
> --- a/include/media/videobuf2-dma-contig.h
> +++ b/include/media/videobuf2-dma-contig.h
> @@ -35,6 +35,7 @@ static inline void *vb2_dma_contig_init_ctx(struct device 
> *dev)
>  }
>  
>  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);
> +int vb2_dma_contig_set_max_seg_size(struct device *dev, unsigned int size);
>  
>  extern const struct vb2_mem_ops vb2_dma_contig_memops;
>  
> 
--
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