Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1265709 > unrolled thread

Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2015-11-09 15:20 +0100
    Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size Robin Murphy <robin.murphy@arm.com> - 2015-11-09 16:30 +0100

#1265709 — Re: [RFC/PATCH] media: omap3isp: Set maximum DMA segment size

Hello everybody,

Ping ?

On Tuesday 13 October 2015 16:18:36 Laurent Pinchart wrote:
> The maximum DMA segment size controls the IOMMU mapping granularity. Its
> default value is 64kB, resulting in potentially non-contiguous IOMMU
> mappings. Configure it to 4GB to ensure that buffers get mapped
> contiguously.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/omap3isp/isp.c | 4 ++++
>  drivers/media/platform/omap3isp/isp.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> I'm posting this as an RFC because I'm not happy with the patch, even if it
> gets the job done.
> 
> On ARM the maximum DMA segment size is used when creating IOMMU mappings. As
> a large number of devices require contiguous memory buffers (this is a very
> common requirement for video-related embedded devices) the default 64kB
> value doesn't work.
> 
> I haven't investigated the history behind this API in details but I have a
> feeling something is not quite right. We force most drivers to explicitly
> set the maximum segment size from a default that seems valid for specific
> use cases only. Furthermore, as the DMA parameters are not stored directly
> in struct device this require allocation of external memory for which we
> have no proper management rule, making automatic handling of the DMA
> parameters in frameworks or helper functions cumbersome (for a discussion
> on this topic see http://www.spinics.net/lists/linux-media/msg92467.html
> and http://lists.infradead.org/pipermail/linux-arm-kernel/2014-> November/305913.html).
> 
> Is it time to fix this mess ?
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 17430a6ed85a..ebf7dc76e94d
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2444,6 +2444,10 @@ static int isp_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto error;
> 
> +	isp->dev->dma_parms = &isp->dma_parms;
> +	dma_set_max_seg_size(isp->dev, DMA_BIT_MASK(32));
> +	dma_set_seg_boundary(isp->dev, 0xffffffff);
> +
>  	platform_set_drvdata(pdev, isp);
> 
>  	/* Regulators */
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index e579943175c4..4b2231cf01be
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -193,6 +193,7 @@ struct isp_device {
>  	u32 syscon_offset;
>  	u32 phy_type;
> 
> +	struct device_dma_parameters dma_parms;
>  	struct dma_iommu_mapping *mapping;
> 
>  	/* ISP Obj */

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1265761

Hi Laurent,

On 09/11/15 14:18, Laurent Pinchart wrote:
> Hello everybody,
>
> Ping ?

Apologies, I did start writing a response a while ago, but it ended up 
getting subsumed into the bigger DMA API discussion thread.

> On Tuesday 13 October 2015 16:18:36 Laurent Pinchart wrote:
>> The maximum DMA segment size controls the IOMMU mapping granularity. Its
>> default value is 64kB, resulting in potentially non-contiguous IOMMU
>> mappings. Configure it to 4GB to ensure that buffers get mapped
>> contiguously.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/media/platform/omap3isp/isp.c | 4 ++++
>>   drivers/media/platform/omap3isp/isp.h | 1 +
>>   2 files changed, 5 insertions(+)
>>
>> I'm posting this as an RFC because I'm not happy with the patch, even if it
>> gets the job done.
>>
>> On ARM the maximum DMA segment size is used when creating IOMMU mappings. As
>> a large number of devices require contiguous memory buffers (this is a very
>> common requirement for video-related embedded devices) the default 64kB
>> value doesn't work.

Per the initial patch (6b7b65105522), dma_parms was intended to expose 
hardware limitations of scatter-gather-capable devices, to prevent DMA 
API implementations from merging segments beyond a device's limits. Thus 
the way 32-bit ARM (and seemingly noone else) is taking it as something 
to apply to non-scatter-gather-capable devices in order to force the DMA 
API implementation to merge segments seems very questionable.

There's nothing in the streaming DMA API which gives any guarantee of a 
contiguous mapping, so it's the incorrect assumption that it does which 
needs fixing. Whether we rework the callers or the API itself is the 
open question there, I think.

>> I haven't investigated the history behind this API in details but I have a
>> feeling something is not quite right. We force most drivers to explicitly
>> set the maximum segment size from a default that seems valid for specific
>> use cases only. Furthermore, as the DMA parameters are not stored directly
>> in struct device this require allocation of external memory for which we
>> have no proper management rule, making automatic handling of the DMA
>> parameters in frameworks or helper functions cumbersome (for a discussion
>> on this topic see http://www.spinics.net/lists/linux-media/msg92467.html
>> and http://lists.infradead.org/pipermail/linux-arm-kernel/2014-> November/305913.html).
>>
>> Is it time to fix this mess ?

I agree that it would certainly be preferable to tackle the underlying 
issue instead of adding more point hacks to further entrench 
non-portable code into the kernel. In terms of modifying the API, the 
most reasonable idea which comes to mind would be a DMA attribute, and 
on closer inspection, I see that DMA_ATTR_FORCE_CONTIGUOUS is already a 
thing - perhaps we should weigh up whether coherent and streaming DMA 
could overload the same attribute with subtly different meanings, or 
whether we'd want e.g. DMA_ATTR_FORCE_PHYS_CONTIGUOUS and 
DMA_ATTR_FORCE_DMA_CONTIGUOUS to coexist.

Robin.

>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index 17430a6ed85a..ebf7dc76e94d
>> 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -2444,6 +2444,10 @@ static int isp_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto error;
>>
>> +	isp->dev->dma_parms = &isp->dma_parms;
>> +	dma_set_max_seg_size(isp->dev, DMA_BIT_MASK(32));
>> +	dma_set_seg_boundary(isp->dev, 0xffffffff);

Whatever happens, 002edb6f6f2a should now make this line redundant :D

>> +
>>   	platform_set_drvdata(pdev, isp);
>>
>>   	/* Regulators */
>> diff --git a/drivers/media/platform/omap3isp/isp.h
>> b/drivers/media/platform/omap3isp/isp.h index e579943175c4..4b2231cf01be
>> 100644
>> --- a/drivers/media/platform/omap3isp/isp.h
>> +++ b/drivers/media/platform/omap3isp/isp.h
>> @@ -193,6 +193,7 @@ struct isp_device {
>>   	u32 syscon_offset;
>>   	u32 phy_type;
>>
>> +	struct device_dma_parameters dma_parms;
>>   	struct dma_iommu_mapping *mapping;
>>
>>   	/* ISP Obj */
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web