Re: OMAPFB: CMA allocation failures

Re: OMAPFB: CMA allocation failures

[Ivajlo Dimitrov]

Ping?

----- Original Message ----- 
From: "Ивайло Димитров" <freemangordon@abv.bg>
To: "Tomi Valkeinen" <tomi.valkeinen@ti.com>
Cc: <minchan@kernel.org>; <pavel@ucw.cz>; <sre@debian.org>; 
<pali.rohar@gmail.com>; <pc+n900@asdf.org>; <linux-kernel@vger.kernel.org>; 
<linux-mm@kvack.org>
Sent: Tuesday, November 05, 2013 9:55 PM
Subject: Re: OMAPFB: CMA allocation failures


>
>
>
>
>
> >-------- Оригинално писмо --------
> >От:  Tomi Valkeinen
> >Относно: Re: OMAPFB: CMA allocation failures
> >До: Ивайло Димитров
> >Изпратено на: Сряда, 2013, Октомври 30 14:19:32 EET
> >
> >I really dislike the idea of adding the omap vram allocator back. Then
> >again, if the CMA doesn't work, something has to be done.
> >
>
> If I got Minchan Kim's explanation correctly, CMA simply can't be used
> for allocation of framebuffer memory, because it is unreliable.
>
> >Pre-allocating is possible, but that won't work if there's any need to
> >re-allocating the framebuffers. Except if the omapfb would retain and
> >manage the pre-allocated buffers, but that would just be more or less
> >the old vram allocator again.
> >
> >So, as I see it, the best option would be to have the standard dma_alloc
> >functions get the memory for omapfb from a private pool, which is not
> >used for anything else.
> >
> >I wonder if that's possible already? It sounds quite trivial to me.
>
> dma_alloc functions use either CMA or (iirc) get_pages_exact if CMA is
> disabled. Both of those fail easily. AFAIK there are several
> implementations with similar functionality, like CMEM and ION but
> (correct me if I am wrong) neither of them is upstreamed. In the
> current kernel I don't see anything that can be used for the purpose
> of reliable allocation of big chunks of contiguous memory.
> So, something should be done, but honestly, I can't think of anything
> but bringing VRAM allocator back. Not that I like the idea of bringing
> back ~700 lines of code, but I see no other option if omapfb driver is
> to be actually useful.
>
> Regards,
> Ivo 

Re: OMAPFB: CMA allocation failures

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4918 bytes --]

On 2013-11-30 12:00, Ivajlo Dimitrov wrote:
> Ping?
> 
> ----- Original Message ----- From: "Ивайло Димитров" <freemangordon@abv.bg>
> To: "Tomi Valkeinen" <tomi.valkeinen@ti.com>
> Cc: <minchan@kernel.org>; <pavel@ucw.cz>; <sre@debian.org>;
> <pali.rohar@gmail.com>; <pc+n900@asdf.org>;
> <linux-kernel@vger.kernel.org>; <linux-mm@kvack.org>
> Sent: Tuesday, November 05, 2013 9:55 PM
> Subject: Re: OMAPFB: CMA allocation failures
> 
> 
>>
>>
>>
>>
>>
>> >-------- Оригинално писмо --------
>> >От:  Tomi Valkeinen
>> >Относно: Re: OMAPFB: CMA allocation failures
>> >До: Ивайло Димитров
>> >Изпратено на: Сряда, 2013, Октомври 30 14:19:32 EET
>> >
>> >I really dislike the idea of adding the omap vram allocator back. Then
>> >again, if the CMA doesn't work, something has to be done.
>> >
>>
>> If I got Minchan Kim's explanation correctly, CMA simply can't be used
>> for allocation of framebuffer memory, because it is unreliable.

Well. All memory allocation is unreliable. And
include/linux/dma-contiguous.h even clearly states that CMA is something
to be used in cases like omapfb.

