[PATCH 0/2] OMAP1: amsdelta: reserve memory for videobuf_contig

[PATCH 0/2] OMAP1: amsdelta: reserve memory for videobuf_contig

[Janusz Krzysztofik]

Latest developements seem to allow for reserving a block of memory on boot to 
be used as a device dedicated dma coherent memory. This may be required for 
videobuf_config based video drivers avoid problems with allocating dma 
coherent memory after system memory gets fragmented.

This set extends the OMAP1 camera resource initialization code with a function 
that can be used for reserving a block of memory early, then declared as the 
camera device dedicated dma coherent memory.

An example use case is provided for Amstrad Delta camera.

 arch/arm/mach-omap1/board-ams-delta.c     |   12 +++++++++-
 arch/arm/mach-omap1/devices.c             |   36 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap1/include/mach/camera.h |    1
 3 files changed, 48 insertions(+), 1 deletion(-)


Thanks,
Janusz

[PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

OMAP1 camera driver, when starting up in its videobuf_contig mode, may have 
problems with allocating dma coherent memory after system memory gets 
fragmented if there is no dedicated memory declared as a dma coherent memory 
block associated with the device. To solve this issue, allow for removing a 
block of memory from system memory early on boot, then, if reserved this way, 
declare it as the device dedicated dma coherent memory.

An example use case on Amstrad Delta will be provided with patch 2/2.

Created and tested against linux-2.6.37-rc4.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 arch/arm/mach-omap1/devices.c             |   36 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap1/include/mach/camera.h |    1
 2 files changed, 37 insertions(+)

--- linux-2.6.37-rc4/arch/arm/mach-omap1/devices.c.orig	2010-12-04 18:00:39.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/devices.c	2010-12-04 22:22:13.000000000 +0100
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/spi/spi.h>
+#include <linux/memblock.h>
 
 #include <mach/hardware.h>
 #include <asm/mach/map.h>
@@ -222,13 +223,48 @@ static struct platform_device omap1_came
 	.resource	= omap1_camera_resources,
 };
 
+static phys_addr_t omap1_camera_phys_mempool_base;
+static phys_addr_t omap1_camera_phys_mempool_size;
+
+void __init omap1_camera_reserve(phys_addr_t size)
+{
+	phys_addr_t paddr;
+
+	if (!size)
+		return;
+
+	paddr = memblock_alloc(size, SZ_1M);
+
+	if (!paddr) {
+		pr_err("%s: failed to reserve %x bytes\n", __func__, size);
+		return;
+	}
+	memblock_free(paddr, size);
+	memblock_remove(paddr, size);
+
+	omap1_camera_phys_mempool_base = paddr;
+	omap1_camera_phys_mempool_size = size;
+}
+
 void __init omap1_camera_init(void *info)
 {
 	struct platform_device *dev = &omap1_camera_device;
+	dma_addr_t paddr = omap1_camera_phys_mempool_base;
+	dma_addr_t size = omap1_camera_phys_mempool_size;
 	int ret;
 
 	dev->dev.platform_data = info;
 
+	if (paddr) {
+		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
+				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
+			pr_info("%s: reserved %d bytes camera buffer memory\n",
+					__func__, size);
+		else
+			pr_warn("%s: cannot reserve camera buffer memory\n",
+					__func__);
+	}
+
 	ret = platform_device_register(dev);
 	if (ret)
 		dev_err(&dev->dev, "unable to register device: %d\n", ret);
--- linux-2.6.37-rc4/arch/arm/mach-omap1/include/mach/camera.h.orig	2010-12-04 18:00:39.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/include/mach/camera.h	2010-12-04 22:26:23.000000000 +0100
@@ -3,6 +3,7 @@
 
 #include <media/omap1_camera.h>
 
+void omap1_camera_reserve(phys_addr_t);
 void omap1_camera_init(void *);
 
 static inline void omap1_set_camera_info(struct omap1_cam_platform_data *info)

[PATCH 2/2] OMAP1: Amstrad Delta: reserve memory for camera

[Janusz Krzysztofik]

Patch 1/2 from this set provides a possibility to reserve a block of memory 
for use as the OMAP1 camera dedicated dma coherent memory. Use this 
functionality to avoid the camera driver switching form videobuf_contig mode 
to less efficient videobuf_sg mode in case of dma coherent memory allocation 
failure after system memory gets fragmented.

Created and tested against linux-2.6.27-rc4 on top of patch 1/2.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 arch/arm/mach-omap1/board-ams-delta.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

--- linux-2.6.37-rc4/arch/arm/mach-omap1/board-ams-delta.c.orig	2010-12-04 18:05:25.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/board-ams-delta.c	2010-12-04 22:19:39.000000000 +0100
@@ -262,6 +262,16 @@ static struct omap1_cam_platform_data am
 	.lclk_khz_max	= 1334,		/* results in 5fps CIF, 10fps QCIF */
 };
 
