[PATCH] media: vb2-dma-contig: configure DMA max segment size properly

[PATCH] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

This patch lets vb2-dma-contig memory allocator to configure DMA max
segment size properly for the client device. Setting it is needed to let
DMA-mapping subsystem to create a single, contiguous mapping in DMA
address space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
of operations).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Hello,

This patch is a follow-up of my previous attempts to let Exynos
multimedia devices to work properly with shared buffers when IOMMU is
enabled:
1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
3. https://patchwork.linuxtv.org/patch/30870/

As sugested by Hans, configuring DMA max segment size should be done by
videobuf2-dma-contig module instead of requiring all device drivers to
do it on their own.

Here is some backgroud why this is done in videobuf2-dc not in the
respective generic bus code:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html

Best regards,
Marek Szyprowski
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 461ae55eaa98..871e370f8e88 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -443,6 +443,35 @@ static void vb2_dc_put_userptr(void *buf_priv)
 }
 
 /*
+ * To allow mapping scatter-list into signle chunk in DMA address space,
+ * device is required to have DMA max segment size parameter set to value
+ * larger than the buffer size. Otherwise, DMA-mapping subsystem will
+ * split the mapping into max segment size chunks.
+ * This function increases DMA max segment size parameter to let
+ * DMA-mapping to map a buffer as a single chunk in DMA address space.
+ * This code assumes that the DMA-mapping subsystem will merge all
+ * scatter-list segments if this is really possible (for example when
+ * IOMMU is available and enabled).
+ * Ideally, this parameter should be set by generic bus code, but it is
+ * left with the default 64KiB value due to some historical limitations
+ * in other subsystems (like limited USB host drivers) and there is no
+ * good place to set it to the proper value. It is done here to avoid
+ * fixing all vb2-dc client drivers.
+ */
+static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	if (!dev->dma_parms) {
+		dev->dma_parms = kzalloc(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;
+}
+
+/*
  * For some kind of reserved memory there might be no struct page available,
  * so all that can be done to support such 'pages' is to try to convert
  * pfn to dma address or at the last resort just assume that
@@ -499,6 +528,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
 		return ERR_PTR(-EINVAL);
 	}
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
@@ -675,10 +708,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 {
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
+	int ret;
 
 	if (dbuf->size < size)
 		return ERR_PTR(-EFAULT);
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
@@ -722,6 +760,7 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
 };
 EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
 
+
 MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
 MODULE_AUTHOR("Pawel Osciak <pawel@osciak.com>");
 MODULE_LICENSE("GPL");
-- 
1.9.2

Re: [PATCH] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

Hi Marek,

Looks good, just a typo and a large sprinkling of extra 'the' in the text :-)

On 04/28/2016 02:12 PM, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 461ae55eaa98..871e370f8e88 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -443,6 +443,35 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping scatter-list into signle chunk in DMA address space,

the scatter-list
signle -> a single
the DMA address space

> + * device is required to have DMA max segment size parameter set to value

the device
the DMA max segment
set to a value

> + * larger than the buffer size. Otherwise, DMA-mapping subsystem will

the DMA-mapping

> + * split the mapping into max segment size chunks.
> + * This function increases DMA max segment size parameter to let

the MDA max

> + * DMA-mapping to map a buffer as a single chunk in DMA address space.

the DMA-mapping map a buffer

> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatter-list segments if this is really possible (for example when
> + * IOMMU is available and enabled).

an IOMMU

> + * Ideally, this parameter should be set by generic bus code, but it is

the generic bus code

> + * left with the default 64KiB value due to some historical limitations
> + * in other subsystems (like limited USB host drivers) and there is no
> + * good place to set it to the proper value. It is done here to avoid
> + * fixing all vb2-dc client drivers.

all the vb2-dc

> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(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;
> +}
> +
> +/*
>   * For some kind of reserved memory there might be no struct page available,
>   * so all that can be done to support such 'pages' is to try to convert
>   * pfn to dma address or at the last resort just assume that
> @@ -499,6 +528,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> @@ -675,10 +708,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  {
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
> +	int ret;
>  
>  	if (dbuf->size < size)
>  		return ERR_PTR(-EFAULT);
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> @@ -722,6 +760,7 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
>  };
>  EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>  
> +

Spurious newline. Intended or a mistake?

>  MODULE_DESCRIPTION("DMA-contig memory handling routines for videobuf2");
>  MODULE_AUTHOR("Pawel Osciak <pawel@osciak.com>");
>  MODULE_LICENSE("GPL");
> 

Regards,

	Hans

[PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

This patch lets vb2-dma-contig memory allocator to configure DMA max
segment size properly for the client device. Setting it is needed to let
DMA-mapping subsystem to create a single, contiguous mapping in DMA
address space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
of operations).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Hello,

This patch is a follow-up of my previous attempts to let Exynos
multimedia devices to work properly with shared buffers when IOMMU is
enabled:
1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
3. https://patchwork.linuxtv.org/patch/30870/

As sugested by Hans, configuring DMA max segment size should be done by
videobuf2-dma-contig module instead of requiring all device drivers to
do it on their own.

Here is some backgroud why this is done in videobuf2-dc not in the
respective generic bus code:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html

Best regards,
Marek Szyprowski

changelog:
v2:
- fixes typos and other language issues in the comments

v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 461ae55eaa98..d0382d62954d 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
 }
 
 /*
+ * To allow mapping the scatter-list into a single chunk in the DMA
+ * address space, the device is required to have the DMA max segment
+ * size parameter set to a value larger than the buffer size. Otherwise,
+ * the DMA-mapping subsystem will split the mapping into max segment
+ * size chunks. This function increases the DMA max segment size
+ * parameter to let DMA-mapping map a buffer as a single chunk in DMA
+ * address space.
+ * This code assumes that the DMA-mapping subsystem will merge all
+ * scatter-list segments if this is really possible (for example when
+ * an IOMMU is available and enabled).
+ * Ideally, this parameter should be set by the generic bus code, but it
+ * is left with the default 64KiB value due to historical litmiations in
+ * other subsystems (like limited USB host drivers) and there no good
+ * place to set it to the proper value. It is done here to avoid fixing
+ * all the vb2-dc client drivers.
+ */
+static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	if (!dev->dma_parms) {
+		dev->dma_parms = kzalloc(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;
+}
+
+/*
  * For some kind of reserved memory there might be no struct page available,
  * so all that can be done to support such 'pages' is to try to convert
  * pfn to dma address or at the last resort just assume that
@@ -499,6 +529,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
 		return ERR_PTR(-EINVAL);
 	}
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
@@ -675,10 +709,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 {
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
+	int ret;
 
 	if (dbuf->size < size)
 		return ERR_PTR(-EFAULT);
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
-- 
1.9.2

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

On 04/28/2016 03:20 PM, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Should this go in for 4.7? (might be too late) Should this go into older
kernels as well?

Regards,

	Hans

> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> 
> changelog:
> v2:
> - fixes typos and other language issues in the comments
> 
> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 461ae55eaa98..d0382d62954d 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping the scatter-list into a single chunk in the DMA
> + * address space, the device is required to have the DMA max segment
> + * size parameter set to a value larger than the buffer size. Otherwise,
> + * the DMA-mapping subsystem will split the mapping into max segment
> + * size chunks. This function increases the DMA max segment size
> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> + * address space.
> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatter-list segments if this is really possible (for example when
> + * an IOMMU is available and enabled).
> + * Ideally, this parameter should be set by the generic bus code, but it
> + * is left with the default 64KiB value due to historical litmiations in
> + * other subsystems (like limited USB host drivers) and there no good
> + * place to set it to the proper value. It is done here to avoid fixing
> + * all the vb2-dc client drivers.
> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(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;
> +}
> +
> +/*
>   * For some kind of reserved memory there might be no struct page available,
>   * so all that can be done to support such 'pages' is to try to convert
>   * pfn to dma address or at the last resort just assume that
> @@ -499,6 +529,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> @@ -675,10 +709,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  {
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
> +	int ret;
>  
>  	if (dbuf->size < size)
>  		return ERR_PTR(-EFAULT);
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Hello,


On 2016-04-28 15:29, Hans Verkuil wrote:
> On 04/28/2016 03:20 PM, Marek Szyprowski wrote:
>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>> segment size properly for the client device. Setting it is needed to let
>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>> address space. This is essential for all devices, which use dma-contig
>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>> of operations).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> Should this go in for 4.7? (might be too late) Should this go into older
> kernels as well?

Yes, please queue it to v4.7 if possible.

It might be a good idea to backport this to older kernels, but at least
in case of Exynos there are still other issues, which prevent using all
multimedia drivers with IOMMU enabled (caused by Exynos MFC device special
needs).

>
> Regards,
>
> 	Hans
>
>> ---
>> Hello,
>>
>> This patch is a follow-up of my previous attempts to let Exynos
>> multimedia devices to work properly with shared buffers when IOMMU is
>> enabled:
>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>> 3. https://patchwork.linuxtv.org/patch/30870/
>>
>> As sugested by Hans, configuring DMA max segment size should be done by
>> videobuf2-dma-contig module instead of requiring all device drivers to
>> do it on their own.
>>
>> Here is some backgroud why this is done in videobuf2-dc not in the
>> respective generic bus code:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>
>> Best regards,
>> Marek Szyprowski
>>
>> changelog:
>> v2:
>> - fixes typos and other language issues in the comments
>>
>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 461ae55eaa98..d0382d62954d 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>   }
>>   
>>   /*
>> + * To allow mapping the scatter-list into a single chunk in the DMA
>> + * address space, the device is required to have the DMA max segment
>> + * size parameter set to a value larger than the buffer size. Otherwise,
>> + * the DMA-mapping subsystem will split the mapping into max segment
>> + * size chunks. This function increases the DMA max segment size
>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>> + * address space.
>> + * This code assumes that the DMA-mapping subsystem will merge all
>> + * scatter-list segments if this is really possible (for example when
>> + * an IOMMU is available and enabled).
>> + * Ideally, this parameter should be set by the generic bus code, but it
>> + * is left with the default 64KiB value due to historical litmiations in
>> + * other subsystems (like limited USB host drivers) and there no good
>> + * place to set it to the proper value. It is done here to avoid fixing
>> + * all the vb2-dc client drivers.
>> + */
>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>> +{
>> +	if (!dev->dma_parms) {
>> +		dev->dma_parms = kzalloc(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;
>> +}
>> +
>> +/*
>>    * For some kind of reserved memory there might be no struct page available,
>>    * so all that can be done to support such 'pages' is to try to convert
>>    * pfn to dma address or at the last resort just assume that
>> @@ -499,6 +529,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);
>> @@ -675,10 +709,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   {
>>   	struct vb2_dc_buf *buf;
>>   	struct dma_buf_attachment *dba;
>> +	int ret;
>>   
>>   	if (dbuf->size < size)
>>   		return ERR_PTR(-EFAULT);
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);
>>
>

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

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

On 04/28/2016 03:39 PM, Marek Szyprowski wrote:
> Hello,
> 
> 
> On 2016-04-28 15:29, Hans Verkuil wrote:
>> On 04/28/2016 03:20 PM, Marek Szyprowski wrote:
>>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>>> segment size properly for the client device. Setting it is needed to let
>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>>> address space. This is essential for all devices, which use dma-contig
>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>>> of operations).
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Should this go in for 4.7? (might be too late) Should this go into older
>> kernels as well?

Oops, typo. I meant 4.6, not 4.7. I gather that committing this for 4.6 is not
necessary, so I just queue it up for 4.7.

Regards,

	Hans

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Sakari Ailus]

Hi Marek,

Thanks for the patch!

On Thu, Apr 28, 2016 at 03:20:03PM +0200, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> 
> changelog:
> v2:
> - fixes typos and other language issues in the comments
> 
> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 461ae55eaa98..d0382d62954d 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping the scatter-list into a single chunk in the DMA
> + * address space, the device is required to have the DMA max segment
> + * size parameter set to a value larger than the buffer size. Otherwise,
> + * the DMA-mapping subsystem will split the mapping into max segment
> + * size chunks. This function increases the DMA max segment size
> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> + * address space.
> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatter-list segments if this is really possible (for example when
> + * an IOMMU is available and enabled).
> + * Ideally, this parameter should be set by the generic bus code, but it
> + * is left with the default 64KiB value due to historical litmiations in
> + * other subsystems (like limited USB host drivers) and there no good
> + * place to set it to the proper value. It is done here to avoid fixing
> + * all the vb2-dc client drivers.
> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);

Doesn't this create a memory leak? Do consider that devices may be also
removed from the system at runtime.

Looks very nice otherwise.

> +		if (!dev->dma_parms)
> +			return -ENOMEM;
> +	}
> +	if (dma_get_max_seg_size(dev) < size)
> +		return dma_set_max_seg_size(dev, size);
> +
> +	return 0;
> +}
> +
> +/*
>   * For some kind of reserved memory there might be no struct page available,
>   * so all that can be done to support such 'pages' is to try to convert
>   * pfn to dma address or at the last resort just assume that
> @@ -499,6 +529,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> @@ -675,10 +709,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  {
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
> +	int ret;
>  
>  	if (dbuf->size < size)
>  		return ERR_PTR(-EFAULT);
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Hi Sakari,


On 2016-04-29 13:21, Sakari Ailus wrote:
> Hi Marek,
>
> Thanks for the patch!
>
> On Thu, Apr 28, 2016 at 03:20:03PM +0200, Marek Szyprowski wrote:
>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>> segment size properly for the client device. Setting it is needed to let
>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>> address space. This is essential for all devices, which use dma-contig
>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>> of operations).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> Hello,
>>
>> This patch is a follow-up of my previous attempts to let Exynos
>> multimedia devices to work properly with shared buffers when IOMMU is
>> enabled:
>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>> 3. https://patchwork.linuxtv.org/patch/30870/
>>
>> As sugested by Hans, configuring DMA max segment size should be done by
>> videobuf2-dma-contig module instead of requiring all device drivers to
>> do it on their own.
>>
>> Here is some backgroud why this is done in videobuf2-dc not in the
>> respective generic bus code:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>
>> Best regards,
>> Marek Szyprowski
>>
>> changelog:
>> v2:
>> - fixes typos and other language issues in the comments
>>
>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 461ae55eaa98..d0382d62954d 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>   }
>>   
>>   /*
>> + * To allow mapping the scatter-list into a single chunk in the DMA
>> + * address space, the device is required to have the DMA max segment
>> + * size parameter set to a value larger than the buffer size. Otherwise,
>> + * the DMA-mapping subsystem will split the mapping into max segment
>> + * size chunks. This function increases the DMA max segment size
>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>> + * address space.
>> + * This code assumes that the DMA-mapping subsystem will merge all
>> + * scatter-list segments if this is really possible (for example when
>> + * an IOMMU is available and enabled).
>> + * Ideally, this parameter should be set by the generic bus code, but it
>> + * is left with the default 64KiB value due to historical litmiations in
>> + * other subsystems (like limited USB host drivers) and there no good
>> + * place to set it to the proper value. It is done here to avoid fixing
>> + * all the vb2-dc client drivers.
>> + */
>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>> +{
>> +	if (!dev->dma_parms) {
>> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);
> Doesn't this create a memory leak? Do consider that devices may be also
> removed from the system at runtime.
>
> Looks very nice otherwise.

Yes it does, but there is completely no way to determine when to do that
(dma_params might have been already allocated by the bus code). The whole
dma_params idea and its handling is a bit messy. IMHO we can leave this
for now until dma_params gets cleaned up (I remember someone said that he
has this on his todo list, but I don't remember now who it was...).

>
>> +		if (!dev->dma_parms)
>> +			return -ENOMEM;
>> +	}
>> +	if (dma_get_max_seg_size(dev) < size)
>> +		return dma_set_max_seg_size(dev, size);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>>    * For some kind of reserved memory there might be no struct page available,
>>    * so all that can be done to support such 'pages' is to try to convert
>>    * pfn to dma address or at the last resort just assume that
>> @@ -499,6 +529,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);
>> @@ -675,10 +709,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   {
>>   	struct vb2_dc_buf *buf;
>>   	struct dma_buf_attachment *dba;
>> +	int ret;
>>   
>>   	if (dbuf->size < size)
>>   		return ERR_PTR(-EFAULT);
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);

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

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Sakari Ailus]

Hi Marek,

On Fri, Apr 29, 2016 at 01:39:43PM +0200, Marek Szyprowski wrote:
> Hi Sakari,
> 
> 
> On 2016-04-29 13:21, Sakari Ailus wrote:
> >Hi Marek,
> >
> >Thanks for the patch!
> >
> >On Thu, Apr 28, 2016 at 03:20:03PM +0200, Marek Szyprowski wrote:
> >>This patch lets vb2-dma-contig memory allocator to configure DMA max
> >>segment size properly for the client device. Setting it is needed to let
> >>DMA-mapping subsystem to create a single, contiguous mapping in DMA
> >>address space. This is essential for all devices, which use dma-contig
> >>videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> >>of operations).
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>---
> >>Hello,
> >>
> >>This patch is a follow-up of my previous attempts to let Exynos
> >>multimedia devices to work properly with shared buffers when IOMMU is
> >>enabled:
> >>1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> >>2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> >>3. https://patchwork.linuxtv.org/patch/30870/
> >>
> >>As sugested by Hans, configuring DMA max segment size should be done by
> >>videobuf2-dma-contig module instead of requiring all device drivers to
> >>do it on their own.
> >>
> >>Here is some backgroud why this is done in videobuf2-dc not in the
> >>respective generic bus code:
> >>http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> >>
> >>Best regards,
> >>Marek Szyprowski
> >>
> >>changelog:
> >>v2:
> >>- fixes typos and other language issues in the comments
> >>
> >>v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> >>---
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
> >>  1 file changed, 39 insertions(+)
> >>
> >>diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>index 461ae55eaa98..d0382d62954d 100644
> >>--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >>@@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
> >>  }
> >>  /*
> >>+ * To allow mapping the scatter-list into a single chunk in the DMA
> >>+ * address space, the device is required to have the DMA max segment
> >>+ * size parameter set to a value larger than the buffer size. Otherwise,
> >>+ * the DMA-mapping subsystem will split the mapping into max segment
> >>+ * size chunks. This function increases the DMA max segment size
> >>+ * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> >>+ * address space.
> >>+ * This code assumes that the DMA-mapping subsystem will merge all
> >>+ * scatter-list segments if this is really possible (for example when
> >>+ * an IOMMU is available and enabled).
> >>+ * Ideally, this parameter should be set by the generic bus code, but it
> >>+ * is left with the default 64KiB value due to historical litmiations in
> >>+ * other subsystems (like limited USB host drivers) and there no good
> >>+ * place to set it to the proper value. It is done here to avoid fixing
> >>+ * all the vb2-dc client drivers.
> >>+ */
> >>+static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> >>+{
> >>+	if (!dev->dma_parms) {
> >>+		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);
> >Doesn't this create a memory leak? Do consider that devices may be also
> >removed from the system at runtime.
> >
> >Looks very nice otherwise.
> 
> Yes it does, but there is completely no way to determine when to do that
> (dma_params might have been already allocated by the bus code). The whole
> dma_params idea and its handling is a bit messy. IMHO we can leave this
> for now until dma_params gets cleaned up (I remember someone said that he
> has this on his todo list, but I don't remember now who it was...).

You could fix this in a V4L2-specific way by storing the information whether
you've allocated the dma-params e.g. in struct video_device. That's probably
not worth the trouble, especially if someone's about to have a better
solution.

I'd add a "FIXME: ..." comment so we won't forget about it.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v2] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

On 04/29/16 15:56, Sakari Ailus wrote:
> Hi Marek,
> 
> On Fri, Apr 29, 2016 at 01:39:43PM +0200, Marek Szyprowski wrote:
>> Hi Sakari,
>>
>>
>> On 2016-04-29 13:21, Sakari Ailus wrote:
>>> Hi Marek,
>>>
>>> Thanks for the patch!
>>>
>>> On Thu, Apr 28, 2016 at 03:20:03PM +0200, Marek Szyprowski wrote:
>>>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>>>> segment size properly for the client device. Setting it is needed to let
>>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>>>> address space. This is essential for all devices, which use dma-contig
>>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>>>> of operations).
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>> Hello,
>>>>
>>>> This patch is a follow-up of my previous attempts to let Exynos
>>>> multimedia devices to work properly with shared buffers when IOMMU is
>>>> enabled:
>>>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>>>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>>>> 3. https://patchwork.linuxtv.org/patch/30870/
>>>>
>>>> As sugested by Hans, configuring DMA max segment size should be done by
>>>> videobuf2-dma-contig module instead of requiring all device drivers to
>>>> do it on their own.
>>>>
>>>> Here is some backgroud why this is done in videobuf2-dc not in the
>>>> respective generic bus code:
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>>>
>>>> Best regards,
>>>> Marek Szyprowski
>>>>
>>>> changelog:
>>>> v2:
>>>> - fixes typos and other language issues in the comments
>>>>
>>>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>>>> ---
>>>>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 39 ++++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> index 461ae55eaa98..d0382d62954d 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> @@ -443,6 +443,36 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>>>  }
>>>>  /*
>>>> + * To allow mapping the scatter-list into a single chunk in the DMA
>>>> + * address space, the device is required to have the DMA max segment
>>>> + * size parameter set to a value larger than the buffer size. Otherwise,
>>>> + * the DMA-mapping subsystem will split the mapping into max segment
>>>> + * size chunks. This function increases the DMA max segment size
>>>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>>>> + * address space.
>>>> + * This code assumes that the DMA-mapping subsystem will merge all
>>>> + * scatter-list segments if this is really possible (for example when
>>>> + * an IOMMU is available and enabled).
>>>> + * Ideally, this parameter should be set by the generic bus code, but it
>>>> + * is left with the default 64KiB value due to historical litmiations in
>>>> + * other subsystems (like limited USB host drivers) and there no good
>>>> + * place to set it to the proper value. It is done here to avoid fixing
>>>> + * all the vb2-dc client drivers.
>>>> + */
>>>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>>>> +{
>>>> +	if (!dev->dma_parms) {
>>>> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);
>>> Doesn't this create a memory leak? Do consider that devices may be also
>>> removed from the system at runtime.
>>>
>>> Looks very nice otherwise.
>>
>> Yes it does, but there is completely no way to determine when to do that
>> (dma_params might have been already allocated by the bus code). The whole
>> dma_params idea and its handling is a bit messy. IMHO we can leave this
>> for now until dma_params gets cleaned up (I remember someone said that he
>> has this on his todo list, but I don't remember now who it was...).
> 
> You could fix this in a V4L2-specific way by storing the information whether
> you've allocated the dma-params e.g. in struct video_device. That's probably
> not worth the trouble, especially if someone's about to have a better
> solution.
> 
> I'd add a "FIXME: ..." comment so we won't forget about it.
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 

I agree with Sakari, please add a FIXME here explaining the issue.
I'll pick up the v3 patch for a pull request on Wednesday.

Regards,

	Hans

[PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

This patch lets vb2-dma-contig memory allocator to configure DMA max
segment size properly for the client device. Setting it is needed to let
DMA-mapping subsystem to create a single, contiguous mapping in DMA
address space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
of operations).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Hello,

This patch is a follow-up of my previous attempts to let Exynos
multimedia devices to work properly with shared buffers when IOMMU is
enabled:
1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
3. https://patchwork.linuxtv.org/patch/30870/

As sugested by Hans, configuring DMA max segment size should be done by
videobuf2-dma-contig module instead of requiring all device drivers to
do it on their own.

Here is some backgroud why this is done in videobuf2-dc not in the
respective generic bus code:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html

Best regards,
Marek Szyprowski

changelog:
v3:
- added FIXME note about possible memory leak

v2:
- fixes typos and other language issues in the comments

v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 461ae55eaa98..2ca7e798f394 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
 }
 
 /*
+ * To allow mapping the scatter-list into a single chunk in the DMA
+ * address space, the device is required to have the DMA max segment
+ * size parameter set to a value larger than the buffer size. Otherwise,
+ * the DMA-mapping subsystem will split the mapping into max segment
+ * size chunks. This function increases the DMA max segment size
+ * parameter to let DMA-mapping map a buffer as a single chunk in DMA
+ * address space.
+ * This code assumes that the DMA-mapping subsystem will merge all
+ * scatterlist segments if this is really possible (for example when
+ * an IOMMU is available and enabled).
+ * Ideally, this parameter should be set by the generic bus code, but it
+ * is left with the default 64KiB value due to historical litmiations in
+ * other subsystems (like limited USB host drivers) and there no good
+ * place to set it to the proper value. It is done here to avoid fixing
+ * all the vb2-dc client drivers.
+ *
+ * FIXME: the allocated dma_params structure is leaked because there
+ * is completely no way to determine when to free it (dma_params might have
+ * been also already allocated by the bus code). However in typical
+ * use cases this function will be called for platform devices, which are
+ * not how-plugged and exist all the time in the target system.
+ */
+static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	if (!dev->dma_parms) {
+		dev->dma_parms = kzalloc(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;
+}
+
+/*
  * For some kind of reserved memory there might be no struct page available,
  * so all that can be done to support such 'pages' is to try to convert
  * pfn to dma address or at the last resort just assume that
@@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
 		return ERR_PTR(-EINVAL);
 	}
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
@@ -675,10 +715,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 {
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
+	int ret;
 
 	if (dbuf->size < size)
 		return ERR_PTR(-EFAULT);
 
+	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
+	if (!ret)
+		return ERR_PTR(ret);
+
 	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
-- 
1.9.2

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Sakari Ailus]

On Mon, May 02, 2016 at 12:59:13PM +0200, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

On 05/02/16 12:59, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> 
> changelog:
> v3:
> - added FIXME note about possible memory leak
> 
> v2:
> - fixes typos and other language issues in the comments
> 
> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 461ae55eaa98..2ca7e798f394 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping the scatter-list into a single chunk in the DMA
> + * address space, the device is required to have the DMA max segment
> + * size parameter set to a value larger than the buffer size. Otherwise,
> + * the DMA-mapping subsystem will split the mapping into max segment
> + * size chunks. This function increases the DMA max segment size
> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> + * address space.
> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatterlist segments if this is really possible (for example when
> + * an IOMMU is available and enabled).
> + * Ideally, this parameter should be set by the generic bus code, but it
> + * is left with the default 64KiB value due to historical litmiations in
> + * other subsystems (like limited USB host drivers) and there no good
> + * place to set it to the proper value. It is done here to avoid fixing
> + * all the vb2-dc client drivers.
> + *
> + * FIXME: the allocated dma_params structure is leaked because there
> + * is completely no way to determine when to free it (dma_params might have
> + * been also already allocated by the bus code). However in typical
> + * use cases this function will be called for platform devices, which are
> + * not how-plugged and exist all the time in the target system.

how-plugged, hmm. I'll change that to hot-plugged before merging :-)

	Hans

> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(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;
> +}
> +
> +/*
>   * For some kind of reserved memory there might be no struct page available,
>   * so all that can be done to support such 'pages' is to try to convert
>   * pfn to dma address or at the last resort just assume that
> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> @@ -675,10 +715,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  {
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
> +	int ret;
>  
>  	if (dbuf->size < size)
>  		return ERR_PTR(-EFAULT);
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

Hi Marek,

On 05/02/2016 12:59 PM, Marek Szyprowski wrote:
> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> 
> changelog:
> v3:
> - added FIXME note about possible memory leak
> 
> v2:
> - fixes typos and other language issues in the comments
> 
> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 461ae55eaa98..2ca7e798f394 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping the scatter-list into a single chunk in the DMA
> + * address space, the device is required to have the DMA max segment
> + * size parameter set to a value larger than the buffer size. Otherwise,
> + * the DMA-mapping subsystem will split the mapping into max segment
> + * size chunks. This function increases the DMA max segment size
> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> + * address space.
> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatterlist segments if this is really possible (for example when
> + * an IOMMU is available and enabled).
> + * Ideally, this parameter should be set by the generic bus code, but it
> + * is left with the default 64KiB value due to historical litmiations in
> + * other subsystems (like limited USB host drivers) and there no good
> + * place to set it to the proper value. It is done here to avoid fixing
> + * all the vb2-dc client drivers.
> + *
> + * FIXME: the allocated dma_params structure is leaked because there
> + * is completely no way to determine when to free it (dma_params might have
> + * been also already allocated by the bus code). However in typical
> + * use cases this function will be called for platform devices, which are
> + * not how-plugged and exist all the time in the target system.
> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(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;
> +}
> +
> +/*
>   * For some kind of reserved memory there might be no struct page available,
>   * so all that can be done to support such 'pages' is to try to convert
>   * pfn to dma address or at the last resort just assume that
> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));

Huh? Against which kernel do you compile? The get_userptr prototype is different
from the latest mainline kernel. Specifically, dev is now conf->dev.

> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);

I'd move the vb2_dc_set_max_seg_size call to after the buf is allocated. Since this call
has side-effects I would only call it when it is really needed.

> @@ -675,10 +715,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  {
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
> +	int ret;
>  
>  	if (dbuf->size < size)
>  		return ERR_PTR(-EFAULT);
>  
> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));

Ditto for argument and moving to after the buf is allocated.

> +	if (!ret)
> +		return ERR_PTR(ret);
> +
>  	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 

Regards,

	Hans

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Hi Hans,


On 2016-05-04 10:22, Hans Verkuil wrote:
> Hi Marek,
>
> On 05/02/2016 12:59 PM, Marek Szyprowski wrote:
>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>> segment size properly for the client device. Setting it is needed to let
>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>> address space. This is essential for all devices, which use dma-contig
>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>> of operations).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> Hello,
>>
>> This patch is a follow-up of my previous attempts to let Exynos
>> multimedia devices to work properly with shared buffers when IOMMU is
>> enabled:
>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>> 3. https://patchwork.linuxtv.org/patch/30870/
>>
>> As sugested by Hans, configuring DMA max segment size should be done by
>> videobuf2-dma-contig module instead of requiring all device drivers to
>> do it on their own.
>>
>> Here is some backgroud why this is done in videobuf2-dc not in the
>> respective generic bus code:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>
>> Best regards,
>> Marek Szyprowski
>>
>> changelog:
>> v3:
>> - added FIXME note about possible memory leak
>>
>> v2:
>> - fixes typos and other language issues in the comments
>>
>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 461ae55eaa98..2ca7e798f394 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>   }
>>   
>>   /*
>> + * To allow mapping the scatter-list into a single chunk in the DMA
>> + * address space, the device is required to have the DMA max segment
>> + * size parameter set to a value larger than the buffer size. Otherwise,
>> + * the DMA-mapping subsystem will split the mapping into max segment
>> + * size chunks. This function increases the DMA max segment size
>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>> + * address space.
>> + * This code assumes that the DMA-mapping subsystem will merge all
>> + * scatterlist segments if this is really possible (for example when
>> + * an IOMMU is available and enabled).
>> + * Ideally, this parameter should be set by the generic bus code, but it
>> + * is left with the default 64KiB value due to historical litmiations in
>> + * other subsystems (like limited USB host drivers) and there no good
>> + * place to set it to the proper value. It is done here to avoid fixing
>> + * all the vb2-dc client drivers.
>> + *
>> + * FIXME: the allocated dma_params structure is leaked because there
>> + * is completely no way to determine when to free it (dma_params might have
>> + * been also already allocated by the bus code). However in typical
>> + * use cases this function will be called for platform devices, which are
>> + * not how-plugged and exist all the time in the target system.
>> + */
>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>> +{
>> +	if (!dev->dma_parms) {
>> +		dev->dma_parms = kzalloc(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;
>> +}
>> +
>> +/*
>>    * For some kind of reserved memory there might be no struct page available,
>>    * so all that can be done to support such 'pages' is to try to convert
>>    * pfn to dma address or at the last resort just assume that
>> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
> Huh? Against which kernel do you compile? The get_userptr prototype is different
> from the latest mainline kernel. Specifically, dev is now conf->dev.

I prepared it on top of your 'context3' branch, as you requested not to 
use the
allocator context related functions, which best suit for this purpose.

>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);
> I'd move the vb2_dc_set_max_seg_size call to after the buf is allocated. Since this call
> has side-effects I would only call it when it is really needed.

OKay.

>
>> @@ -675,10 +715,15 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>>   {
>>   	struct vb2_dc_buf *buf;
>>   	struct dma_buf_attachment *dba;
>> +	int ret;
>>   
>>   	if (dbuf->size < size)
>>   		return ERR_PTR(-EFAULT);
>>   
>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size));
> Ditto for argument and moving to after the buf is allocated.
>
>> +	if (!ret)
>> +		return ERR_PTR(ret);
>> +
>>   	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>>   	if (!buf)
>>   		return ERR_PTR(-ENOMEM);
>>

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

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Hans Verkuil]

Hi Marek,

On 05/04/2016 10:28 AM, Marek Szyprowski wrote:
> Hi Hans,
> 
> 
> On 2016-05-04 10:22, Hans Verkuil wrote:
>> Hi Marek,
>>
>> On 05/02/2016 12:59 PM, Marek Szyprowski wrote:
>>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>>> segment size properly for the client device. Setting it is needed to let
>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>>> address space. This is essential for all devices, which use dma-contig
>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>>> of operations).
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>> Hello,
>>>
>>> This patch is a follow-up of my previous attempts to let Exynos
>>> multimedia devices to work properly with shared buffers when IOMMU is
>>> enabled:
>>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>>> 3. https://patchwork.linuxtv.org/patch/30870/
>>>
>>> As sugested by Hans, configuring DMA max segment size should be done by
>>> videobuf2-dma-contig module instead of requiring all device drivers to
>>> do it on their own.
>>>
>>> Here is some backgroud why this is done in videobuf2-dc not in the
>>> respective generic bus code:
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>>
>>> Best regards,
>>> Marek Szyprowski
>>>
>>> changelog:
>>> v3:
>>> - added FIXME note about possible memory leak
>>>
>>> v2:
>>> - fixes typos and other language issues in the comments
>>>
>>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>>> ---
>>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
>>>   1 file changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> index 461ae55eaa98..2ca7e798f394 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>>   }
>>>   
>>>   /*
>>> + * To allow mapping the scatter-list into a single chunk in the DMA
>>> + * address space, the device is required to have the DMA max segment
>>> + * size parameter set to a value larger than the buffer size. Otherwise,
>>> + * the DMA-mapping subsystem will split the mapping into max segment
>>> + * size chunks. This function increases the DMA max segment size
>>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>>> + * address space.
>>> + * This code assumes that the DMA-mapping subsystem will merge all
>>> + * scatterlist segments if this is really possible (for example when
>>> + * an IOMMU is available and enabled).
>>> + * Ideally, this parameter should be set by the generic bus code, but it
>>> + * is left with the default 64KiB value due to historical litmiations in
>>> + * other subsystems (like limited USB host drivers) and there no good
>>> + * place to set it to the proper value. It is done here to avoid fixing
>>> + * all the vb2-dc client drivers.
>>> + *
>>> + * FIXME: the allocated dma_params structure is leaked because there
>>> + * is completely no way to determine when to free it (dma_params might have
>>> + * been also already allocated by the bus code). However in typical
>>> + * use cases this function will be called for platform devices, which are
>>> + * not how-plugged and exist all the time in the target system.
>>> + */
>>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>>> +{
>>> +	if (!dev->dma_parms) {
>>> +		dev->dma_parms = kzalloc(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;
>>> +}
>>> +
>>> +/*
>>>    * For some kind of reserved memory there might be no struct page available,
>>>    * so all that can be done to support such 'pages' is to try to convert
>>>    * pfn to dma address or at the last resort just assume that
>>> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>>>   		return ERR_PTR(-EINVAL);
>>>   	}
>>>   
>>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
>> Huh? Against which kernel do you compile? The get_userptr prototype is different
>> from the latest mainline kernel. Specifically, dev is now conf->dev.
> 
> I prepared it on top of your 'context3' branch, as you requested not to 
> use the
> allocator context related functions, which best suit for this purpose.

That's not quite what I meant, sorry for the confusion. My reference to the
context3 branch was just that: to show upcoming changes and why it was not a
good idea to call this function from the context allocate/free functions. Since
those will disappear.

The context3 branch isn't for 4.7 (too late for that), but I want to get it
in early in the 4.8 cycle.

So just base this patch on the latest media_tree master.

Regards,

	Hans

Re: [PATCH v3] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Hi Hans


On 2016-05-04 10:32, Hans Verkuil wrote:
> Hi Marek,
>
> On 05/04/2016 10:28 AM, Marek Szyprowski wrote:
>> Hi Hans,
>>
>>
>> On 2016-05-04 10:22, Hans Verkuil wrote:
>>> Hi Marek,
>>>
>>> On 05/02/2016 12:59 PM, Marek Szyprowski wrote:
>>>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>>>> segment size properly for the client device. Setting it is needed to let
>>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>>>> address space. This is essential for all devices, which use dma-contig
>>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>>>> of operations).
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>> Hello,
>>>>
>>>> This patch is a follow-up of my previous attempts to let Exynos
>>>> multimedia devices to work properly with shared buffers when IOMMU is
>>>> enabled:
>>>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>>>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>>>> 3. https://patchwork.linuxtv.org/patch/30870/
>>>>
>>>> As sugested by Hans, configuring DMA max segment size should be done by
>>>> videobuf2-dma-contig module instead of requiring all device drivers to
>>>> do it on their own.
>>>>
>>>> Here is some backgroud why this is done in videobuf2-dc not in the
>>>> respective generic bus code:
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>>>
>>>> Best regards,
>>>> Marek Szyprowski
>>>>
>>>> changelog:
>>>> v3:
>>>> - added FIXME note about possible memory leak
>>>>
>>>> v2:
>>>> - fixes typos and other language issues in the comments
>>>>
>>>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>>>> ---
>>>>    drivers/media/v4l2-core/videobuf2-dma-contig.c | 45 ++++++++++++++++++++++++++
>>>>    1 file changed, 45 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> index 461ae55eaa98..2ca7e798f394 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> @@ -443,6 +443,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>>>    }
>>>>    
>>>>    /*
>>>> + * To allow mapping the scatter-list into a single chunk in the DMA
>>>> + * address space, the device is required to have the DMA max segment
>>>> + * size parameter set to a value larger than the buffer size. Otherwise,
>>>> + * the DMA-mapping subsystem will split the mapping into max segment
>>>> + * size chunks. This function increases the DMA max segment size
>>>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>>>> + * address space.
>>>> + * This code assumes that the DMA-mapping subsystem will merge all
>>>> + * scatterlist segments if this is really possible (for example when
>>>> + * an IOMMU is available and enabled).
>>>> + * Ideally, this parameter should be set by the generic bus code, but it
>>>> + * is left with the default 64KiB value due to historical litmiations in
>>>> + * other subsystems (like limited USB host drivers) and there no good
>>>> + * place to set it to the proper value. It is done here to avoid fixing
>>>> + * all the vb2-dc client drivers.
>>>> + *
>>>> + * FIXME: the allocated dma_params structure is leaked because there
>>>> + * is completely no way to determine when to free it (dma_params might have
>>>> + * been also already allocated by the bus code). However in typical
>>>> + * use cases this function will be called for platform devices, which are
>>>> + * not how-plugged and exist all the time in the target system.
>>>> + */
>>>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>>>> +{
>>>> +	if (!dev->dma_parms) {
>>>> +		dev->dma_parms = kzalloc(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;
>>>> +}
>>>> +
>>>> +/*
>>>>     * For some kind of reserved memory there might be no struct page available,
>>>>     * so all that can be done to support such 'pages' is to try to convert
>>>>     * pfn to dma address or at the last resort just assume that
>>>> @@ -499,6 +535,10 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
>>>>    		return ERR_PTR(-EINVAL);
>>>>    	}
>>>>    
>>>> +	ret = vb2_dc_set_max_seg_size(dev, PAGE_ALIGN(size + PAGE_SIZE));
>>> Huh? Against which kernel do you compile? The get_userptr prototype is different
>>> from the latest mainline kernel. Specifically, dev is now conf->dev.
>> I prepared it on top of your 'context3' branch, as you requested not to
>> use the
>> allocator context related functions, which best suit for this purpose.
> That's not quite what I meant, sorry for the confusion. My reference to the
> context3 branch was just that: to show upcoming changes and why it was not a
> good idea to call this function from the context allocate/free functions. Since
> those will disappear.
>
> The context3 branch isn't for 4.7 (too late for that), but I want to get it
> in early in the 4.8 cycle.
>
> So just base this patch on the latest media_tree master.

I will send a version based on media tree in a few minutes.

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

[PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

This patch lets vb2-dma-contig memory allocator to configure DMA max
segment size properly for the client device. Setting it is needed to let
DMA-mapping subsystem to create a single, contiguous mapping in DMA
address space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
of operations).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hello,

This patch is a follow-up of my previous attempts to let Exynos
multimedia devices to work properly with shared buffers when IOMMU is
enabled:
1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
3. https://patchwork.linuxtv.org/patch/30870/

As sugested by Hans, configuring DMA max segment size should be done by
videobuf2-dma-contig module instead of requiring all device drivers to
do it on their own.

Here is some backgroud why this is done in videobuf2-dc not in the
respective generic bus code:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html

Best regards,
Marek Szyprowski

changelog:
v4:
- rebased onto media master tree
- call vb2_dc_set_max_seg_size after allocating vb2 buf object

v3:
- added FIXME note about possible memory leak

v2:
- fixes typos and other language issues in the comments

v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 53 +++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5361197f3e57..6291842a889f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -448,6 +448,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
 }
 
 /*
+ * To allow mapping the scatter-list into a single chunk in the DMA
+ * address space, the device is required to have the DMA max segment
+ * size parameter set to a value larger than the buffer size. Otherwise,
+ * the DMA-mapping subsystem will split the mapping into max segment
+ * size chunks. This function increases the DMA max segment size
+ * parameter to let DMA-mapping map a buffer as a single chunk in DMA
+ * address space.
+ * This code assumes that the DMA-mapping subsystem will merge all
+ * scatterlist segments if this is really possible (for example when
+ * an IOMMU is available and enabled).
+ * Ideally, this parameter should be set by the generic bus code, but it
+ * is left with the default 64KiB value due to historical litmiations in
+ * other subsystems (like limited USB host drivers) and there no good
+ * place to set it to the proper value. It is done here to avoid fixing
+ * all the vb2-dc client drivers.
+ *
+ * FIXME: the allocated dma_params structure is leaked because there
+ * is completely no way to determine when to free it (dma_params might have
+ * been also already allocated by the bus code). However in typical
+ * use cases this function will be called for platform devices, which are
+ * not hot-plugged and exist all the time in the target system.
+ */
+static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	if (!dev->dma_parms) {
+		dev->dma_parms = kzalloc(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;
+}
+
+/*
  * For some kind of reserved memory there might be no struct page available,
  * so all that can be done to support such 'pages' is to try to convert
  * pfn to dma address or at the last resort just assume that
@@ -509,6 +545,10 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
+	ret = vb2_dc_set_max_seg_size(conf->dev, PAGE_ALIGN(size + PAGE_SIZE));
+	if (!ret)
+		goto fail_buf;
+
 	buf->dev = conf->dev;
 	buf->dma_dir = dma_dir;
 
@@ -682,6 +722,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
 	struct vb2_dc_conf *conf = alloc_ctx;
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
+	int ret;
 
 	if (dbuf->size < size)
 		return ERR_PTR(-EFAULT);
@@ -690,13 +731,17 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
+	ret = vb2_dc_set_max_seg_size(conf->dev, PAGE_ALIGN(size));
+	if (!ret)
+		goto fail_buf;
+
 	buf->dev = conf->dev;
 	/* create attachment for the dmabuf with the user device */
 	dba = dma_buf_attach(dbuf, buf->dev);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
-		kfree(buf);
-		return dba;
+		ret = PTR_ERR(dba);
+		goto fail_buf;
 	}
 
 	buf->dma_dir = dma_dir;
@@ -704,6 +749,10 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
 	buf->db_attach = dba;
 
 	return buf;
+
+fail_buf:
+	kfree(buf);
+	return ERR_PTR(ret);
 }
 
 /*********************************************/
-- 
1.9.2

Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly

[Mauro Carvalho Chehab]

Em Wed, 04 May 2016 11:00:03 +0200
Marek Szyprowski <m.szyprowski@samsung.com> escreveu:

> This patch lets vb2-dma-contig memory allocator to configure DMA max
> segment size properly for the client device. Setting it is needed to let
> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> address space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> of operations).
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hello,
> 
> This patch is a follow-up of my previous attempts to let Exynos
> multimedia devices to work properly with shared buffers when IOMMU is
> enabled:
> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> 3. https://patchwork.linuxtv.org/patch/30870/
> 
> As sugested by Hans, configuring DMA max segment size should be done by
> videobuf2-dma-contig module instead of requiring all device drivers to
> do it on their own.
> 
> Here is some backgroud why this is done in videobuf2-dc not in the
> respective generic bus code:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> 
> Best regards,
> Marek Szyprowski
> 
> changelog:
> v4:
> - rebased onto media master tree
> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
> 
> v3:
> - added FIXME note about possible memory leak
> 
> v2:
> - fixes typos and other language issues in the comments
> 
> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 53 +++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5361197f3e57..6291842a889f 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -448,6 +448,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>  }
>  
>  /*
> + * To allow mapping the scatter-list into a single chunk in the DMA
> + * address space, the device is required to have the DMA max segment
> + * size parameter set to a value larger than the buffer size. Otherwise,
> + * the DMA-mapping subsystem will split the mapping into max segment
> + * size chunks. This function increases the DMA max segment size
> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> + * address space.
> + * This code assumes that the DMA-mapping subsystem will merge all
> + * scatterlist segments if this is really possible (for example when
> + * an IOMMU is available and enabled).
> + * Ideally, this parameter should be set by the generic bus code, but it
> + * is left with the default 64KiB value due to historical litmiations in
> + * other subsystems (like limited USB host drivers) and there no good
> + * place to set it to the proper value. It is done here to avoid fixing
> + * all the vb2-dc client drivers.
> + *
> + * FIXME: the allocated dma_params structure is leaked because there
> + * is completely no way to determine when to free it (dma_params might have
> + * been also already allocated by the bus code). However in typical
> + * use cases this function will be called for platform devices, which are
> + * not hot-plugged and exist all the time in the target system.
> + */
> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> +{
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);

Why don't you use devm_kzalloc() here? dma_parms will then be freed
if the device gets hot-unplugged/unbind.

And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
device via sysfs.

Just assuming that only platform drivers will use dma-contig and adding
a memory leak here seems really ugly!

Regards,
Mauro
-- 
Thanks,
Mauro

Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Hi Mauro


On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
> Em Wed, 04 May 2016 11:00:03 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> escreveu:
>
>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>> segment size properly for the client device. Setting it is needed to let
>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>> address space. This is essential for all devices, which use dma-contig
>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>> of operations).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>> Hello,
>>
>> This patch is a follow-up of my previous attempts to let Exynos
>> multimedia devices to work properly with shared buffers when IOMMU is
>> enabled:
>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>> 3. https://patchwork.linuxtv.org/patch/30870/
>>
>> As sugested by Hans, configuring DMA max segment size should be done by
>> videobuf2-dma-contig module instead of requiring all device drivers to
>> do it on their own.
>>
>> Here is some backgroud why this is done in videobuf2-dc not in the
>> respective generic bus code:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>
>> Best regards,
>> Marek Szyprowski
>>
>> changelog:
>> v4:
>> - rebased onto media master tree
>> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
>>
>> v3:
>> - added FIXME note about possible memory leak
>>
>> v2:
>> - fixes typos and other language issues in the comments
>>
>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 53 +++++++++++++++++++++++++-
>>   1 file changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> index 5361197f3e57..6291842a889f 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -448,6 +448,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>   }
>>   
>>   /*
>> + * To allow mapping the scatter-list into a single chunk in the DMA
>> + * address space, the device is required to have the DMA max segment
>> + * size parameter set to a value larger than the buffer size. Otherwise,
>> + * the DMA-mapping subsystem will split the mapping into max segment
>> + * size chunks. This function increases the DMA max segment size
>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>> + * address space.
>> + * This code assumes that the DMA-mapping subsystem will merge all
>> + * scatterlist segments if this is really possible (for example when
>> + * an IOMMU is available and enabled).
>> + * Ideally, this parameter should be set by the generic bus code, but it
>> + * is left with the default 64KiB value due to historical litmiations in
>> + * other subsystems (like limited USB host drivers) and there no good
>> + * place to set it to the proper value. It is done here to avoid fixing
>> + * all the vb2-dc client drivers.
>> + *
>> + * FIXME: the allocated dma_params structure is leaked because there
>> + * is completely no way to determine when to free it (dma_params might have
>> + * been also already allocated by the bus code). However in typical
>> + * use cases this function will be called for platform devices, which are
>> + * not hot-plugged and exist all the time in the target system.
>> + */
>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>> +{
>> +	if (!dev->dma_parms) {
>> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);
> Why don't you use devm_kzalloc() here? dma_parms will then be freed
> if the device gets hot-unplugged/unbind.

Although the structure will be freed, but the pointer in the struct device
will still point to the freed resource. Please note that devm_ allocations
are freed on driver removal/unbind not on device removal. That's why I 
decided
to leak memory instead of allowing to access resource that has been freed.

> And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
> device via sysfs.
>
> Just assuming that only platform drivers will use dma-contig and adding
> a memory leak here seems really ugly!

The whole handling of dma_params structure is really ugly and right now 
there
is no good way of ensuring proper dma parameters.

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

Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly

[Mauro Carvalho Chehab]

Em Mon, 09 May 2016 08:13:06 +0200
Marek Szyprowski <m.szyprowski@samsung.com> escreveu:

> Hi Mauro
> 
> 
> On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
> > Em Wed, 04 May 2016 11:00:03 +0200
> > Marek Szyprowski <m.szyprowski@samsung.com> escreveu:
> >  
> >> This patch lets vb2-dma-contig memory allocator to configure DMA max
> >> segment size properly for the client device. Setting it is needed to let
> >> DMA-mapping subsystem to create a single, contiguous mapping in DMA
> >> address space. This is essential for all devices, which use dma-contig
> >> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
> >> of operations).
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> Hello,
> >>
> >> This patch is a follow-up of my previous attempts to let Exynos
> >> multimedia devices to work properly with shared buffers when IOMMU is
> >> enabled:
> >> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
> >> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
> >> 3. https://patchwork.linuxtv.org/patch/30870/
> >>
> >> As sugested by Hans, configuring DMA max segment size should be done by
> >> videobuf2-dma-contig module instead of requiring all device drivers to
> >> do it on their own.
> >>
> >> Here is some backgroud why this is done in videobuf2-dc not in the
> >> respective generic bus code:
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
> >>
> >> Best regards,
> >> Marek Szyprowski
> >>
> >> changelog:
> >> v4:
> >> - rebased onto media master tree
> >> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
> >>
> >> v3:
> >> - added FIXME note about possible memory leak
> >>
> >> v2:
> >> - fixes typos and other language issues in the comments
> >>
> >> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
> >> ---
> >>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 53 +++++++++++++++++++++++++-
> >>   1 file changed, 51 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> index 5361197f3e57..6291842a889f 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -448,6 +448,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
> >>   }
> >>   
> >>   /*
> >> + * To allow mapping the scatter-list into a single chunk in the DMA
> >> + * address space, the device is required to have the DMA max segment
> >> + * size parameter set to a value larger than the buffer size. Otherwise,
> >> + * the DMA-mapping subsystem will split the mapping into max segment
> >> + * size chunks. This function increases the DMA max segment size
> >> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
> >> + * address space.
> >> + * This code assumes that the DMA-mapping subsystem will merge all
> >> + * scatterlist segments if this is really possible (for example when
> >> + * an IOMMU is available and enabled).
> >> + * Ideally, this parameter should be set by the generic bus code, but it
> >> + * is left with the default 64KiB value due to historical litmiations in
> >> + * other subsystems (like limited USB host drivers) and there no good
> >> + * place to set it to the proper value. It is done here to avoid fixing
> >> + * all the vb2-dc client drivers.
> >> + *
> >> + * FIXME: the allocated dma_params structure is leaked because there
> >> + * is completely no way to determine when to free it (dma_params might have
> >> + * been also already allocated by the bus code). However in typical
> >> + * use cases this function will be called for platform devices, which are
> >> + * not hot-plugged and exist all the time in the target system.
> >> + */
> >> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
> >> +{
> >> +	if (!dev->dma_parms) {
> >> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);  
> > Why don't you use devm_kzalloc() here? dma_parms will then be freed
> > if the device gets hot-unplugged/unbind.  
> 
> Although the structure will be freed, but the pointer in the struct device
> will still point to the freed resource. 

Then you'll need some other logic (maybe a kref?) both free it and zero
the pointer when it is safe.

Btw, the only two drivers that seem to dynamically allocate it are
using devm_*:

drivers/gpu/drm/exynos/exynos_drm_iommu.c:      subdrv_dev->dma_parms = devm_kzalloc(subdrv_dev,
drivers/gpu/drm/exynos/exynos_drm_iommu.c:                                      sizeof(*subdrv_dev->dma_parms),
drivers/gpu/drm/exynos/exynos_drm_iommu.c:      if (!subdrv_dev->dma_parms)
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    if (!dev->dma_parms) {

On the other drivers, the struct is embed on some other struct
that is freed only after the need of dma_parms.

> Please note that devm_ allocations
> are freed on driver removal/unbind not on device removal.

Yes, I know. Personally, I don't like devm_ allocations due to that.
Yet, it is better to free it later than to keep it leaking.

> That's why I 
> decided
> to leak memory instead of allowing to access resource that has been freed.

The problem is that, if a non-platform driver needs to use DMA-CONTIG,
the leak can be harmful, as things like qemu can unbind the host
driver in order to use the device by the guest.

Btw, currently, 3 non-platform drivers already use it:

	drivers/media/pci/dt3155/Kconfig:	select VIDEOBUF2_DMA_CONTIG
	drivers/media/pci/solo6x10/Kconfig:	select VIDEOBUF2_DMA_CONTIG
	drivers/media/pci/sta2x11/Kconfig:	select VIDEOBUF2_DMA_CONTIG

(and ideally ivtv and cx18 should be converted to VB2 dma-contig some day)

Btw, as the above PCI drivers are not for ARM, I'm wandering if the
assumptions you took on your patch are also valid on x86 and other 
architectures that support the PCI bus. In any case, if you want to
do it generic, for all drivers, all bus controllers, you'll need to
do lots of tests with non-ARM devices, to be 100% sure that it won't
cause regressions.

On a quick inspection, though, I suspect it will be doing the wrong
thing for the above drivers, as drivers/pci/probe.c already sets
dma_parms during pci_device_add(), and enforces a max segment size of
64K, by calling	
	pci_set_dma_max_seg_size(dev, 65536);
	(with is a wrapper to dma_set_max_seg_size)

> > And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
> > device via sysfs.
> >
> > Just assuming that only platform drivers will use dma-contig and adding
> > a memory leak here seems really ugly!  
> 
> The whole handling of dma_params structure is really ugly and right now 
> there
> is no good way of ensuring proper dma parameters.

Yeah, agreed.

Well, there are some alternatives (ordered from the better to the
worse):

1) Fix the dma_parms core support;
2) Add a pair of functions to register/unregister dma_parms at
   include/media/videobuf2-dma-contig.h. The driver needing to set
   it should call the unregister function if it gets unbind/removed;
3) Add kref somewhere to do the dma_parms de-allocation in a sane
   way.
4) do any dma_parms-related code inside the drivers, and not at
   the core (probably not the best idea, but at least we confine
   the troubles);

I think I would go for (2) for now, and try to do (1) for long
run.

Regards,
Mauro

Re: [PATCH v4] media: vb2-dma-contig: configure DMA max segment size properly

[Marek Szyprowski]

Dear Mauro,


On 2016-05-09 12:09, Mauro Carvalho Chehab wrote:
> Em Mon, 09 May 2016 08:13:06 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> escreveu:
>
>> Hi Mauro
>>
>>
>> On 2016-05-06 20:52, Mauro Carvalho Chehab wrote:
>>> Em Wed, 04 May 2016 11:00:03 +0200
>>> Marek Szyprowski <m.szyprowski@samsung.com> escreveu:
>>>   
>>>> This patch lets vb2-dma-contig memory allocator to configure DMA max
>>>> segment size properly for the client device. Setting it is needed to let
>>>> DMA-mapping subsystem to create a single, contiguous mapping in DMA
>>>> address space. This is essential for all devices, which use dma-contig
>>>> videobuf2 memory allocator and shared buffers (in USERPTR or DMAbuf modes
>>>> of operations).
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>> Hello,
>>>>
>>>> This patch is a follow-up of my previous attempts to let Exynos
>>>> multimedia devices to work properly with shared buffers when IOMMU is
>>>> enabled:
>>>> 1. https://www.mail-archive.com/linux-media@vger.kernel.org/msg96946.html
>>>> 2. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/97316
>>>> 3. https://patchwork.linuxtv.org/patch/30870/
>>>>
>>>> As sugested by Hans, configuring DMA max segment size should be done by
>>>> videobuf2-dma-contig module instead of requiring all device drivers to
>>>> do it on their own.
>>>>
>>>> Here is some backgroud why this is done in videobuf2-dc not in the
>>>> respective generic bus code:
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html
>>>>
>>>> Best regards,
>>>> Marek Szyprowski
>>>>
>>>> changelog:
>>>> v4:
>>>> - rebased onto media master tree
>>>> - call vb2_dc_set_max_seg_size after allocating vb2 buf object
>>>>
>>>> v3:
>>>> - added FIXME note about possible memory leak
>>>>
>>>> v2:
>>>> - fixes typos and other language issues in the comments
>>>>
>>>> v1: http://article.gmane.org/gmane.linux.kernel.samsung-soc/53690
>>>> ---
>>>>    drivers/media/v4l2-core/videobuf2-dma-contig.c | 53 +++++++++++++++++++++++++-
>>>>    1 file changed, 51 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> index 5361197f3e57..6291842a889f 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>>>> @@ -448,6 +448,42 @@ static void vb2_dc_put_userptr(void *buf_priv)
>>>>    }
>>>>    
>>>>    /*
>>>> + * To allow mapping the scatter-list into a single chunk in the DMA
>>>> + * address space, the device is required to have the DMA max segment
>>>> + * size parameter set to a value larger than the buffer size. Otherwise,
>>>> + * the DMA-mapping subsystem will split the mapping into max segment
>>>> + * size chunks. This function increases the DMA max segment size
>>>> + * parameter to let DMA-mapping map a buffer as a single chunk in DMA
>>>> + * address space.
>>>> + * This code assumes that the DMA-mapping subsystem will merge all
>>>> + * scatterlist segments if this is really possible (for example when
>>>> + * an IOMMU is available and enabled).
>>>> + * Ideally, this parameter should be set by the generic bus code, but it
>>>> + * is left with the default 64KiB value due to historical litmiations in
>>>> + * other subsystems (like limited USB host drivers) and there no good
>>>> + * place to set it to the proper value. It is done here to avoid fixing
>>>> + * all the vb2-dc client drivers.
>>>> + *
>>>> + * FIXME: the allocated dma_params structure is leaked because there
>>>> + * is completely no way to determine when to free it (dma_params might have
>>>> + * been also already allocated by the bus code). However in typical
>>>> + * use cases this function will be called for platform devices, which are
>>>> + * not hot-plugged and exist all the time in the target system.
>>>> + */
>>>> +static int vb2_dc_set_max_seg_size(struct device *dev, unsigned int size)
>>>> +{
>>>> +	if (!dev->dma_parms) {
>>>> +		dev->dma_parms = kzalloc(sizeof(dev->dma_parms), GFP_KERNEL);
>>> Why don't you use devm_kzalloc() here? dma_parms will then be freed
>>> if the device gets hot-unplugged/unbind.
>> Although the structure will be freed, but the pointer in the struct device
>> will still point to the freed resource.
> Then you'll need some other logic (maybe a kref?) both free it and zero
> the pointer when it is safe.
>
> Btw, the only two drivers that seem to dynamically allocate it are
> using devm_*:
>
> drivers/gpu/drm/exynos/exynos_drm_iommu.c:      subdrv_dev->dma_parms = devm_kzalloc(subdrv_dev,
> drivers/gpu/drm/exynos/exynos_drm_iommu.c:                                      sizeof(*subdrv_dev->dma_parms),
> drivers/gpu/drm/exynos/exynos_drm_iommu.c:      if (!subdrv_dev->dma_parms)
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    if (!dev->dma_parms) {
>
> On the other drivers, the struct is embed on some other struct
> that is freed only after the need of dma_parms.
>
>> Please note that devm_ allocations
>> are freed on driver removal/unbind not on device removal.
> Yes, I know. Personally, I don't like devm_ allocations due to that.
> Yet, it is better to free it later than to keep it leaking.
>
>> That's why I
>> decided
>> to leak memory instead of allowing to access resource that has been freed.
> The problem is that, if a non-platform driver needs to use DMA-CONTIG,
> the leak can be harmful, as things like qemu can unbind the host
> driver in order to use the device by the guest.
>
> Btw, currently, 3 non-platform drivers already use it:
>
> 	drivers/media/pci/dt3155/Kconfig:	select VIDEOBUF2_DMA_CONTIG
> 	drivers/media/pci/solo6x10/Kconfig:	select VIDEOBUF2_DMA_CONTIG
> 	drivers/media/pci/sta2x11/Kconfig:	select VIDEOBUF2_DMA_CONTIG
>
> (and ideally ivtv and cx18 should be converted to VB2 dma-contig some day)
>
> Btw, as the above PCI drivers are not for ARM, I'm wandering if the
> assumptions you took on your patch are also valid on x86 and other
> architectures that support the PCI bus. In any case, if you want to
> do it generic, for all drivers, all bus controllers, you'll need to
> do lots of tests with non-ARM devices, to be 100% sure that it won't
> cause regressions.

Well, the only case when vb2-dma-contig really 'works' is a hacked solution
where userspace provides USERPTR buffer, which is in fact mmaped contig
buffer from the other device. Also DMABUF made of contig buffer should work,
assuming that its exporter won't split it into several chunks. Anything else
will fail, regardless of the arch/platform and the presence of the IOMMU.
Especially the PCI drivers will not support USERPTR/DMABUF modes properly
due to limited dma_max_seg_size property. This is a historical mess related
to scatterlist code, which we cannot do much to improve.

On the other side there are DRM drivers which 'creatively' use scatterlists
by ignoring dma_params limits. In case of vb2-dma-contig this approach 
cannot
be applied, because it relies on dma-mapping subsystem, which always obeys
the limits.

Probably the best would be to leave existing scatterlist API with its
limitations for storage/network drivers and develop something more suitable
for mapping a whole buffer to DMA address space and it for media and drm
(something like dma_map_page_array() or dma_map_pfn_array() will be much
more suitable that using scatter list, which is designed for describing
a 'buffer' with sub-page resolution, typical in case of storage and network
devices).

> On a quick inspection, though, I suspect it will be doing the wrong
> thing for the above drivers, as drivers/pci/probe.c already sets
> dma_parms during pci_device_add(), and enforces a max segment size of
> 64K, by calling	
> 	pci_set_dma_max_seg_size(dev, 65536);
> 	(with is a wrapper to dma_set_max_seg_size)
>
>>> And yes: it is possible to hot-unplug (actually, hot-unbind) a platform
>>> device via sysfs.
>>>
>>> Just assuming that only platform drivers will use dma-contig and adding
>>> a memory leak here seems really ugly!
>> The whole handling of dma_params structure is really ugly and right now
>> there
>> is no good way of ensuring proper dma parameters.
> Yeah, agreed.
>
> Well, there are some alternatives (ordered from the better to the
> worse):
>
> 1) Fix the dma_parms core support;
> 2) Add a pair of functions to register/unregister dma_parms at
>     include/media/videobuf2-dma-contig.h. The driver needing to set
>     it should call the unregister function if it gets unbind/removed;
> 3) Add kref somewhere to do the dma_parms de-allocation in a sane
>     way.
> 4) do any dma_parms-related code inside the drivers, and not at
>     the core (probably not the best idea, but at least we confine
>     the troubles);
>
> I think I would go for (2) for now, and try to do (1) for long
> run.

I've tried with (2) approach, but I was instructed by Hans to do it in the
vb2-core. I can get back to such approach if you want.

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