>> >Pre-allocating is possible, but that won't work if there's any need to
>> >re-allocating the framebuffers. Except if the omapfb would retain and
>> >manage the pre-allocated buffers, but that would just be more or less
>> >the old vram allocator again.
>> >
>> >So, as I see it, the best option would be to have the standard dma_alloc
>> >functions get the memory for omapfb from a private pool, which is not
>> >used for anything else.
>> >
>> >I wonder if that's possible already? It sounds quite trivial to me.
>>
>> dma_alloc functions use either CMA or (iirc) get_pages_exact if CMA is
>> disabled. Both of those fail easily. AFAIK there are several
>> implementations with similar functionality, like CMEM and ION but
>> (correct me if I am wrong) neither of them is upstreamed. In the
>> current kernel I don't see anything that can be used for the purpose
>> of reliable allocation of big chunks of contiguous memory.
>> So, something should be done, but honestly, I can't think of anything
>> but bringing VRAM allocator back. Not that I like the idea of bringing
>> back ~700 lines of code, but I see no other option if omapfb driver is
>> to be actually useful.

How about the patch below? If I'm not mistaken (and I might) it reserves
separate memory area for omapfb, which is not used by CMA.

If it works, it should be extended to get the parameters via kernel
cmdline, and use that alloc only if the user requests it.


diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
index 2dabb9ecb986..9beecded0380 100644
--- a/arch/arm/mach-omap2/common.c
+++ b/arch/arm/mach-omap2/common.c
@@ -33,4 +33,5 @@ void __init omap_reserve(void)
 	omap_dsp_reserve_sdram_memblock();
 	omap_secure_ram_reserve_memblock();
 	omap_barrier_reserve_memblock();
+	omap_fb_reserve_memblock();
 }
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 48e9cd34cae0..874786f05ec3 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -40,6 +40,8 @@

 #include "usb.h"

+void __init omap_fb_reserve_memblock(void);
+
 #define OMAP_INTC_START		NR_IRQS

 #if defined(CONFIG_PM) && defined(CONFIG_ARCH_OMAP2)
diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c
index 26e28e94f625..8f339e88c7cd 100644
--- a/arch/arm/mach-omap2/fb.c
+++ b/arch/arm/mach-omap2/fb.c
@@ -30,9 +30,11 @@
 #include <linux/dma-mapping.h>

 #include <asm/mach/map.h>
+#include <asm/memblock.h>

 #include "soc.h"
 #include "display.h"
+#include "common.h"

 #ifdef CONFIG_OMAP2_VRFB

@@ -106,9 +108,41 @@ static struct platform_device omap_fb_device = {
 	.num_resources = 0,
 };

+static phys_addr_t omapfb_mem_base __initdata;
+static phys_addr_t omapfb_mem_size __initdata;
+
+void __init omap_fb_reserve_memblock(void)
+{
+	omapfb_mem_size = ALIGN(1920*1200*4*3, SZ_2M);
+	omapfb_mem_base = arm_memblock_steal(omapfb_mem_size, SZ_2M);
+	if (omapfb_mem_base)
+		pr_info("omapfb: reserved %u bytes at %x\n",
+			omapfb_mem_size, omapfb_mem_base);
+	else
+		pr_err("omapfb: arm_memblock_steal failed\n");
+}
+
 int __init omap_init_fb(void)
 {
-	return platform_device_register(&omap_fb_device);
+	int r;
+	int dma;
+
+	r = platform_device_register(&omap_fb_device);
+	if (r)
+		return r;
+
+	if (!omapfb_mem_base)
+		return 0;
+
+	dma = dma_declare_coherent_memory(&omap_fb_device.dev,
+		omapfb_mem_base, omapfb_mem_base,
+		omapfb_mem_size,
+		DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE);
+
+	if (!(dma & DMA_MEMORY_MAP))
+		pr_err("omapfb: dma_declare_coherent_memory failed\n");
+
+	return 0;
 }
 #else
 int __init omap_init_fb(void) { return 0; }



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: OMAPFB: CMA allocation failures

[Ivajlo Dimitrov]

On 05.12.2013 13:25, Tomi Valkeinen wrote:
> How about the patch below? If I'm not mistaken (and I might) it reserves
> separate memory area for omapfb, which is not used by CMA.
>
> If it works, it should be extended to get the parameters via kernel
> cmdline, and use that alloc only if the user requests it.
>

YAY!!! That one seems to fix the issue.

Though I had to revert 7faa92339bbb1e6b9a80983b206642517327eb75 (well, I 
hacked check_horiz_timing_omap3 to always return 0). Otherwise I have 
"omapdss DISPC error: horizontal timing too tight" error when try to 
play anything above 320x240 or so. I'll look at the issue.