+void __init amsdelta_reserve(void)
+{
+#if defined(CONFIG_VIDEO_OMAP1) || defined(CONFIG_VIDEO_OMAP1_MODULE)
+	omap1_camera_reserve(PAGE_SIZE << get_order(352 * 288 * 2 *
+			OMAP1_CAMERA_MIN_BUF_COUNT(OMAP1_CAM_DMA_CONTIG)));
+#endif
+
+	omap_reserve();
+}
+
 static struct platform_device *ams_delta_devices[] __initdata = {
 	&ams_delta_kp_device,
 	&ams_delta_lcd_device,
@@ -366,7 +376,7 @@ MACHINE_START(AMS_DELTA, "Amstrad E3 (De
 	/* Maintainer: Jonathan McDowell <noodles@earth.li> */
 	.boot_params	= 0x10000100,
 	.map_io		= ams_delta_map_io,
-	.reserve	= omap_reserve,
+	.reserve	= amsdelta_reserve,
 	.init_irq	= ams_delta_init_irq,
 	.init_machine	= ams_delta_init,
 	.timer		= &omap_timer,

Re: [PATCH 0/2] OMAP1: amsdelta: reserve memory for videobuf_contig

[Tony Lindgren]

* Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [101205 10:19]:
> Latest developements seem to allow for reserving a block of memory on boot to 
> be used as a device dedicated dma coherent memory. This may be required for 
> videobuf_config based video drivers avoid problems with allocating dma 
> coherent memory after system memory gets fragmented.
> 
> This set extends the OMAP1 camera resource initialization code with a function 
> that can be used for reserving a block of memory early, then declared as the 
> camera device dedicated dma coherent memory.
> 
> An example use case is provided for Amstrad Delta camera.

These look good to me, however, can you please also Cc linux-arm-kernel
list as otherwise I will need to repost these before merging.

Thanks,

Tony

Re: [PATCH 0/2] OMAP1: amsdelta: reserve memory for videobuf_contig

[Janusz Krzysztofik]

Friday 10 December 2010 02:29:09 Tony Lindgren wrote:
> * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [101205 10:19]:
> > Latest developements seem to allow for reserving a block of memory on
> > boot to be used as a device dedicated dma coherent memory. This may be
> > required for videobuf_config based video drivers avoid problems with
> > allocating dma coherent memory after system memory gets fragmented.
> >
> > This set extends the OMAP1 camera resource initialization code with a
> > function that can be used for reserving a block of memory early, then
> > declared as the camera device dedicated dma coherent memory.
> >
> > An example use case is provided for Amstrad Delta camera.
>
> These look good to me, however, can you please also Cc linux-arm-kernel
> list as otherwise I will need to repost these before merging.

Sure. Perhaps that first submission should rather go as an RFC, now that there 
are no requests for changes, I'll repost with Cc linux-arm-kernel.

Thanks,
Janusz

[RESEND] [PATCH 0/2] OMAP1: amsdelta: reserve memory for videobuf_contig

[Janusz Krzysztofik]

Since there were no request for changes, I'm resending the set, 
this time Cc linux-arm-kernel.

Sunday 05 December 2010 19:29:05 Janusz Krzysztofik wrote:
> Latest developements seem to allow for reserving a block of memory on boot
> to be used as a device dedicated dma coherent memory. This may be required
> for videobuf_config based video drivers avoid problems with allocating dma
> coherent memory after system memory gets fragmented.
>
> This set extends the OMAP1 camera resource initialization code with a
> function that can be used for reserving a block of memory early, then
> declared as the camera device dedicated dma coherent memory.
>
> An example use case is provided for Amstrad Delta camera.
>
>  arch/arm/mach-omap1/board-ams-delta.c     |   12 +++++++++-
>  arch/arm/mach-omap1/devices.c             |   36 ++++++++++++++++++++++++++++++ 
>  arch/arm/mach-omap1/include/mach/camera.h |    1
>  3 files changed, 48 insertions(+), 1 deletion(-)
>
>
> Thanks,
> Janusz

[RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

OMAP1 camera driver, when starting up in its videobuf_contig mode, may have 
problems with allocating dma coherent memory after system memory gets 
fragmented if there is no dedicated memory declared as a dma coherent memory 
block associated with the device. To solve this issue, allow for removing a 
block of memory from system memory early on boot, then, if reserved this way, 
declare it as the device dedicated dma coherent memory.

An example use case on Amstrad Delta will be provided with patch 2/2.

Created and tested against linux-2.6.37-rc4.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 arch/arm/mach-omap1/devices.c             |   36 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap1/include/mach/camera.h |    1
 2 files changed, 37 insertions(+)

--- linux-2.6.37-rc4/arch/arm/mach-omap1/devices.c.orig	2010-12-04 18:00:39.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/devices.c	2010-12-04 22:22:13.000000000 +0100
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/spi/spi.h>
+#include <linux/memblock.h>
 
 #include <mach/hardware.h>
 #include <asm/mach/map.h>
@@ -222,13 +223,48 @@ static struct platform_device omap1_came
 	.resource	= omap1_camera_resources,
 };
 
+static phys_addr_t omap1_camera_phys_mempool_base;
+static phys_addr_t omap1_camera_phys_mempool_size;
+
+void __init omap1_camera_reserve(phys_addr_t size)
+{
+	phys_addr_t paddr;
+
+	if (!size)
+		return;
+
+	paddr = memblock_alloc(size, SZ_1M);
+
+	if (!paddr) {
+		pr_err("%s: failed to reserve %x bytes\n", __func__, size);
+		return;
+	}
+	memblock_free(paddr, size);
+	memblock_remove(paddr, size);
+
+	omap1_camera_phys_mempool_base = paddr;
+	omap1_camera_phys_mempool_size = size;
+}
+
 void __init omap1_camera_init(void *info)
 {
 	struct platform_device *dev = &omap1_camera_device;
+	dma_addr_t paddr = omap1_camera_phys_mempool_base;
+	dma_addr_t size = omap1_camera_phys_mempool_size;
 	int ret;
 
 	dev->dev.platform_data = info;
 
+	if (paddr) {
+		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
+				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
+			pr_info("%s: reserved %d bytes camera buffer memory\n",
+					__func__, size);
+		else
+			pr_warn("%s: cannot reserve camera buffer memory\n",
+					__func__);
+	}
+
 	ret = platform_device_register(dev);
 	if (ret)
 		dev_err(&dev->dev, "unable to register device: %d\n", ret);
--- linux-2.6.37-rc4/arch/arm/mach-omap1/include/mach/camera.h.orig	2010-12-04 18:00:39.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/include/mach/camera.h	2010-12-04 22:26:23.000000000 +0100
@@ -3,6 +3,7 @@
 
 #include <media/omap1_camera.h>
 
+void omap1_camera_reserve(phys_addr_t);
 void omap1_camera_init(void *);
 
 static inline void omap1_set_camera_info(struct omap1_cam_platform_data *info)

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Russell King - ARM Linux]

On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
>  void __init omap1_camera_init(void *info)
>  {
>  	struct platform_device *dev = &omap1_camera_device;
> +	dma_addr_t paddr = omap1_camera_phys_mempool_base;
> +	dma_addr_t size = omap1_camera_phys_mempool_size;
>  	int ret;
>  
>  	dev->dev.platform_data = info;
>  
> +	if (paddr) {
> +		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> +				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))

Although this works, you're ending up with SDRAM being mapped via
ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
mapped as if it were a device.

For OMAP1, which is ARMv5 or lower, device memory becomes 'uncacheable,
unbufferable' which is luckily what is used for the DMA coherent
memory on those platforms - so no technical problem here.

However, on ARMv6 and later, ioremapped memory is device memory, which
has different ordering wrt normal memory mappings, and may appear on
different busses on the CPU's interface.  So, this is something I don't
encourage as it's unclear that the hardware will work.

Essentially, dma_declare_coherent_memory() on ARM with main SDRAM should
be viewed as a 'it might work, it might not, and it might stop working
in the future' kind of interface.  In other words, there is no guarantee
that this kind of thing will be supported in the future.

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

Friday 10 December 2010 18:03:56 Russell King - ARM Linux napisał(a):
> On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> >  void __init omap1_camera_init(void *info)
> >  {
> >  	struct platform_device *dev = &omap1_camera_device;
> > +	dma_addr_t paddr = omap1_camera_phys_mempool_base;
> > +	dma_addr_t size = omap1_camera_phys_mempool_size;
> >  	int ret;
> >
> >  	dev->dev.platform_data = info;
> >
> > +	if (paddr) {
> > +		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> > +				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
>
> Although this works, you're ending up with SDRAM being mapped via
> ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> mapped as if it were a device.
>
> For OMAP1, which is ARMv5 or lower, device memory becomes 'uncacheable,
> unbufferable' which is luckily what is used for the DMA coherent
> memory on those platforms - so no technical problem here.
>
> However, on ARMv6 and later, ioremapped memory is device memory, which
> has different ordering wrt normal memory mappings, and may appear on
> different busses on the CPU's interface.  So, this is something I don't
> encourage as it's unclear that the hardware will work.
>
> Essentially, dma_declare_coherent_memory() on ARM with main SDRAM should
> be viewed as a 'it might work, it might not, and it might stop working
> in the future' kind of interface.  In other words, there is no guarantee
> that this kind of thing will be supported in the future.

I was affraid of an unswer of this kind. I'm not capable of comming out with 
any better, alternative solutions. Any hints other than giving up with that 
old videobuf-contig, coherent memory based interface? Or can we agree on 
that 'luckily uncacheable, unbufferable, SDRAM based DMA coherent memory' 
solution for now?

Thanks,
Janusz

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

On Fri 10 Dec 2010 at 22:03:52 Janusz Krzysztofik wrote:
> Friday 10 December 2010 18:03:56 Russell King - ARM Linux napisał(a):
> > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> > >  void __init omap1_camera_init(void *info)
> > >  {
> > >  
> > >  	struct platform_device *dev = &omap1_camera_device;
> > > 
> > > +	dma_addr_t paddr = omap1_camera_phys_mempool_base;
> > > +	dma_addr_t size = omap1_camera_phys_mempool_size;
> > > 
> > >  	int ret;
> > >  	
> > >  	dev->dev.platform_data = info;
> > > 
> > > +	if (paddr) {
> > > +		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> > > +				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
> > 
> > Although this works, you're ending up with SDRAM being mapped via
> > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> > mapped as if it were a device.
> > 
> > For OMAP1, which is ARMv5 or lower, device memory becomes
> > 'uncacheable, unbufferable' which is luckily what is used for the
> > DMA coherent memory on those platforms - so no technical problem
> > here.
> > 
> > However, on ARMv6 and later, ioremapped memory is device memory,
> > which has different ordering wrt normal memory mappings, and may
> > appear on different busses on the CPU's interface.  So, this is
> > something I don't encourage as it's unclear that the hardware will
> > work.
> > 
> > Essentially, dma_declare_coherent_memory() on ARM with main SDRAM
> > should be viewed as a 'it might work, it might not, and it might
> > stop working in the future' kind of interface.  In other words,
> > there is no guarantee that this kind of thing will be supported in
> > the future.
> 
> I was affraid of an unswer of this kind. I'm not capable of comming
> out with any better, alternative solutions. Any hints other than
> giving up with that old videobuf-contig, coherent memory based
> interface? Or can we agree on that 'luckily uncacheable,
> unbufferable, SDRAM based DMA coherent memory' solution for now?

Russell, Tony,

Sorry for getting back to this old thread, but since my previous 
attempts to provide[1] or support[2] a possibly better solution to the 
problem all failed on one hand, and I can see patches not very different 
from mine[3] being accepted for arch/arm/mach-{mx3,imx} during this and 
previous merge windows[4][5][6] on the other, is there any chance for the 
'dma_declare_coherent_memory() over a memblock_alloc()->free()->remove() 
obtained area' based solution being accepted for omap1_camera as well if 
I resend it refreshed?

BTW, commit 6d3163ce86dd386b4f7bda80241d7fea2bc0bb1d, "mm: check if any 
page in a pageblock is reserved before marking it MIGRATE_RESERVE", 
almost corrected the problem for me, allowing for very stable device 
operation in videobuf_dma_contig mode once all resident programs get 
settled up and no unusual activity happens, but this is still not 100% 
reliable, so I think that only using a kind of memory area reservered 
during boot for the purpose can be considered as free of transient 
-ENOMEM issues.

Thanks,
Janusz

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036419.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036551.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/034643.html
[4] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;h=164f7b5237cca2701dd2943928ec8d078877a28d
[5] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;h=031e912741746e4350204bb0436590ca0e993a7d
[6] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;h=dca7c0b4293a06d1ed9387e729a4882896abccc2

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Russell King - ARM Linux]

On Wed, Jun 08, 2011 at 11:53:49PM +0200, Janusz Krzysztofik wrote:
> On Fri 10 Dec 2010 at 22:03:52 Janusz Krzysztofik wrote:
> > Friday 10 December 2010 18:03:56 Russell King - ARM Linux napisał(a):
> > > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> > > >  void __init omap1_camera_init(void *info)
> > > >  {
> > > >  
> > > >  	struct platform_device *dev = &omap1_camera_device;
> > > > 
> > > > +	dma_addr_t paddr = omap1_camera_phys_mempool_base;
> > > > +	dma_addr_t size = omap1_camera_phys_mempool_size;
> > > > 
> > > >  	int ret;
> > > >  	
> > > >  	dev->dev.platform_data = info;
> > > > 
> > > > +	if (paddr) {
> > > > +		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> > > > +				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
> > > 
> > > Although this works, you're ending up with SDRAM being mapped via
> > > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> > > mapped as if it were a device.
> > > 
> > > For OMAP1, which is ARMv5 or lower, device memory becomes
> > > 'uncacheable, unbufferable' which is luckily what is used for the
> > > DMA coherent memory on those platforms - so no technical problem
> > > here.
> > > 
> > > However, on ARMv6 and later, ioremapped memory is device memory,
> > > which has different ordering wrt normal memory mappings, and may
> > > appear on different busses on the CPU's interface.  So, this is
> > > something I don't encourage as it's unclear that the hardware will
> > > work.
> > > 
> > > Essentially, dma_declare_coherent_memory() on ARM with main SDRAM
> > > should be viewed as a 'it might work, it might not, and it might
> > > stop working in the future' kind of interface.  In other words,
> > > there is no guarantee that this kind of thing will be supported in
> > > the future.
> > 
> > I was affraid of an unswer of this kind. I'm not capable of comming
> > out with any better, alternative solutions. Any hints other than
> > giving up with that old videobuf-contig, coherent memory based
> > interface? Or can we agree on that 'luckily uncacheable,
> > unbufferable, SDRAM based DMA coherent memory' solution for now?
> 
> Russell, Tony,
> 
> Sorry for getting back to this old thread, but since my previous 
> attempts to provide[1] or support[2] a possibly better solution to the 
> problem all failed on one hand, and I can see patches not very different 
> from mine[3] being accepted for arch/arm/mach-{mx3,imx} during this and 
> previous merge windows[4][5][6] on the other, is there any chance for the 
> 'dma_declare_coherent_memory() over a memblock_alloc()->free()->remove() 
> obtained area' based solution being accepted for omap1_camera as well if 
> I resend it refreshed?

I stand by my answer to your patches quoted above from a technical point
of view; we should not be mapping SDRAM using device mappings.

That ioremap() inside dma_declare_coherent_memory() needs to die, but
it seems that those who now look after the DMA API really aren't
interested in the technical details of this being wrong for some
architecture - just like they're not really interested in the details
of devices using dma-engines for their DMA support.

I'm afraid that the DMA support in Linux sucks because of this, and I
have no real desire to participate in discussions with brick walls over
this.

Certainly the memblock_alloc()+free()+remove() is the right way to
reserve the memory, but as it stands dma_declare_coherent_memory()
should not be used on ARMv6 or higher CPUs to pass that memory to the
device driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

On Thu 09 Jun 2011 at 00:13:30 Russell King - ARM Linux wrote:
> 
> I stand by my answer to your patches quoted above from a technical
> point of view; we should not be mapping SDRAM using device mappings.
> 
> That ioremap() inside dma_declare_coherent_memory() needs to die,

Then how about your alternative, ARM specific solution, "Avoid aliasing 
mappings in DMA coherent allocator"? There was a series of initially 
two, then three patches, of which the two others (459c1517f987 "ARM: 
DMA: Replace page_to_dma()/dma_to_page() with pfn_to_dma()/dma_to_pfn()" 
and 9eedd96301ca "ARM: DMA: top-down allocation in DMA coherent region") 
are already in the mainline tree. I'm still porting a copy of that third 
one from kernel version to version to have my device working 100% 
reliably, in hope you finally push it into the mainline. No plans 
against it? Or is there something about it I could help with?

Thanks,
Janusz

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

On Thu, 9 Jun 2011 at 00:13:30 Russell King - ARM Linux wrote:
> On Wed, Jun 08, 2011 at 11:53:49PM +0200, Janusz Krzysztofik wrote:
> > On Fri 10 Dec 2010 at 22:03:52 Janusz Krzysztofik wrote:
> > > Friday 10 December 2010 18:03:56 Russell King - ARM Linux napisał(a):
> > > > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> > > > >  void __init omap1_camera_init(void *info)
> > > > >  {
> > > > >  
> > > > >  	struct platform_device *dev = &omap1_camera_device;
> > > > > 
> > > > > +	dma_addr_t paddr = omap1_camera_phys_mempool_base;
> > > > > +	dma_addr_t size = omap1_camera_phys_mempool_size;
> > > > > 
> > > > >  	int ret;
> > > > >  	
> > > > >  	dev->dev.platform_data = info;
> > > > > 
> > > > > +	if (paddr) {
> > > > > +		if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> > > > > +				DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
> > > > 
> > > > Although this works, you're ending up with SDRAM being mapped
> > > > via ioremap, which uses MT_DEVICE - so what is SDRAM ends up
> > > > being mapped as if it were a device.
> > > > 
> > > > For OMAP1, which is ARMv5 or lower, device memory becomes
> > > > 'uncacheable, unbufferable' which is luckily what is used for
> > > > the DMA coherent memory on those platforms - so no technical
> > > > problem here.
> > > > 
> > > > However, on ARMv6 and later, ioremapped memory is device
> > > > memory, which has different ordering wrt normal memory
> > > > mappings, and may appear on different busses on the CPU's
> > > > interface.  So, this is something I don't encourage as it's
> > > > unclear that the hardware will work.
> > > > 
> > > > Essentially, dma_declare_coherent_memory() on ARM with main
> > > > SDRAM should be viewed as a 'it might work, it might not, and
> > > > it might stop working in the future' kind of interface.  In
> > > > other words, there is no guarantee that this kind of thing
> > > > will be supported in the future.
> > 
> > Russell, Tony,
> > 
> > Sorry for getting back to this old thread, but since my previous
> > attempts to provide[1] or support[2] a possibly better solution to
> > the problem all failed on one hand, and I can see patches not very
> > different from mine[3] being accepted for arch/arm/mach-{mx3,imx}
> > during this and previous merge windows[4][5][6] on the other, is
> > there any chance for the 'dma_declare_coherent_memory() over a
> > memblock_alloc()->free()->remove() obtained area' based solution
> > being accepted for omap1_camera as well if I resend it refreshed?
> 
> I stand by my answer to your patches quoted above from a technical
> point of view; we should not be mapping SDRAM using device mappings.

Russell,
Would it change anything here if we define ARCH_HAS_HOLES_MEMORYMODEL, 
as suggested by Marek Szyprowski recently[*], when configuring for 
ARCH_OMAP1 with VIDEO_OMAP1 (camera) selected?

[*] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/057447.html

> Certainly the memblock_alloc()+free()+remove() is the right way to
> reserve the memory, but as it stands dma_declare_coherent_memory()
> should not be used on ARMv6 or higher CPUs to pass that memory to the
> device driver.

Tony,
With full respect to all Russell's reservations about incorrectness of 
ioremapping SDRAM in general, would you be willing to accept this 
solution in the OMAP1 camera case, taking into account that, quoting 
Russell, "there is no technical problem here", and similiar solutions 
had been accepted recently with other ARM platforms?

Thanks,
Janusz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Catalin Marinas]

On 10 December 2010 17:03, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
>>  void __init omap1_camera_init(void *info)
>>  {
>>       struct platform_device *dev = &omap1_camera_device;
>> +     dma_addr_t paddr = omap1_camera_phys_mempool_base;
>> +     dma_addr_t size = omap1_camera_phys_mempool_size;
>>       int ret;
>>
>>       dev->dev.platform_data = info;
>>
>> +     if (paddr) {
>> +             if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
>> +                             DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
>
> Although this works, you're ending up with SDRAM being mapped via
> ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> mapped as if it were a device.

BTW, does the generic dma_declare_coherent_memory() does the correct
thing in using ioremap()? Maybe some other function that takes a
pgprot_t would be better (ioremap_page_range) and could pass something
like pgprot_noncached (though ideally a pgprot_dmacoherent). Or just
an architecturally-defined function.

-- 
Catalin

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Russell King - ARM Linux]

On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote:
> On 10 December 2010 17:03, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> >>  void __init omap1_camera_init(void *info)
> >>  {
> >>       struct platform_device *dev = &omap1_camera_device;
> >> +     dma_addr_t paddr = omap1_camera_phys_mempool_base;
> >> +     dma_addr_t size = omap1_camera_phys_mempool_size;
> >>       int ret;
> >>
> >>       dev->dev.platform_data = info;
> >>
> >> +     if (paddr) {
> >> +             if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> >> +                             DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
> >
> > Although this works, you're ending up with SDRAM being mapped via
> > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> > mapped as if it were a device.
> 
> BTW, does the generic dma_declare_coherent_memory() does the correct
> thing in using ioremap()?

I argue it doesn't, as I said above.  It means we map SDRAM as device
memory, and as I understand the way interconnects work, it's entirely
possible that this may not result in the SDRAM being accessible.

> Maybe some other function that takes a
> pgprot_t would be better (ioremap_page_range) and could pass something
> like pgprot_noncached (though ideally a pgprot_dmacoherent). Or just
> an architecturally-defined function.

This API was designed in the ARMv5 days when things were a lot simpler.
What we now have is rather messy though, and really needs sorting out
before we get too many users of it.  (Arguably it should never have been
accepted in its current state.)

We have a rule that the returned value of ioremap() is not to be used
as a pointer which can be dereferenced by anything except driver code.
Yet this is exactly what dma_declare_coherent_memory() does:

struct dma_coherent_mem {
        void            *virt_base;
...
};

int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
                                dma_addr_t device_addr, size_t size, int flags)
{
        void __iomem *mem_base = NULL;
        mem_base = ioremap(bus_addr, size);
        dev->dma_mem->virt_base = mem_base;
}

int dma_alloc_from_coherent(struct device *dev, ssize_t size,
                                       dma_addr_t *dma_handle, void **ret)
{
        *ret = mem->virt_base + (pageno << PAGE_SHIFT);
}

and drivers expect the returned value from dma_alloc_coherent() to be
dereferenceable.

Note that this also fails sparse checking.  It will fail on any
architecture where the return value from ioremap() is not a virtual
address.

So, not only does this fail the kernel's own rules, but as we already know,
it fails the architecture's restrictions with multiple mappings of memory
when used with SDRAM, and it also maps main memory as a device.  I wonder
how many more things this broken API needs to do wrong before it's current
implementation is consigned to the circular filing cabinet.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Catalin Marinas]

On 13 December 2010 16:29, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote:
>> On 10 December 2010 17:03, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
>> >>  void __init omap1_camera_init(void *info)
>> >>  {
>> >>       struct platform_device *dev = &omap1_camera_device;
>> >> +     dma_addr_t paddr = omap1_camera_phys_mempool_base;
>> >> +     dma_addr_t size = omap1_camera_phys_mempool_size;
>> >>       int ret;
>> >>
>> >>       dev->dev.platform_data = info;
>> >>
>> >> +     if (paddr) {
>> >> +             if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
>> >> +                             DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
>> >
>> > Although this works, you're ending up with SDRAM being mapped via
>> > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
>> > mapped as if it were a device.
>>
>> BTW, does the generic dma_declare_coherent_memory() does the correct
>> thing in using ioremap()?
>
> I argue it doesn't, as I said above.  It means we map SDRAM as device
> memory, and as I understand the way interconnects work, it's entirely
> possible that this may not result in the SDRAM being accessible.
[...]
> So, not only does this fail the kernel's own rules, but as we already know,
> it fails the architecture's restrictions with multiple mappings of memory
> when used with SDRAM, and it also maps main memory as a device.  I wonder
> how many more things this broken API needs to do wrong before it's current
> implementation is consigned to the circular filing cabinet.

Should we not try to fix the generic code and still allow platforms to
use dma_declare_coherent_memory() in a safer way? I guess it may need
some arguing/explanation on linux-arch.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Russell King - ARM Linux]

On Wed, Dec 15, 2010 at 12:39:20PM +0000, Catalin Marinas wrote:
> On 13 December 2010 16:29, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote:
> >> On 10 December 2010 17:03, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote:
> >> >>  void __init omap1_camera_init(void *info)
> >> >>  {
> >> >>       struct platform_device *dev = &omap1_camera_device;
> >> >> +     dma_addr_t paddr = omap1_camera_phys_mempool_base;
> >> >> +     dma_addr_t size = omap1_camera_phys_mempool_size;
> >> >>       int ret;
> >> >>
> >> >>       dev->dev.platform_data = info;
> >> >>
> >> >> +     if (paddr) {
> >> >> +             if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size,
> >> >> +                             DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE))
> >> >
> >> > Although this works, you're ending up with SDRAM being mapped via
> >> > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being
> >> > mapped as if it were a device.
> >>
> >> BTW, does the generic dma_declare_coherent_memory() does the correct
> >> thing in using ioremap()?
> >
> > I argue it doesn't, as I said above.  It means we map SDRAM as device
> > memory, and as I understand the way interconnects work, it's entirely
> > possible that this may not result in the SDRAM being accessible.
> [...]
> > So, not only does this fail the kernel's own rules, but as we already know,
> > it fails the architecture's restrictions with multiple mappings of memory
> > when used with SDRAM, and it also maps main memory as a device.  I wonder
> > how many more things this broken API needs to do wrong before it's current
> > implementation is consigned to the circular filing cabinet.
> 
> Should we not try to fix the generic code and still allow platforms to
> use dma_declare_coherent_memory() in a safer way? I guess it may need
> some arguing/explanation on linux-arch.

I think so - one of the issues with dma_declare_coherent_memory() is that
it's original intention (as I understand it) was to allow drivers to use
on-device dma coherent memory.

Eg, a network controller with its own local SRAM which it can fetch DMA
descriptors from, which tells it where in the bus address space to fetch
packets from.  This SRAM is not part of the hosts memory, but is on the
peripheral's bus, and so ioremap() (or maybe ioremap_wc()) would be
appropriate for it.

However, ioremap() on system RAM (even that which has been taken out on
the memory map) may be problematical.

I think the correct solution would be to revise the interface so it takes
a void * pointer, which can be handed out by dma_alloc_coherent() directly
without the API having to worry about how to map the memory.  IOW, push
the mapping of that memory up a level to the caller of
dma_declare_coherent_memory().

We can then sanely do preallocations via dma_coherent_alloc() and caching
them back into dma_declare_coherent_memory() without creating additional
mappings which cause architectural violations.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera

[Janusz Krzysztofik]

Wednesday 15 December 2010 18:01:42 Russell King - ARM Linux wrote:
> On Wed, Dec 15, 2010 at 12:39:20PM +0000, Catalin Marinas wrote:
> >
> > Should we not try to fix the generic code and still allow platforms
> > to use dma_declare_coherent_memory() in a safer way? I guess it may
> > need some arguing/explanation on linux-arch.
>
> I think so - 

Hi Russel,
I've already started implementing what you've suggested, with an already 
working result, but have two questions:

1. Is it save to leave iounmap() being called on virtual addresses not 
   obtained with ioremap()? This would make things less complicated.

2. Can I quote your full explanation, just like it looks below, in my 
   commit message?

Thanks,
Janusz

Wednesday 15 December 2010 18:01:42 Russell King - ARM Linux wrote:
> ... one of the issues with dma_declare_coherent_memory() is 
> that it's original intention (as I understand it) was to allow
> drivers to use on-device dma coherent memory.
>
> Eg, a network controller with its own local SRAM which it can fetch
> DMA descriptors from, which tells it where in the bus address space
> to fetch packets from.  This SRAM is not part of the hosts memory,
> but is on the peripheral's bus, and so ioremap() (or maybe
> ioremap_wc()) would be appropriate for it.
>
> However, ioremap() on system RAM (even that which has been taken out
> on the memory map) may be problematical.
>
> I think the correct solution would be to revise the interface so it
> takes a void * pointer, which can be handed out by
> dma_alloc_coherent() directly without the API having to worry about
> how to map the memory.  IOW, push the mapping of that memory up a
> level to the caller of
> dma_declare_coherent_memory().
>
> We can then sanely do preallocations via dma_coherent_alloc() and
> caching them back into dma_declare_coherent_memory() without creating
> additional mappings which cause architectural violations.

[PATCH 2/2] OMAP1: Amstrad Delta: reserve memory for camera

[Janusz Krzysztofik]

Patch 1/2 from this set provides a possibility to reserve a block of memory 
for use as the OMAP1 camera dedicated dma coherent memory. Use this 
functionality to avoid the camera driver switching form videobuf_contig mode 
to less efficient videobuf_sg mode in case of dma coherent memory allocation 
failure after system memory gets fragmented.

Created and tested against linux-2.6.27-rc4 on top of patch 1/2.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
 arch/arm/mach-omap1/board-ams-delta.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

--- linux-2.6.37-rc4/arch/arm/mach-omap1/board-ams-delta.c.orig	2010-12-04 18:05:25.000000000 +0100
+++ linux-2.6.37-rc4/arch/arm/mach-omap1/board-ams-delta.c	2010-12-04 22:19:39.000000000 +0100
@@ -262,6 +262,16 @@ static struct omap1_cam_platform_data am
 	.lclk_khz_max	= 1334,		/* results in 5fps CIF, 10fps QCIF */
 };
 
+void __init amsdelta_reserve(void)
+{
+#if defined(CONFIG_VIDEO_OMAP1) || defined(CONFIG_VIDEO_OMAP1_MODULE)
+	omap1_camera_reserve(PAGE_SIZE << get_order(352 * 288 * 2 *
+			OMAP1_CAMERA_MIN_BUF_COUNT(OMAP1_CAM_DMA_CONTIG)));
+#endif
+
+	omap_reserve();
+}
+
 static struct platform_device *ams_delta_devices[] __initdata = {
 	&ams_delta_kp_device,
 	&ams_delta_lcd_device,
@@ -366,7 +376,7 @@ MACHINE_START(AMS_DELTA, "Amstrad E3 (De
 	/* Maintainer: Jonathan McDowell <noodles@earth.li> */
 	.boot_params	= 0x10000100,
 	.map_io		= ams_delta_map_io,
-	.reserve	= omap_reserve,
+	.reserve	= amsdelta_reserve,
 	.init_irq	= ams_delta_init_irq,
 	.init_machine	= ams_delta_init,
 	.timer		= &omap_timer,