| Message ID | 1388013159-3036-1-git-send-email-ivo.g.dimitrov.75@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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 -- 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
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
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
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 -- 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
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
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