Regards,
Ivo

[PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

From: Ivaylo Dimitrov <freemangordon@abv.bg>

On memory limited devices, CMA fails easily when asked to allocate big
chunks of memory like framebuffer memory needed for video playback.

Add boot parameter "omapfb_memsize" which allocates memory to be used
as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator when
trying to allocate memory for the framebuffers

Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>
---
 arch/arm/mach-omap2/common.c |    1 +
 arch/arm/mach-omap2/common.h |    2 +
 arch/arm/mach-omap2/fb.c     |   46 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
index 2dabb9e..9beecde 100644
--- a/arch/arm/mach-omap2/common.c
+++ b/arch/arm/mach-omap2/common.c
@@ -33,4 +33,5 @@ void __init omap_reserve(void)
 	omap_dsp_reserve_sdram_memblock();
 	omap_secure_ram_reserve_memblock();
 	omap_barrier_reserve_memblock();
+	omap_fb_reserve_memblock();
 }
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index e30ef67..21afdc0 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -304,6 +304,8 @@ extern void omap_reserve(void);
 struct omap_hwmod;
 extern int omap_dss_reset(struct omap_hwmod *);
 
+extern void omap_fb_reserve_memblock(void);
+
 /* SoC specific clock initializer */
 extern int (*omap_clk_init)(void);
 
diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c
index 26e28e9..0eacbe9 100644
--- a/arch/arm/mach-omap2/fb.c
+++ b/arch/arm/mach-omap2/fb.c
@@ -30,6 +30,7 @@
 #include <linux/dma-mapping.h>
 
 #include <asm/mach/map.h>
+#include <asm/memblock.h>
 
 #include "soc.h"
 #include "display.h"
@@ -106,10 +107,53 @@ static struct platform_device omap_fb_device = {
 	.num_resources = 0,
 };
 
+static phys_addr_t omapfb_mem_base __initdata;
+static phys_addr_t omapfb_mem_size __initdata;
+
+void __init omap_fb_reserve_memblock(void)
+{
+	if (omapfb_mem_size) {
+		omapfb_mem_base = arm_memblock_steal(omapfb_mem_size, SZ_1M);
+		if (omapfb_mem_base)
+			pr_info("omapfb: reserved %u bytes at %x\n",
+				omapfb_mem_size, omapfb_mem_base);
+		else
+			pr_err("omapfb: arm_memblock_steal failed\n");
+	}
+}
+
 int __init omap_init_fb(void)
 {
-	return platform_device_register(&omap_fb_device);
+	int ret;
+
+	ret = platform_device_register(&omap_fb_device);
+
+	if (ret)
+		return ret;
+
+	if (!omapfb_mem_base)
+		return 0;
+
+	ret = dma_declare_coherent_memory(&omap_fb_device.dev,
+					  omapfb_mem_base, omapfb_mem_base,
+					  omapfb_mem_size, DMA_MEMORY_MAP |
+					  DMA_MEMORY_EXCLUSIVE);
+	if (!(ret & DMA_MEMORY_MAP))
+		pr_err("omapfb: dma_declare_coherent_memory failed\n");
+
+	return 0;
+}
+
+static int __init early_omapfb_memsize(char *p)
+{
+	omapfb_mem_size = ALIGN(memparse(p, &p), SZ_1M);
+
+	if(!omapfb_mem_size)
+		pr_err("omapfb: bad memsize parameter\n");
+
+	return 0;
 }
+early_param("omapfb_memsize", early_omapfb_memsize);
 #else
 int __init omap_init_fb(void) { return 0; }
 #endif
-- 
1.5.6.1

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Pavel Machek]

On Thu 2013-12-26 01:12:39, Ivaylo Dimitrov wrote:
> From: Ivaylo Dimitrov <freemangordon@abv.bg>
> 
> On memory limited devices, CMA fails easily when asked to allocate big
> chunks of memory like framebuffer memory needed for video playback.
> 
> Add boot parameter "omapfb_memsize" which allocates memory to be used
> as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator when
> trying to allocate memory for the framebuffers
> 
> Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>

Hmm, would it make sense to add a parameter to reserve certain ammount
of memory for CMA? omapfb is probably not the only user hitting
this...?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

