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

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

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
---
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);
+		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);
+
 MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
 MODULE_AUTHOR("Pawel Osciak <pawel@osciak.com>");
 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;
 
-- 
1.9.2

[PATCH RESEND 2/2] media: set proper max seg size for devices on Exynos SoCs

[Marek Szyprowski]

All multimedia devices found on Exynos SoCs support only contiguous
buffers, so set DMA max segment size to DMA_BIT_MASK(32) to let memory
allocator to correctly create contiguous memory mappings.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
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/platform/exynos-gsc/gsc-core.c  | 1 +
 drivers/media/platform/exynos4-is/fimc-core.c | 1 +
 drivers/media/platform/exynos4-is/fimc-is.c   | 1 +
 drivers/media/platform/exynos4-is/fimc-lite.c | 1 +
 drivers/media/platform/s5p-g2d/g2d.c          | 1 +
 drivers/media/platform/s5p-jpeg/jpeg-core.c   | 1 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c      | 2 ++
 drivers/media/platform/s5p-tv/mixer_video.c   | 1 +
 8 files changed, 9 insertions(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9b9e423..4f90be4 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1140,6 +1140,7 @@ static int gsc_probe(struct platform_device *pdev)
 		goto err_m2m;
 
 	/* Initialize continious memory allocator */
+	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	gsc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(gsc->alloc_ctx)) {
 		ret = PTR_ERR(gsc->alloc_ctx);
diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index cef2a7f..368e19b 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -1019,6 +1019,7 @@ static int fimc_probe(struct platform_device *pdev)
 	}
 
 	/* Initialize contiguous memory allocator */
+	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index 979c388..3f50856 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -847,6 +847,7 @@ static int fimc_is_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_pm;
 
+	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	is->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(is->alloc_ctx)) {
 		ret = PTR_ERR(is->alloc_ctx);
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index dc1b929..95841c8 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -1551,6 +1551,7 @@ static int fimc_lite_probe(struct platform_device *pdev)
 			goto err_sd;
 	}
 
+	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 74bd46c..5048b68 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -681,6 +681,7 @@ static int g2d_probe(struct platform_device *pdev)
 		goto put_clk_gate;
 	}
 
+	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
 	dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
 	if (IS_ERR(dev->alloc_ctx)) {
 		ret = PTR_ERR(dev->alloc_ctx);
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index c3b13a6..e535ccf 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2838,6 +2838,7 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
 		goto device_register_rollback;
 	}
 
+	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
 	jpeg->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
 	if (IS_ERR(jpeg->alloc_ctx)) {
 		v4l2_err(&jpeg->v4l2_dev, "Failed to init memory allocator\n");
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 927ab49..ae0bf26 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1164,11 +1164,13 @@ static int s5p_mfc_probe(struct platform_device *pdev)
 		}
 	}
 
+	vb2_dma_contig_set_max_seg_size(dev->mem_dev_l, DMA_BIT_MASK(32));
 	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);
 	if (IS_ERR(dev->alloc_ctx[0])) {
 		ret = PTR_ERR(dev->alloc_ctx[0]);
 		goto err_res;
 	}
+	vb2_dma_contig_set_max_seg_size(dev->mem_dev_r, DMA_BIT_MASK(32));
 	dev->alloc_ctx[1] = vb2_dma_contig_init_ctx(dev->mem_dev_r);
 	if (IS_ERR(dev->alloc_ctx[1])) {
 		ret = PTR_ERR(dev->alloc_ctx[1]);
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index d9e7f03..17d6df7 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -80,6 +80,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
 		goto fail;
 	}
 
