[PATCH] media: videobuf2-dc: set properly dma_max_segment_size

[PATCH] media: videobuf2-dc: set properly dma_max_segment_size

[Marek Szyprowski]

If device has no DMA max_seg_size set, we assume that there is no limit
and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
let DMA-mapping API always create contiguous mappings in DMA address
space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 644dec73d220..9d7c1814b0f3 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -862,6 +862,23 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
 void *vb2_dma_contig_init_ctx(struct device *dev)
 {
 	struct vb2_dc_conf *conf;
+	int err;
+
+	/*
+	 * if device has no max_seg_size set, we assume that there is no limit
+	 * and force it to DMA_BIT_MASK(32) to always use contiguous mappings
+	 * in DMA address space
+	 */
+	if (!dev->dma_parms) {
+		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
+		if (!dev->dma_parms)
+			return ERR_PTR(-ENOMEM);
+	}
+	if (dma_get_max_seg_size(dev) < DMA_BIT_MASK(32)) {
+		err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
+		if (err)
+			return ERR_PTR(err);
+	}
 
 	conf = kzalloc(sizeof *conf, GFP_KERNEL);
 	if (!conf)
-- 
1.9.2

Re: [PATCH] media: videobuf2-dc: set properly dma_max_segment_size

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Monday 01 June 2015 14:14:17 Marek Szyprowski wrote:
> If device has no DMA max_seg_size set, we assume that there is no limit
> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
> let DMA-mapping API always create contiguous mappings in DMA address
> space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index
> 644dec73d220..9d7c1814b0f3 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -862,6 +862,23 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>  void *vb2_dma_contig_init_ctx(struct device *dev)
>  {
>  	struct vb2_dc_conf *conf;
> +	int err;
> +
> +	/*
> +	 * if device has no max_seg_size set, we assume that there is no limit
> +	 * and force it to DMA_BIT_MASK(32) to always use contiguous mappings
> +	 * in DMA address space
> +	 */
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);

I was checking how dma_parms was usually allocated and freed, and was shocked 
to find that the memory is never freed. OK, actually not shocked, I had a bad 
feeling about it already, but it's still not good :-/

This goes beyond the scope of this patch, but I think we need to clean up 
dma_parms. The structure is 8 bytes long on 32-bit systems and 16 bytes long 
on 64-bit systems. I wonder if it's really worth it to allocate it separately 
from struct device. It might if we moved more DMA-related fields to struct 
device_dma_parameters but that hasn't happened since 2008 when the structure 
was introduced (yes that's more than 7 years ago).

If we consider it's worth it (and I believe Josh Triplett might, in the 
context of the Linux kernel tinification project), we should at least handle 
allocation and free of the field coherently across drivers.

> +		if (!dev->dma_parms)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +	if (dma_get_max_seg_size(dev) < DMA_BIT_MASK(32)) {
> +		err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32));

What if the device has set a maximum segment size smaller than 4GB because of 
hardware limitations ?

I also wonder whether this is the correct place to solve the issue. Why is the 
default value returned by dma_get_max_seg_size() set to 64kB ?

> +		if (err)
> +			return ERR_PTR(err);
> +	}
> 
>  	conf = kzalloc(sizeof *conf, GFP_KERNEL);
>  	if (!conf)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: videobuf2-dc: set properly dma_max_segment_size

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Monday 01 June 2015 14:14:17 Marek Szyprowski wrote:
> If device has no DMA max_seg_size set, we assume that there is no limit
> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
> let DMA-mapping API always create contiguous mappings in DMA address
> space. This is essential for all devices, which use dma-contig
> videobuf2 memory allocator.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index
> 644dec73d220..9d7c1814b0f3 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -862,6 +862,23 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>  void *vb2_dma_contig_init_ctx(struct device *dev)
>  {
>  	struct vb2_dc_conf *conf;
> +	int err;
> +
> +	/*
> +	 * if device has no max_seg_size set, we assume that there is no limit
> +	 * and force it to DMA_BIT_MASK(32) to always use contiguous mappings
> +	 * in DMA address space
> +	 */
> +	if (!dev->dma_parms) {
> +		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);

I was checking how dma_parms was usually allocated and freed, and was shocked 
to find that the memory is never freed. OK, actually not shocked, I had a bad 
feeling about it already, but it's still not good :-/

This goes beyond the scope of this patch, but I think we need to clean up 
dma_parms. The structure is 8 bytes long on 32-bit systems and 16 bytes long 
on 64-bit systems. I wonder if it's really worth it to allocate it separately 
from struct device. It might if we moved more DMA-related fields to struct 
device_dma_parameters but that hasn't happened since 2008 when the structure 
was introduced (yes that's more than 7 years ago).

If we consider it's worth it (and I believe Josh Triplett might, in the 
context of the Linux kernel tinification project), we should at least handle 
allocation and free of the field coherently across drivers.

> +		if (!dev->dma_parms)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +	if (dma_get_max_seg_size(dev) < DMA_BIT_MASK(32)) {
> +		err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32));