On 27.12.2013 11:48, Pavel Machek wrote:
> On Thu 2013-12-26 01:12:39, Ivaylo Dimitrov wrote:
>> From: Ivaylo Dimitrov <freemangordon@abv.bg>
>>
>> On memory limited devices, CMA fails easily when asked to allocate big
>> chunks of memory like framebuffer memory needed for video playback.
>>
>> Add boot parameter "omapfb_memsize" which allocates memory to be used
>> as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator when
>> trying to allocate memory for the framebuffers
>>
>> Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>
> Hmm, would it make sense to add a parameter to reserve certain ammount
> of memory for CMA? omapfb is probably not the only user hitting
> this...?
> 									Pavel

But that would mean that one must have CMA enabled to use that 
functionality and it is an additional memory overhead. Also, I don't 
think we will have much of a benefit of that - for video playback we'll 
still have to preallocate the same amount of RAM as now - but with the 
additional overhead of page migration when that RAM becomes needed by 
DSP and OMAPFB. However, even if such functionality is someday 
implemented in CMA, it doesn't conflict with the proposed patch - by 
simply not preallocating memory for omapfb, one will automatically use it.

BTW there is CMEM driver (not upstreamed afaik) which does exactly that 
- it manages a contiguous ("stolen")memory pool. No idea how easy it 
would be to merge CMEM into CMA. Neither I am the right guy for the 
task, IMO :)

Ivo

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

On 26.12.2013 01:12, Ivaylo Dimitrov wrote:
> From: Ivaylo Dimitrov <freemangordon@abv.bg>
>
> On memory limited devices, CMA fails easily when asked to allocate big
> chunks of memory like framebuffer memory needed for video playback.
>
> Add boot parameter "omapfb_memsize" which allocates memory to be used
> as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator when
> trying to allocate memory for the framebuffers
>
> Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>
> ---
>   arch/arm/mach-omap2/common.c |    1 +
>   arch/arm/mach-omap2/common.h |    2 +
>   arch/arm/mach-omap2/fb.c     |   46 +++++++++++++++++++++++++++++++++++++++++-
>   3 files changed, 48 insertions(+), 1 deletions(-)
>

ping

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3546 bytes --]

Hi Tomi! Can you review this patch? It is waiting here for two years!

