RE: [PATCH 3/3] davinci: add SRAM allocator
Just a quick "can we talk through this patch a bit" note... I _think_ there are existing codecs on some DaVinci platforms (e.g. DM365) that are already using this SRAM memory. The current [easy] solution to their use case was to keep this memory out of the kernel, and simply give it to CMEM to manage (via its insmod params) and the codecs indirectly allocate from CMEM. If you're unfamiliar with CMEM, there is an overview here: http://tiexpressdsp.com/index.php?title=CMEM_Overview I'm not the expert, I only want to encourage further discussion! But I'm a little concerned about a resource collision if this SRAM allocator gets pushed, and more than one client thinks they own that resource. Chris > -Original Message- > From: davinci-linux-open-source-boun...@linux.davincidsp.com > [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com > ] On Behalf Of Kevin Hilman > Sent: Thursday, May 21, 2009 2:47 PM > To: linux-arm-ker...@lists.arm.linux.org.uk > Cc: davinci-linux-open-source@linux.davincidsp.com; David Brownell > Subject: [PATCH 3/3] davinci: add SRAM allocator > > From: David Brownell > > Provide a generic SRAM allocator using genalloc, and vaguely > modeled after what AVR32 uses. This builds on top of the > static CPU mapping set up in the previous patch, and returns > DMA mappings as requested (if possible). > > Compared to its OMAP cousin, there's no current support for > (currently non-existent) DaVinci power management code running > in SRAM; and this has ways to deallocate, instead of being > allocate-only. > > The initial user of this should probably be the audio code, > because EDMA from DDR is subject to various dropouts on at > least DM355 and DM6446 chips. > > Signed-off-by: David Brownell > Signed-off-by: Kevin Hilman > --- > arch/arm/Kconfig |1 + > arch/arm/mach-davinci/Makefile|2 +- > arch/arm/mach-davinci/include/mach/sram.h | 27 ++ > arch/arm/mach-davinci/sram.c | 74 > + > 4 files changed, 103 insertions(+), 1 deletions(-) > create mode 100644 arch/arm/mach-davinci/include/mach/sram.h > create mode 100644 arch/arm/mach-davinci/sram.c > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index e60ec54..ffa8427 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -586,6 +586,7 @@ config ARCH_DAVINCI > select ZONE_DMA > select HAVE_IDE > select COMMON_CLKDEV > + select GENERIC_ALLOCATOR > help > Support for TI's DaVinci platform. > > diff --git a/arch/arm/mach-davinci/Makefile > b/arch/arm/mach-davinci/Makefile > index 5b62d8a..059ab78 100644 > --- a/arch/arm/mach-davinci/Makefile > +++ b/arch/arm/mach-davinci/Makefile > @@ -5,7 +5,7 @@ > > # Common objects > obj-y:= time.o clock.o serial.o io.o psc.o \ > -gpio.o devices.o dma.o usb.o common.o > +gpio.o devices.o dma.o usb.o common.o sram.o > > obj-$(CONFIG_DAVINCI_MUX)+= mux.o > > diff --git a/arch/arm/mach-davinci/include/mach/sram.h > b/arch/arm/mach-davinci/include/mach/sram.h > new file mode 100644 > index 000..111f7cc > --- /dev/null > +++ b/arch/arm/mach-davinci/include/mach/sram.h > @@ -0,0 +1,27 @@ > +/* > + * mach/sram.h - DaVinci simple SRAM allocator > + * > + * Copyright (C) 2009 David Brownell > + * > + * This program is free software; you can redistribute it > and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#ifndef __MACH_SRAM_H > +#define __MACH_SRAM_H > + > +/* ARBITRARY: SRAM allocations are multiples of this 2^N size */ > +#define SRAM_GRANULARITY 512 > + > +/* > + * SRAM allocations return a CPU virtual address, or NULL on error. > + * If a DMA address is requested and the SRAM supports DMA, its > + * mapped address is also returned. > + * > + * Errors include SRAM memory not being available, and requesting > + * DMA mapped SRAM on systems which don't allow that. > + */ > +extern void *sram_alloc(size_t len, dma_addr_t *dma); > +extern void sram_free(void *addr, size_t len); > + > +#endif /* __MACH_SRAM_H */ > diff --git a/arch/arm/mach-davinci/sram.c > b/arch/arm/mach-davinci/sram.c > new file mode 100644 > index 000..0309e72 > --- /dev/null > +++ b/arch/arm/mach-davinci/sram.c > @@ -0,0 +1,74 @@ > +/* > + * mach-davinci/sram.c - DaVinci simple SRAM allocator > + * > + * Copyright (C) 2009 David Brownell > + * > + * This program is free software; you can redistribute it > and/or modify > + * it under the terms of the GNU General Public License as > published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > + > +static struct gen_pool *sram_pool; > + > +v
Re: [PATCH 3/3] davinci: add SRAM allocator
On Thursday 21 May 2009, Ring, Chris wrote: > I _think_ there are existing codecs on some DaVinci > platforms (e.g. DM365) that are already using this > SRAM memory. The current [easy] solution to their use > case was to keep this memory out of the kernel, and > simply give it to CMEM to manage (via its insmod > params) and the codecs indirectly allocate from CMEM. If you can find out the answer, that'd be good. If that's a real issue, it'll be trivial to add a "reserve this memory" call ... but nobody had brought up that issue in any previous incarnation of this code, which is why it's not there. IMO that could wait for DM365 support though. A somewhat similar use: maybe you're using a JTAG debugger like OpenOCD, and don't want to have to back up the data all the time. So you might want to just reserve some of it for those purposes. - Dave ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 3/3] davinci: add SRAM allocator
"Ring, Chris" writes: > Just a quick "can we talk through this patch a bit" note... > > I _think_ there are existing codecs on some DaVinci platforms > (e.g. DM365) that are already using this SRAM memory. The current > [easy] solution to their use case was to keep this memory out of the > kernel, and simply give it to CMEM to manage (via its insmod params) > and the codecs indirectly allocate from CMEM. Couldn't the CMEM driver just call sram_alloc() in this case? > If you're unfamiliar with CMEM, there is an overview here: > http://tiexpressdsp.com/index.php?title=CMEM_Overview > > I'm not the expert, I only want to encourage further discussion! > But I'm a little concerned about a resource collision if this SRAM > allocator gets pushed, and more than one client thinks they own that > resource. I would argue that these potentical collisions are all the more reason to push an allocator which can be used to avoid collisions. Kevin >> -Original Message- >> From: davinci-linux-open-source-boun...@linux.davincidsp.com >> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com >> ] On Behalf Of Kevin Hilman >> Sent: Thursday, May 21, 2009 2:47 PM >> To: linux-arm-ker...@lists.arm.linux.org.uk >> Cc: davinci-linux-open-source@linux.davincidsp.com; David Brownell >> Subject: [PATCH 3/3] davinci: add SRAM allocator >> >> From: David Brownell >> >> Provide a generic SRAM allocator using genalloc, and vaguely >> modeled after what AVR32 uses. This builds on top of the >> static CPU mapping set up in the previous patch, and returns >> DMA mappings as requested (if possible). >> >> Compared to its OMAP cousin, there's no current support for >> (currently non-existent) DaVinci power management code running >> in SRAM; and this has ways to deallocate, instead of being >> allocate-only. >> >> The initial user of this should probably be the audio code, >> because EDMA from DDR is subject to various dropouts on at >> least DM355 and DM6446 chips. >> >> Signed-off-by: David Brownell >> Signed-off-by: Kevin Hilman >> --- >> arch/arm/Kconfig |1 + >> arch/arm/mach-davinci/Makefile|2 +- >> arch/arm/mach-davinci/include/mach/sram.h | 27 ++ >> arch/arm/mach-davinci/sram.c | 74 >> + >> 4 files changed, 103 insertions(+), 1 deletions(-) >> create mode 100644 arch/arm/mach-davinci/include/mach/sram.h >> create mode 100644 arch/arm/mach-davinci/sram.c >> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> index e60ec54..ffa8427 100644 >> --- a/arch/arm/Kconfig >> +++ b/arch/arm/Kconfig >> @@ -586,6 +586,7 @@ config ARCH_DAVINCI >> select ZONE_DMA >> select HAVE_IDE >> select COMMON_CLKDEV >> +select GENERIC_ALLOCATOR >> help >>Support for TI's DaVinci platform. >> >> diff --git a/arch/arm/mach-davinci/Makefile >> b/arch/arm/mach-davinci/Makefile >> index 5b62d8a..059ab78 100644 >> --- a/arch/arm/mach-davinci/Makefile >> +++ b/arch/arm/mach-davinci/Makefile >> @@ -5,7 +5,7 @@ >> >> # Common objects >> obj-y := time.o clock.o serial.o io.o psc.o \ >> - gpio.o devices.o dma.o usb.o common.o >> + gpio.o devices.o dma.o usb.o common.o sram.o >> >> obj-$(CONFIG_DAVINCI_MUX) += mux.o >> >> diff --git a/arch/arm/mach-davinci/include/mach/sram.h >> b/arch/arm/mach-davinci/include/mach/sram.h >> new file mode 100644 >> index 000..111f7cc >> --- /dev/null >> +++ b/arch/arm/mach-davinci/include/mach/sram.h >> @@ -0,0 +1,27 @@ >> +/* >> + * mach/sram.h - DaVinci simple SRAM allocator >> + * >> + * Copyright (C) 2009 David Brownell >> + * >> + * This program is free software; you can redistribute it >> and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#ifndef __MACH_SRAM_H >> +#define __MACH_SRAM_H >> + >> +/* ARBITRARY: SRAM allocations are multiples of this 2^N size */ >> +#define SRAM_GRANULARITY512 >> + >> +/* >> + * SRAM allocations return a CPU virtual address, or NULL on error. >> + * If a DMA address is requested and the SRAM supports DMA, its >> + * mapped address is also returned. >> + * >> + * Errors include SRAM memory not being available, and requesting >> + * DMA mapped SRAM on systems which don't allow that. >> + */ >> +extern void *sram_alloc(size_t len, dma_addr_t *dma); >> +extern void sram_free(void *addr, size_t len); >> + >> +#endif /* __MACH_SRAM_H */ >> diff --git a/arch/arm/mach-davinci/sram.c >> b/arch/arm/mach-davinci/sram.c >> new file mode 100644 >> index 000..0309e72 >> --- /dev/null >> +++ b/arch/arm/mach-davinci/sram.c >> @@ -0,0 +1,74 @@ >> +/* >> + * mach-davinci/sram.c - DaVinci simple SRAM allocator >> + * >> + * Copyright (C) 2009 David Brownell >> + * >> + * This program is free software; you can redistribute it >> and/or modify >> + * i
RE: [PATCH 3/3] davinci: add SRAM allocator
> -Original Message- > From: davinci-linux-open-source-boun...@linux.davincidsp.com > [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com > ] On Behalf Of Kevin Hilman > Sent: Friday, May 22, 2009 8:11 AM > To: Ring, Chris > Cc: davinci-linux-open-source@linux.davincidsp.com; David > Brownell; linux-arm-ker...@lists.arm.linux.org.uk > Subject: Re: [PATCH 3/3] davinci: add SRAM allocator > > "Ring, Chris" writes: > > > Just a quick "can we talk through this patch a bit" note... > > > > I _think_ there are existing codecs on some DaVinci platforms (e.g. > > DM365) that are already using this SRAM memory. The current [easy] > > solution to their use case was to keep this memory out of > the kernel, > > and simply give it to CMEM to manage (via its insmod > params) and the > > codecs indirectly allocate from CMEM. > > Couldn't the CMEM driver just call sram_alloc() in this case? I suppose it could, in cases where sram_alloc() is defined for the particular Linux version. However, supporting SRAM w/ CMEM as it is today involves simply using the 2nd "block instance" that CMEM already supports, i.e., no additional code needed to differentiate between using CMEM's internal allocator or using sram_alloc(). I think the way codecs are going to request SRAM is by asking for all of it, which means that if the audio driver takes any of it through sram_alloc(), codecs won't have any. And if codecs take it all before the audio driver asks for it, the audio driver is out of luck. David Brownell suggested that it would be easy to add a "request_mem_region()" reservation call into the sram allocator. Regardless of whether or not CMEM uses sram_alloc(), I'd still like to see that. > > > If you're unfamiliar with CMEM, there is an overview here: > > http://tiexpressdsp.com/index.php?title=CMEM_Overview > > > > I'm not the expert, I only want to encourage further discussion! > > But I'm a little concerned about a resource collision if this SRAM > > allocator gets pushed, and more than one client thinks they > own that > > resource. > > I would argue that these potentical collisions are all the > more reason to push an allocator which can be used to avoid > collisions. I'd like to see CMEM's internal allocator go by the wayside as well, and one thing that would help that goal is to have an allocator that can handle all types of memory that CMEM handles. Today, those types that CMEM has been asked to handle are: - DDR that is not given to the Linux kernel (the large majority of the cases) - SRAM on OMAP3 - TCM on DM365 CMEM is just told a memory range. If we need to call a different allocator for each type, so be it, and CMEM would have to now be told the type of memory (just thinking out loud here). If we had a common allocator for all types above, CMEM would be that much cleaner. Would the "slab allocator" be at all appropriate here? Regards, - Rob > > Kevin > > > >> -Original Message- > >> From: davinci-linux-open-source-boun...@linux.davincidsp.com > >> [mailto:davinci-linux-open-source-boun...@linux.davincidsp.com > >> ] On Behalf Of Kevin Hilman > >> Sent: Thursday, May 21, 2009 2:47 PM > >> To: linux-arm-ker...@lists.arm.linux.org.uk > >> Cc: davinci-linux-open-source@linux.davincidsp.com; David Brownell > >> Subject: [PATCH 3/3] davinci: add SRAM allocator > >> > >> From: David Brownell > >> > >> Provide a generic SRAM allocator using genalloc, and > vaguely modeled > >> after what AVR32 uses. This builds on top of the static > CPU mapping > >> set up in the previous patch, and returns DMA mappings as > requested > >> (if possible). > >> > >> Compared to its OMAP cousin, there's no current support for > >> (currently non-existent) DaVinci power management code running in > >> SRAM; and this has ways to deallocate, instead of being > >> allocate-only. > >> > >> The initial user of this should probably be the audio > code, because > >> EDMA from DDR is subject to various dropouts on at least DM355 and > >> DM6446 chips. > >> > >> Signed-off-by: David Brownell > >> Signed-off-by: Kevin Hilman > >> --- > >> arch/arm/Kconfig |1 + > >> arch/arm/mach-davinci/Makefile|2 +- > >> arch/arm/mach-davinci/include/mach/sram.h | 27 ++ > >> arch/arm/mach-davinci/sram.c | 74 > >> +
Re: [PATCH 3/3] davinci: add SRAM allocator
On Friday 22 May 2009, Tivy, Robert wrote: > I think the way codecs are going to request SRAM is by > asking for all of it, Yeech. I hope that's on the list of bugs to fix! > which means that if the audio driver takes any of it > through sram_alloc(), codecs won't have any. And if > codecs take it all before the audio driver asks for it, > the audio driver is out of luck. Same kind of issue, eventually, with power management, which may need SRAM in order to put the system into low power states. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 3/3] davinci: add SRAM allocator
> -Original Message- > From: David Brownell [mailto:davi...@pacbell.net] > > On Friday 22 May 2009, Tivy, Robert wrote: > > I think the way codecs are going to request SRAM is by > > asking for all of it, > > Yeech. I hope that's on the list of bugs to fix! AFAIK, it's not, nor is it a bug. If no one else is using it, the codecs should feel free to. The system integrator should be able to decide who should have it rather than hard-coding codecs/audio drivers/etc to require it. If it's not available, the codecs will get the memory from somewhere else and simply(?) suffer a performance cost. But some systems may decide using this memory to improve codec performance is more important than giving it to someone else. And vice-versa. Chris___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 3/3] davinci: add SRAM allocator
"Ring, Chris" writes: >> -Original Message- >> From: David Brownell [mailto:davi...@pacbell.net] >> >> On Friday 22 May 2009, Tivy, Robert wrote: >> > I think the way codecs are going to request SRAM is by >> > asking for all of it, >> >> Yeech. I hope that's on the list of bugs to fix! > > AFAIK, it's not, nor is it a bug. If no one else is using it, the codecs > should feel free to. The system integrator should be able to decide who > should have it rather than hard-coding codecs/audio drivers/etc to require it. > > If it's not available, the codecs will get the memory from somewhere else and > simply(?) suffer a performance cost. But some systems may decide using this > memory to improve codec performance is more important than giving it to > someone else. And vice-versa. > All of this discussion isn't relevant to the need for an in-kernel SRAM allocator upstream. What is being discussed here is various ways of using or not using it. IMHO, this discussion isn't relevant to whether or not this allocator should go upstream. Even with this allocator upstream, the system integrator is free not to use it and replace it with something else. Let's not bog down the merging of a feature just because some users may not use it. Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 3/3] davinci: add SRAM allocator
> -Original Message- > From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > All of this discussion isn't relevant to the need for an in-kernel > SRAM allocator upstream. What is being discussed here is various ways > of using or not using it. IMHO, this discussion isn't relevant to > whether or not this allocator should go upstream. > > Even with this allocator upstream, the system integrator is free not > to use it and replace it with something else. > > Let's not bog down the merging of a feature just because some users > may not use it. Peace. Speaking just for me (not necessarily TI), I'm ok with that. I like Rob/David's suggestion of adding request_mem_region()/release_mem_region() calls. CMEM added this recently too, so at least we'd get a nice error msg rather than have them stomp on each other. Assuming this SRAM allocator gets pushed then, can we talk through what happens when the audio driver (or any other driver!) requests this SRAM and either no SRAM is available, or this SRAM allocator isn't configured into the kernel? Do we lose the audio driver support? Does the audio driver need to plan ahead and have a mode where it doesn't use this SRAM? Chris___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 3/3] davinci: add SRAM allocator
[ removing linux-arm-kernel ] "Ring, Chris" writes: >> -Original Message- >> From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >> All of this discussion isn't relevant to the need for an in-kernel >> SRAM allocator upstream. What is being discussed here is various ways >> of using or not using it. IMHO, this discussion isn't relevant to >> whether or not this allocator should go upstream. >> >> Even with this allocator upstream, the system integrator is free not >> to use it and replace it with something else. >> >> Let's not bog down the merging of a feature just because some users >> may not use it. > > Peace. Speaking just for me (not necessarily TI), I'm ok with that. > > I like Rob/David's suggestion of adding > request_mem_region()/release_mem_region() calls. CMEM added this recently > too, so at least we'd get a nice error msg rather than have them stomp on > each other. > > Assuming this SRAM allocator gets pushed then, can we talk through what > happens when the audio driver (or any other driver!) requests this SRAM and > either no SRAM is available, or this SRAM allocator isn't configured into the > kernel? > > Do we lose the audio driver support? Does the audio driver need to plan > ahead and have a mode where it doesn't use this SRAM? > Chris, Do you mind starting up a new thread on the davinci list to discuss this? Thanks, Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