+	vb2_dma_contig_set_max_seg_size(mdev->dev, DMA_BIT_MASK(32));
 	mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);
 	if (IS_ERR(mdev->alloc_ctx)) {
 		mxr_err(mdev, "could not acquire vb2 allocator\n");
-- 
1.9.2

Re: [PATCH RESEND 2/2] media: set proper max seg size for devices on Exynos SoCs

[Hans Verkuil]

On 04/27/2016 02:00 PM, Marek Szyprowski wrote:
> All multimedia devices found on Exynos SoCs support only contiguous
> buffers, so set DMA max segment size to DMA_BIT_MASK(32) to let memory
> allocator to correctly create contiguous memory mappings.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> 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/platform/exynos-gsc/gsc-core.c  | 1 +
>  drivers/media/platform/exynos4-is/fimc-core.c | 1 +
>  drivers/media/platform/exynos4-is/fimc-is.c   | 1 +
>  drivers/media/platform/exynos4-is/fimc-lite.c | 1 +
>  drivers/media/platform/s5p-g2d/g2d.c          | 1 +
>  drivers/media/platform/s5p-jpeg/jpeg-core.c   | 1 +
>  drivers/media/platform/s5p-mfc/s5p_mfc.c      | 2 ++
>  drivers/media/platform/s5p-tv/mixer_video.c   | 1 +
>  8 files changed, 9 insertions(+)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9b9e423..4f90be4 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1140,6 +1140,7 @@ static int gsc_probe(struct platform_device *pdev)
>  		goto err_m2m;
>  
>  	/* Initialize continious memory allocator */

Typo: continious -> contiguous

Regards,

	Hans


> +	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	gsc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
>  	if (IS_ERR(gsc->alloc_ctx)) {
>  		ret = PTR_ERR(gsc->alloc_ctx);
> diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
> index cef2a7f..368e19b 100644
> --- a/drivers/media/platform/exynos4-is/fimc-core.c
> +++ b/drivers/media/platform/exynos4-is/fimc-core.c
> @@ -1019,6 +1019,7 @@ static int fimc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Initialize contiguous memory allocator */
> +	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
>  	if (IS_ERR(fimc->alloc_ctx)) {
>  		ret = PTR_ERR(fimc->alloc_ctx);
> diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
> index 979c388..3f50856 100644
> --- a/drivers/media/platform/exynos4-is/fimc-is.c
> +++ b/drivers/media/platform/exynos4-is/fimc-is.c
> @@ -847,6 +847,7 @@ static int fimc_is_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_pm;
>  
> +	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	is->alloc_ctx = vb2_dma_contig_init_ctx(dev);
>  	if (IS_ERR(is->alloc_ctx)) {
>  		ret = PTR_ERR(is->alloc_ctx);
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> index dc1b929..95841c8 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -1551,6 +1551,7 @@ static int fimc_lite_probe(struct platform_device *pdev)
>  			goto err_sd;
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev);
>  	if (IS_ERR(fimc->alloc_ctx)) {
>  		ret = PTR_ERR(fimc->alloc_ctx);
> diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
> index 74bd46c..5048b68 100644
> --- a/drivers/media/platform/s5p-g2d/g2d.c
> +++ b/drivers/media/platform/s5p-g2d/g2d.c
> @@ -681,6 +681,7 @@ static int g2d_probe(struct platform_device *pdev)
>  		goto put_clk_gate;
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
>  	dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
>  	if (IS_ERR(dev->alloc_ctx)) {
>  		ret = PTR_ERR(dev->alloc_ctx);
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index c3b13a6..e535ccf 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2838,6 +2838,7 @@ static int s5p_jpeg_probe(struct platform_device *pdev)
>  		goto device_register_rollback;
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
>  	jpeg->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
>  	if (IS_ERR(jpeg->alloc_ctx)) {
>  		v4l2_err(&jpeg->v4l2_dev, "Failed to init memory allocator\n");
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 927ab49..ae0bf26 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1164,11 +1164,13 @@ static int s5p_mfc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(dev->mem_dev_l, DMA_BIT_MASK(32));
>  	dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);
>  	if (IS_ERR(dev->alloc_ctx[0])) {
>  		ret = PTR_ERR(dev->alloc_ctx[0]);
>  		goto err_res;
>  	}
> +	vb2_dma_contig_set_max_seg_size(dev->mem_dev_r, DMA_BIT_MASK(32));
>  	dev->alloc_ctx[1] = vb2_dma_contig_init_ctx(dev->mem_dev_r);
>  	if (IS_ERR(dev->alloc_ctx[1])) {
>  		ret = PTR_ERR(dev->alloc_ctx[1]);
> diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
> index d9e7f03..17d6df7 100644
> --- a/drivers/media/platform/s5p-tv/mixer_video.c
> +++ b/drivers/media/platform/s5p-tv/mixer_video.c
> @@ -80,6 +80,7 @@ int mxr_acquire_video(struct mxr_device *mdev,
>  		goto fail;
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(mdev->dev, DMA_BIT_MASK(32));
>  	mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);
>  	if (IS_ERR(mdev->alloc_ctx)) {
>  		mxr_err(mdev, "could not acquire vb2 allocator\n");
> 

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 <m.szyprowski@samsung.com>
> ---
> 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 <pawel@osciak.com>");
>  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;
>  
> 

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 <m.szyprowski@samsung.com>
>> ---
>> 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&D Institute Poland

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 <m.szyprowski@samsung.com>
>>> ---
>>> 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

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 <m.szyprowski@samsung.com>
>>>> ---
>>>> 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&D Institute Poland

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 <m.szyprowski@samsung.com>
>>>>> ---
>>>>> 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