On Thursday 26 December 2013 00:12:39 Ivaylo Dimitrov wrote:
> From: Ivaylo Dimitrov <freemangordon@abv.bg>
> 
> On memory limited devices, CMA fails easily when asked to allocate
> big chunks of memory like framebuffer memory needed for video
> playback.
> 
> Add boot parameter "omapfb_memsize" which allocates memory to be used
> as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator
> when trying to allocate memory for the framebuffers
> 
> Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>
> ---
>  arch/arm/mach-omap2/common.c |    1 +
>  arch/arm/mach-omap2/common.h |    2 +
>  arch/arm/mach-omap2/fb.c     |   46
> +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 48
> insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/common.c
> b/arch/arm/mach-omap2/common.c index 2dabb9e..9beecde 100644
> --- a/arch/arm/mach-omap2/common.c
> +++ b/arch/arm/mach-omap2/common.c
> @@ -33,4 +33,5 @@ void __init omap_reserve(void)
>  	omap_dsp_reserve_sdram_memblock();
>  	omap_secure_ram_reserve_memblock();
>  	omap_barrier_reserve_memblock();
> +	omap_fb_reserve_memblock();
>  }
> diff --git a/arch/arm/mach-omap2/common.h
> b/arch/arm/mach-omap2/common.h index e30ef67..21afdc0 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -304,6 +304,8 @@ extern void omap_reserve(void);
>  struct omap_hwmod;
>  extern int omap_dss_reset(struct omap_hwmod *);
> 
> +extern void omap_fb_reserve_memblock(void);
> +
>  /* SoC specific clock initializer */
>  extern int (*omap_clk_init)(void);
> 
> diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c
> index 26e28e9..0eacbe9 100644
> --- a/arch/arm/mach-omap2/fb.c
> +++ b/arch/arm/mach-omap2/fb.c
> @@ -30,6 +30,7 @@
>  #include <linux/dma-mapping.h>
> 
>  #include <asm/mach/map.h>
> +#include <asm/memblock.h>
> 
>  #include "soc.h"
>  #include "display.h"
> @@ -106,10 +107,53 @@ static struct platform_device omap_fb_device =
> { .num_resources = 0,
>  };
> 
> +static phys_addr_t omapfb_mem_base __initdata;
> +static phys_addr_t omapfb_mem_size __initdata;
> +
> +void __init omap_fb_reserve_memblock(void)
> +{
> +	if (omapfb_mem_size) {
> +		omapfb_mem_base = arm_memblock_steal(omapfb_mem_size, SZ_1M);
> +		if (omapfb_mem_base)
> +			pr_info("omapfb: reserved %u bytes at %x\n",
> +				omapfb_mem_size, omapfb_mem_base);
> +		else
> +			pr_err("omapfb: arm_memblock_steal failed\n");
> +	}
> +}
> +
>  int __init omap_init_fb(void)
>  {
> -	return platform_device_register(&omap_fb_device);
> +	int ret;
> +
> +	ret = platform_device_register(&omap_fb_device);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (!omapfb_mem_base)
> +		return 0;
> +
> +	ret = dma_declare_coherent_memory(&omap_fb_device.dev,
> +					  omapfb_mem_base, omapfb_mem_base,
> +					  omapfb_mem_size, DMA_MEMORY_MAP |
> +					  DMA_MEMORY_EXCLUSIVE);
> +	if (!(ret & DMA_MEMORY_MAP))
> +		pr_err("omapfb: dma_declare_coherent_memory failed\n");
> +
> +	return 0;
> +}
> +
> +static int __init early_omapfb_memsize(char *p)
> +{
> +	omapfb_mem_size = ALIGN(memparse(p, &p), SZ_1M);
> +
> +	if(!omapfb_mem_size)
> +		pr_err("omapfb: bad memsize parameter\n");
> +
> +	return 0;
>  }
> +early_param("omapfb_memsize", early_omapfb_memsize);
>  #else
>  int __init omap_init_fb(void) { return 0; }
>  #endif

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

Hi,

On 01/01/16 14:01, Pali Rohár wrote:
> Hi Tomi! Can you review this patch? It is waiting here for two years!
> 
> On Thursday 26 December 2013 00:12:39 Ivaylo Dimitrov wrote:
>> From: Ivaylo Dimitrov <freemangordon@abv.bg>
>>
>> On memory limited devices, CMA fails easily when asked to allocate
>> big chunks of memory like framebuffer memory needed for video
>> playback.
>>
>> Add boot parameter "omapfb_memsize" which allocates memory to be used
>> as dma coherent memory, so dma_alloc_attrs won't hit CMA allocator
>> when trying to allocate memory for the framebuffers

We probably need exactly the same for omapdrm, as omapfb is on the way
to being deprecated. And sounds to me that we probably need similar for
other devices which try to do large allocations (camera? video decoders?).

So I really think this should be somehow be a general option for any device.

I also wonder if CMA can be improved to not need anything like this? If
you just increase the CMA area, won't that increase the chances CMA will
work?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

Hi Tomi,

On  4.01.2016 13:37, Tomi Valkeinen wrote:
>
> We probably need exactly the same for omapdrm, as omapfb is on the way
> to being deprecated. And sounds to me that we probably need similar for
> other devices which try to do large allocations (camera? video decoders?).
>

Re omapdrm - I guess it wouldn't be hard for omapdrm to use the same 
preallocated memory, when/if it is needed. Though I know nothing about 
omapdrm, so can't really tell.

If not mistaken, camera driver uses sg lists. DSP needs such a memory, 
but anyway it(driver) was removed from mainline, with no signs/hope to 
be returned anytime soon.

> So I really think this should be somehow be a general option for any device.
>

Then maybe add the relevant people in CC, so we to start some kind of 
discussion. But until such a general option exists, I think it makes 
sense to apply the $subject patch, we can easily fix it to use whatever 
general purpose API might the discussion come up with. As it is now, 
omapfb simply cannot be used to play any video with sane resolution 
(without preallocated memory that is), unless this is the only thing the 
device does. And even then it is not assured.

> I also wonder if CMA can be improved to not need anything like this? If
> you just increase the CMA area, won't that increase the chances CMA will
> work?
>