What if the device has set a maximum segment size smaller than 4GB because of 
hardware limitations ?

I also wonder whether this is the correct place to solve the issue. Why is the 
default value returned by dma_get_max_seg_size() set to 64kB ?

> +		if (err)
> +			return ERR_PTR(err);
> +	}
> 
>  	conf = kzalloc(sizeof *conf, GFP_KERNEL);
>  	if (!conf)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: videobuf2-dc: set properly dma_max_segment_size

[Marek Szyprowski]

Hello,

On 2015-06-03 03:22, Laurent Pinchart wrote:
> On Monday 01 June 2015 14:14:17 Marek Szyprowski wrote:
>> If device has no DMA max_seg_size set, we assume that there is no limit
>> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
>> let DMA-mapping API always create contiguous mappings in DMA address
>> space. This is essential for all devices, which use dma-contig
>> videobuf2 memory allocator.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index
>> 644dec73d220..9d7c1814b0f3 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -862,6 +862,23 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
>>   void *vb2_dma_contig_init_ctx(struct device *dev)
>>   {
>>   	struct vb2_dc_conf *conf;
>> +	int err;
>> +
>> +	/*
>> +	 * if device has no max_seg_size set, we assume that there is no limit
>> +	 * and force it to DMA_BIT_MASK(32) to always use contiguous mappings
>> +	 * in DMA address space
>> +	 */
>> +	if (!dev->dma_parms) {
>> +		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
> I was checking how dma_parms was usually allocated and freed, and was shocked
> to find that the memory is never freed. OK, actually not shocked, I had a bad
> feeling about it already, but it's still not good :-/
>
> This goes beyond the scope of this patch, but I think we need to clean up
> dma_parms. The structure is 8 bytes long on 32-bit systems and 16 bytes long
> on 64-bit systems. I wonder if it's really worth it to allocate it separately
> from struct device. It might if we moved more DMA-related fields to struct
> device_dma_parameters but that hasn't happened since 2008 when the structure
> was introduced (yes that's more than 7 years ago).
>
> If we consider it's worth it (and I believe Josh Triplett might, in the
> context of the Linux kernel tinification project), we should at least handle
> allocation and free of the field coherently across drivers.

Right, the whole dma_params approach looks like some unfinished thing. 
Maybe it
would be better to remove it completely instead of having separate structure
just for 2 values? This will solve the allocation/freeing issue as well.

>
>> +		if (!dev->dma_parms)
>> +			return ERR_PTR(-ENOMEM);
>> +	}
>> +	if (dma_get_max_seg_size(dev) < DMA_BIT_MASK(32)) {
>> +		err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
> What if the device has set a maximum segment size smaller than 4GB because of
> hardware limitations ?

Then it looks that it will make more sense to set max_seg_size only when dma
params structure has been allocated and keep the old value otherwise.

> I also wonder whether this is the correct place to solve the issue.

Frankly I don't see any good place for this code, especially if you consider
the default 64kB value and the fact that some code relies on it, so you 
cannot
change it easily in generic code.

> Why is the
> default value returned by dma_get_max_seg_size() set to 64kB ?

I really have no idea why the default value is 64kB, but it looks that 
there are
drivers that rely on this value:
https://www.marc.info/?l=linux-arm-kernel&m=141692087708203&w=2

>
>> +		if (err)
>> +			return ERR_PTR(err);
>> +	}
>>
>>   	conf = kzalloc(sizeof *conf, GFP_KERNEL);
>>   	if (!conf)
>

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