The short answer is no, at least not with the CMA code currently 
upstream. A kind of a long answer could be found on 
http://marc.info/?l=linux-mm&m=141571797202006&w=2

Regards,
Ivo

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]


On 04/01/16 15:04, Ivaylo Dimitrov wrote:
> Hi Tomi,
> 
> On  4.01.2016 13:37, Tomi Valkeinen wrote:
>>
>> We probably need exactly the same for omapdrm, as omapfb is on the way
>> to being deprecated. And sounds to me that we probably need similar for
>> other devices which try to do large allocations (camera? video
>> decoders?).
>>
> 
> Re omapdrm - I guess it wouldn't be hard for omapdrm to use the same
> preallocated memory, when/if it is needed. Though I know nothing about
> omapdrm, so can't really tell.
> 
> If not mistaken, camera driver uses sg lists. DSP needs such a memory,
> but anyway it(driver) was removed from mainline, with no signs/hope to
> be returned anytime soon.

I don't know about omap3 (if that's what you're talking about), but
generally, I think it depends very much on the IPs used. I don't think
all capture IPs support sg.

>> So I really think this should be somehow be a general option for any
>> device.
>>
> 
> Then maybe add the relevant people in CC, so we to start some kind of
> discussion. But until such a general option exists, I think it makes
> sense to apply the $subject patch, we can easily fix it to use whatever
> general purpose API might the discussion come up with. As it is now,
> omapfb simply cannot be used to play any video with sane resolution
> (without preallocated memory that is), unless this is the only thing the
> device does. And even then it is not assured.

The patch itself looks fine to me, and I have no problem adding
temporary code to help solve use cases. Except when they add new
userspace APIs, which is what's done here. I've been bitten too many
times by an userspace API that I need to maintain forever, making new
development difficult. That's the reason I'm (maybe overly) cautious here.

I also want to point out that the patch was posted two years ago. And
now there's a ping for the first time. It cannot be a huge problem to a
lot of people.

Adding to that is the fact that omapfb is now in maintenance mode, and
all new development is done for omapdrm.

So, I'm not very enthusiastic about adding this feature as an omapfb
specific boot parameter.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

Hi Tomi,

On 11.01.2016 20:34, Tomi Valkeinen wrote:
>
> So, I'm not very enthusiastic about adding this feature as an omapfb
> specific boot parameter.
>

What about something like (not properly formatted, just want your 
opinion on the idea):

diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c
index 1f1ecf8..0d109d8 100644
--- a/arch/arm/mach-omap2/fb.c
+++ b/arch/arm/mach-omap2/fb.c
@@ -28,6 +28,7 @@
  #include <linux/io.h>
  #include <linux/omapfb.h>
  #include <linux/dma-mapping.h>
+#include <linux/of_reserved_mem.h>

  #include <asm/mach/map.h>

@@ -110,6 +111,49 @@ int __init omap_init_fb(void)
  {
         return platform_device_register(&omap_fb_device);
  }
+
+static int rmem_omapfb_device_init(struct reserved_mem *rmem, struct 
device *dev)
+{
+       int dma;
+
+       if (rmem->priv)
+               return 0;
+
+       dma = dma_declare_coherent_memory(&omap_fb_device.dev, rmem->base,
+                                         rmem->base, rmem->size,
+                                         DMA_MEMORY_MAP |
+                                         DMA_MEMORY_EXCLUSIVE);
+
+       if (!(dma & DMA_MEMORY_MAP)) {
+               pr_err("omapfb: dma_declare_coherent_memory failed\n");
+               return -ENOMEM;
+       }
+       else
+               rmem->priv = omap_fb_device.dev.dma_mem;
+
+       return 0;
+}
+
+static void rmem_omapfb_device_release(struct reserved_mem *rmem,
+                                      struct device *dev)
+{
+       dma_release_declared_memory(&omap_fb_device.dev);
+}
+
+static const struct reserved_mem_ops rmem_omapfb_ops = {
+       .device_init    = rmem_omapfb_device_init,
+       .device_release = rmem_omapfb_device_release,
+};
+
+static int __init rmem_omapfb_setup(struct reserved_mem *rmem)
+{
+       rmem->ops = &rmem_omapfb_ops;
+       pr_info("omapfb: reserved %d bytes at %pa\n", rmem->size, 
&rmem->base);
+
+       return 0;
+}
+
+RESERVEDMEM_OF_DECLARE(dss, "ti,omapfb-memsize", rmem_omapfb_setup);
  #else
  int __init omap_init_fb(void) { return 0; }
  #endif

diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 6ab13d1..6f0ba03 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -28,6 +28,7 @@
  #include <linux/slab.h>
  #include <linux/mfd/syscon.h>
  #include <linux/regmap.h>
+#include <linux/of_reserved_mem.h>

  #include <video/omapdss.h>
  #include "omap_hwmod.h"
@@ -640,6 +641,7 @@ int __init omapdss_init_of(void)
         omap_display_device.dev.platform_data = &board_data;

         r = platform_device_register(&omap_display_device);
+
         if (r < 0) {
                 pr_err("Unable to register omapdss device\n");
                 return r;
@@ -666,6 +668,9 @@ int __init omapdss_init_of(void)
                 return r;
         }

+       /* Init fb reserved memory, there may be none so ignore the 
result */
+       of_reserved_mem_device_init(&pdev->dev);
+
         /* create V4L2 display device */
         r = omap_init_vout();
         if (r < 0) {


Regards,
Ivo

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]



On 13/02/16 09:25, Ivaylo Dimitrov wrote:
> Hi Tomi,
> 
> On 11.01.2016 20:34, Tomi Valkeinen wrote:
>>
>> So, I'm not very enthusiastic about adding this feature as an omapfb
>> specific boot parameter.
>>
> 
> What about something like (not properly formatted, just want your
> opinion on the idea):
> 
> diff --git a/arch/arm/mach-omap2/fb.c b/arch/arm/mach-omap2/fb.c
> index 1f1ecf8..0d109d8 100644
> --- a/arch/arm/mach-omap2/fb.c
> +++ b/arch/arm/mach-omap2/fb.c
> @@ -28,6 +28,7 @@
>  #include <linux/io.h>
>  #include <linux/omapfb.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/of_reserved_mem.h>
> 
>  #include <asm/mach/map.h>
> 
> @@ -110,6 +111,49 @@ int __init omap_init_fb(void)
>  {
>         return platform_device_register(&omap_fb_device);
>  }
> +
> +static int rmem_omapfb_device_init(struct reserved_mem *rmem, struct
> device *dev)
> +{
> +       int dma;
> +
> +       if (rmem->priv)
> +               return 0;
> +
> +       dma = dma_declare_coherent_memory(&omap_fb_device.dev, rmem->base,
> +                                         rmem->base, rmem->size,
> +                                         DMA_MEMORY_MAP |
> +                                         DMA_MEMORY_EXCLUSIVE);
> +
> +       if (!(dma & DMA_MEMORY_MAP)) {
> +               pr_err("omapfb: dma_declare_coherent_memory failed\n");
> +               return -ENOMEM;
> +       }
> +       else
> +               rmem->priv = omap_fb_device.dev.dma_mem;
> +
> +       return 0;
> +}
> +
> +static void rmem_omapfb_device_release(struct reserved_mem *rmem,
> +                                      struct device *dev)
> +{
> +       dma_release_declared_memory(&omap_fb_device.dev);
> +}
> +
> +static const struct reserved_mem_ops rmem_omapfb_ops = {
> +       .device_init    = rmem_omapfb_device_init,
> +       .device_release = rmem_omapfb_device_release,
> +};
> +
> +static int __init rmem_omapfb_setup(struct reserved_mem *rmem)
> +{
> +       rmem->ops = &rmem_omapfb_ops;
> +       pr_info("omapfb: reserved %d bytes at %pa\n", rmem->size,
> &rmem->base);
> +
> +       return 0;
> +}
> +
> +RESERVEDMEM_OF_DECLARE(dss, "ti,omapfb-memsize", rmem_omapfb_setup);
>  #else
>  int __init omap_init_fb(void) { return 0; }
>  #endif
> 
> diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
> index 6ab13d1..6f0ba03 100644
> --- a/arch/arm/mach-omap2/display.c
> +++ b/arch/arm/mach-omap2/display.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/of_reserved_mem.h>
> 
>  #include <video/omapdss.h>
>  #include "omap_hwmod.h"
> @@ -640,6 +641,7 @@ int __init omapdss_init_of(void)
>         omap_display_device.dev.platform_data = &board_data;
> 
>         r = platform_device_register(&omap_display_device);
> +
>         if (r < 0) {
>                 pr_err("Unable to register omapdss device\n");
>                 return r;
> @@ -666,6 +668,9 @@ int __init omapdss_init_of(void)
>                 return r;
>         }
> 
> +       /* Init fb reserved memory, there may be none so ignore the
> result */
> +       of_reserved_mem_device_init(&pdev->dev);
> +

Does it work for you? I haven't used DT reserved-memory, do you have an
example .dts change?

Now, having to support DT bindings is not any better than supporting
cmdline options. But with a quick read of reserved-memory.txt I like the
idea. However we should have "reserved memory for display", not for
omapfb, so that the same reserved area could be used by omapdrm too.

Another thing, with v4.5, omapfb has moved into maintenance mode. I
don't want to merge new features there. Are you planning to move to
omapdrm, and if not, why? I'd rather see all this done for omapdrm only.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Pali Rohár]

On Tuesday 16 February 2016 15:51:34 Tomi Valkeinen wrote:
> Another thing, with v4.5, omapfb has moved into maintenance mode. I
> don't want to merge new features there. Are you planning to move to
> omapdrm, and if not, why? I'd rather see all this done for omapdrm only.

Hi! We are using (patched) Maemo and for OpenGL ES support we need SGX
PVR graphics kernel driver which depends on omapfb :-( Because Maemo
needs OpenGL ES we needs omapfb driver. Kernel part of SGX PVR driver is
open source (GPL) but there is no manpower to port it to omapdrm...

So if we want to use (patched) Maemo (which I really want) we need to
stay on omapfb for now...

And there are no full open source drivers for SGX PVR graphic chip which
would work on TI omap3 Nokia N900 device...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] ARM: omapfb: Add early framebuffer memory allocator

[Ivaylo Dimitrov]

Hi,

On 16.02.2016 15:51, Tomi Valkeinen wrote:
>
> Does it work for you? I haven't used DT reserved-memory, do you have an
> example .dts change?
>

Yes, it does work, I tested it on n900:

diff --git a/arch/arm/boot/dts/omap3-n900.dts 
b/arch/arm/boot/dts/omap3-n900.dts
index 1e94237..863d547 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -59,6 +59,18 @@
                 reg = <0x80000000 0x10000000>; /* 256 MB */
         };

+       reserved-memory {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               ranges;
+
+               omapfb_reserved: omapfb {
+                       size = <0x700000>;
+                       alignment = <0x100000>;
+                       compatible = "ti,omapfb-memsize";
+               };
+       };
+
         gpio_keys {
                 compatible = "gpio-keys";

@@ -1083,6 +1095,8 @@

         vdds_sdi-supply = <&vaux1>;

+       memory-region = <&omapfb_reserved>;
+
         ports {
                 #address-cells = <1>;
                 #size-cells = <0>;

> Now, having to support DT bindings is not any better than supporting
> cmdline options. But with a quick read of reserved-memory.txt I like the
> idea. However we should have "reserved memory for display", not for
> omapfb, so that the same reserved area could be used by omapdrm too.

Sounds reasonable and I don't really care how it is to be called or who 
does the actual reservation, as long as there is some reserved memory we 
can use for omapfb :)

Keep in mind that the changes I did were just a quick-n-dirty hack to 
see if it will work and if you will accept something like that. A better 
approach is maybe to move RESERVEDMEM_OF_DECLARE() and co to display.c 
and pass base and size to whoever needs them (be it omapfb or omapdrm). 
Also, compatible could be called "ti,dss-memsize" or the like, but those 
are cosmetics IMO.

>
> Another thing, with v4.5, omapfb has moved into maintenance mode. I
> don't want to merge new features there. Are you planning to move to
> omapdrm, and if not, why? I'd rather see all this done for omapdrm only.

I don't see a reason to not merge a small change like that in omapfb if 
there is reserved display memory used by omapdrm, but still, I am not 
the maintainer.

Pali already explained the situation with PVR driver we use to boot 
maemo UI. Honestly, I have no idea what it takes to move from omapfb to 
omapdrm. Any hints?

Ivo