[RFC] tidspbridge: use a parameter to allocate shared memory

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

tidspbridge driver uses a block of memory denominated SHared Memory
to store info & communicate with DSP, this SHM needs to be physically
contiguous and non-cacheable, to achieve the latter the driver ioremaps
the memory reserved to be SHM, this will trigger a warning if the
memory is under kernel control (because it creates another set of
mapping attributes, for the same memory area).

For now this can be avoided if a portion of memory (6MB) is left out
of kernel control (using bootarg attribute mem=) where tidspbridge
driver can make use of the memory and ioremap it without above
restriction.

Parameter has precedence over memblock allocator for shared memory.

i.e.: on a system with 256MB

set 'bootargs mem=250M ...'

cat /proc/iomem
...
80000000-8f9fffff : System RAM

So driver needs to be installed with:

insmod bridgedriver.ko phys_mempool_base=0x8FA00000

Same rationale applies for the menuconfig option.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---

Code to allocate dspbridge memblock needs to be disabled since
it will be wasting 6MB.

 arch/arm/mach-omap2/dsp.c                        |    2 +-
 drivers/staging/tidspbridge/Kconfig              |   10 ++++++++++
 drivers/staging/tidspbridge/rmgr/drv_interface.c |   15 ++++++++++++++-
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/dsp.c b/arch/arm/mach-omap2/dsp.c
index 6feeeae..0a4ba2f 100644
--- a/arch/arm/mach-omap2/dsp.c
+++ b/arch/arm/mach-omap2/dsp.c
@@ -45,7 +45,7 @@ static int __init omap_dsp_init(void)
 	int err = -ENOMEM;
 	struct omap_dsp_platform_data *pdata = &omap_dsp_pdata;
 
-	pdata->phys_mempool_base = omap_dsp_get_mempool_base();
+	pdata->phys_mempool_base = CONFIG_TIDSPBRIDGE_MEMPOOL_BASE;
 
 	if (pdata->phys_mempool_base) {
 		pdata->phys_mempool_size = CONFIG_TIDSPBRIDGE_MEMPOOL_SIZE;
diff --git a/drivers/staging/tidspbridge/Kconfig b/drivers/staging/tidspbridge/Kconfig
index 93de4f2..1214cbc 100644
--- a/drivers/staging/tidspbridge/Kconfig
+++ b/drivers/staging/tidspbridge/Kconfig
@@ -23,6 +23,16 @@ config TIDSPBRIDGE_DVFS
 	  performance and power consumption to the current processing
 	  requirements.
 
+config TIDSPBRIDGE_MEMPOOL_BASE
+	hex "Physical memory pool base (Addr)"
+	depends on TIDSPBRIDGE
+	default 0
+	help
+	  Use this address as the start of the shared memory block, it is assumed
+	  that this address is outside kernel control and configured by tweaking
+	  mem= in  bootargs. BASE + SIZE should fit in the system RAM address
+	  space.
+
 config TIDSPBRIDGE_MEMPOOL_SIZE
 	hex "Physical memory pool size (Byte)"
 	depends on TIDSPBRIDGE
diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 1981e46..4533605 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -86,6 +86,7 @@ static s32 driver_major;
 static char *base_img;
 char *iva_img;
 static s32 shm_size = 0x500000;	/* 5 MB */
+static unsigned int phys_mempool_base;
 static int tc_wordswapon;	/* Default value is always false */
 #ifdef CONFIG_TIDSPBRIDGE_RECOVERY
 #define REC_TIMEOUT 5000	/*recovery timeout in msecs */
@@ -132,6 +133,9 @@ MODULE_PARM_DESC(shm_size, "shm size, default = 4 MB, minimum = 64 KB");
 module_param(tc_wordswapon, int, 0);
 MODULE_PARM_DESC(tc_wordswapon, "TC Word Swap Option. default = 0");
 
+module_param(phys_mempool_base, uint, 0);
+MODULE_PARM_DESC(phys_mempool_base, "Physical Address base for SHM");
+
 MODULE_AUTHOR("Texas Instruments");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DSPBRIDGE_VERSION);
@@ -298,7 +302,16 @@ static int omap3_bridge_startup(struct platform_device *pdev)
 	}
 	dev_dbg(bridge, "%s: requested shm_size = 0x%x\n", __func__, shm_size);
 
-	phys_membase = pdata->phys_mempool_base;
+	if (phys_mempool_base) {
+		/* Out of kernel SHM */
+		phys_membase = phys_mempool_base;
+	} else if (pdata->phys_mempool_base) {
+		/* Memblock allocator for SHM */
+		phys_membase = pdata->phys_mempool_base;
+	} else {
+		pr_err("%s: couldn't get SHM physical address\n", __func__);
+		goto err3;
+	}
 	phys_memsize = pdata->phys_mempool_size;
 	if (phys_membase > 0 && phys_memsize > 0)
 		mem_ext_phys_pool_init(phys_membase, phys_memsize);
-- 
1.7.1

[RFC] tidspbridge: use a parameter to allocate shared memory

[Laurent Pinchart]

Hi Omar,

On Thursday 07 October 2010 07:45:36 Omar Ramirez Luna wrote:
> tidspbridge driver uses a block of memory denominated SHared Memory
> to store info & communicate with DSP, this SHM needs to be physically
> contiguous and non-cacheable,

There are non-cacheable mappings, but there's no such thing as non-cacheable 
memory. Does the MPU mapping for that SHM block really needs to be non-
cacheable, your could you instead flush the cache after writing to it 
(performance issues might be involved, I don't know the details about that SHM 
usage) ?

> to achieve the latter the driver ioremaps
> the memory reserved to be SHM, this will trigger a warning if the
> memory is under kernel control (because it creates another set of
> mapping attributes, for the same memory area).
> 
> For now this can be avoided if a portion of memory (6MB) is left out
> of kernel control (using bootarg attribute mem=) where tidspbridge
> driver can make use of the memory and ioremap it without above
> restriction.
> 
> Parameter has precedence over memblock allocator for shared memory.
> 
> i.e.: on a system with 256MB
> 
> set 'bootargs mem=250M ...'
> 
> cat /proc/iomem
> ...
> 80000000-8f9fffff : System RAM
> 
> So driver needs to be installed with:
> 
> insmod bridgedriver.ko phys_mempool_base=0x8FA00000
> 
> Same rationale applies for the menuconfig option.
> 
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>

-- 
Regards,

Laurent Pinchart

[RFC] tidspbridge: use a parameter to allocate shared memory

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 09:40:12AM +0200, Laurent Pinchart wrote:
> Hi Omar,
> 
> On Thursday 07 October 2010 07:45:36 Omar Ramirez Luna wrote:
> > tidspbridge driver uses a block of memory denominated SHared Memory
> > to store info & communicate with DSP, this SHM needs to be physically
> > contiguous and non-cacheable,
> 
> There are non-cacheable mappings, but there's no such thing as non-cacheable 
> memory. Does the MPU mapping for that SHM block really needs to be non-
> cacheable, your could you instead flush the cache after writing to it 
> (performance issues might be involved, I don't know the details about that SHM 
> usage) ?

ARMv6 and above don't like having multiple mappings with different
memory type/shareability/cache attributes.  It's architecturally
forbidden.

So if you want non-cacheable memory and you want to be architecturally
compliant, you have to exclude it from the kernel's direct-mapped
memory mapping.

[RFC] tidspbridge: use a parameter to allocate shared memory

[Laurent Pinchart]

Hi Russell

On Thursday 07 October 2010 10:32:42 Russell King - ARM Linux wrote:
> On Thu, Oct 07, 2010 at 09:40:12AM +0200, Laurent Pinchart wrote:
> > Hi Omar,
> > 
> > On Thursday 07 October 2010 07:45:36 Omar Ramirez Luna wrote:
> > > tidspbridge driver uses a block of memory denominated SHared Memory
> > > to store info & communicate with DSP, this SHM needs to be physically
> > > contiguous and non-cacheable,
> > 
> > There are non-cacheable mappings, but there's no such thing as
> > non-cacheable memory. Does the MPU mapping for that SHM block really
> > needs to be non- cacheable, your could you instead flush the cache after
> > writing to it (performance issues might be involved, I don't know the
> > details about that SHM usage) ?
> 
> ARMv6 and above don't like having multiple mappings with different
> memory type/shareability/cache attributes.  It's architecturally
> forbidden.
> 
> So if you want non-cacheable memory and you want to be architecturally
> compliant, you have to exclude it from the kernel's direct-mapped
> memory mapping.

That's why Omar's patch uses 'mem=' to exclude system memory from the kernel 
mappings. That's not ideal though, as that memory will be wasted forever, 
hence my comments regarding whether a non-cacheable mapping was really 
required.

Do we have an infrastructure, or even an embryo thereof, to remove pages from 
the kernel's direct-mapped memory mapping at runtime ? The use of super pages 
probably complicates the matter.

-- 
Regards,

Laurent Pinchart

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

On 10/7/2010 2:40 AM, Laurent Pinchart wrote:
> Hi Omar,
>
> On Thursday 07 October 2010 07:45:36 Omar Ramirez Luna wrote:
>> tidspbridge driver uses a block of memory denominated SHared Memory
>> to store info&  communicate with DSP, this SHM needs to be physically
>> contiguous and non-cacheable,
>
> There are non-cacheable mappings, but there's no such thing as non-cacheable
> memory. Does the MPU mapping for that SHM block really needs to be non-
> cacheable,

yes

> your could you instead flush the cache after writing to it
> (performance issues might be involved, I don't know the details about that SHM
> usage) ?

You can do that too, but it will involve more changes to dsp side, and 
yes performance might be an issue too.

The so called "shared memory" is used between arm tidspbridge and the 
DSP, they exchange communication structures/streams/messages/parameters 
needed on both sides for its correct functionality (this is an eagle-eye 
view of the SHM, for more info if interested check page 32 of the 
overview pdf[1]).

tidspbridge could have the changes made for flushing the SHM every time 
it writes into it, a flag could be used to prevent both of them (ARM & 
DSP) flushing at the same time if needed, but I don't know how feasible 
would be making those changes in the dsp code.

Regards,

Omar

---
[1] 
https://gforge.ti.com/gf/download/docmanfileversion/17/674/OMAP3430_Bridge_overview.pdf

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

On 10/7/2010 9:01 AM, Laurent Pinchart wrote:
> On Thursday 07 October 2010 10:32:42 Russell King - ARM Linux wrote:
>>
>> ARMv6 and above don't like having multiple mappings with different
>> memory type/shareability/cache attributes.  It's architecturally
>> forbidden.
>>
>> So if you want non-cacheable memory and you want to be architecturally
>> compliant, you have to exclude it from the kernel's direct-mapped
>> memory mapping.
>
> That's why Omar's patch uses 'mem=' to exclude system memory from the kernel
> mappings. That's not ideal though, as that memory will be wasted forever,
> hence my comments regarding whether a non-cacheable mapping was really
> required.

it is not ideal to waste that memory, but strictly speaking old bootmem 
does the same, as no one will be touching that memory. i.e. you compile 
bridge as a module but you never insmod it, the reserved bootmem space 
is there for bridge anyway; same for bootargs tweaking, if you need 
dspbridge and are going to use it then you set aside some memory for it.

What might be a pain for end-user, is to have drivers that need to do 
tweaking to bootargs to work; but right now that is a requirement, until 
a better solution is found/created.

Regards,

Omar

[RFC] tidspbridge: use a parameter to allocate shared memory

[Felipe Contreras]

On Thu, Oct 7, 2010 at 8:01 PM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> On 10/7/2010 2:40 AM, Laurent Pinchart wrote:
>> On Thursday 07 October 2010 07:45:36 Omar Ramirez Luna wrote:
>>>
>>> tidspbridge driver uses a block of memory denominated SHared Memory
>>> to store info& ?communicate with DSP, this SHM needs to be physically
>>> contiguous and non-cacheable,
>>
>> There are non-cacheable mappings, but there's no such thing as
>> non-cacheable
>> memory. Does the MPU mapping for that SHM block really needs to be non-
>> cacheable,
>
> yes
>
>> your could you instead flush the cache after writing to it
>> (performance issues might be involved, I don't know the details about that
>> SHM
>> usage) ?
>
> You can do that too, but it will involve more changes to dsp side, and yes
> performance might be an issue too.

I think it's worth experimenting, otherwise it will be difficult for
people to compile and use the driver.

> The so called "shared memory" is used between arm tidspbridge and the DSP,
> they exchange communication structures/streams/messages/parameters needed on
> both sides for its correct functionality (this is an eagle-eye view of the
> SHM, for more info if interested check page 32 of the overview pdf[1]).
>
> tidspbridge could have the changes made for flushing the SHM every time it
> writes into it, a flag could be used to prevent both of them (ARM & DSP)
> flushing at the same time if needed, but I don't know how feasible would be
> making those changes in the dsp code.

The synchronization should be already be in place, otherwise you would
be overriding data regardless of the cacheability of that data.

Note that the "shared memory" described in the document you share has
nothing to do with the SHM pool. AFAIK that memory is used for other
things, like MMU PTEs, and storing the base image and socket-nodes,
thus it needs to be contiguous.

Right now allocating contiguous memory can only be done with memblock
(bootmem), but in the future it could be done with CMA/VCMM. But the
cacheability is a separate issue.

I don't see any problem flushing the SHM area when needed, which
probably has performance implications when mmaping/unmapping buffers,
at which point you need to flush bigger memory areas anyway, so that's
not an issue.

Anyway, we will not know for sure until we try... Right?

Cheers.

-- 
Felipe Contreras

[RFC] tidspbridge: use a parameter to allocate shared memory

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 04:01:02PM +0200, Laurent Pinchart wrote:
> Do we have an infrastructure, or even an embryo thereof, to remove pages from 
> the kernel's direct-mapped memory mapping at runtime ? The use of super pages 
> probably complicates the matter.

No, and yes, using section mappings/supersection mappings further
complicates the issue because these are duplicated across all
processes in the system.

Even if you did walk all processes in the system, if your system is SMP
without h/w TLB broadcasting, you'd need to IPI the other CPUs and wait
for the IPI to complete - which you can not do if you're trying to
allocate non-cacheable memory from IRQ context.

We _could_ reduce TASK_SIZE to be slightly below 2GB, which then means
we can split the page tables in hardware, but I don't think its safe to
assume that the kernel will always use the init_mm page table to lookup
the >2GB page table entries.

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

On 10/7/2010 1:22 PM, Felipe Contreras wrote:
...
>
> Note that the "shared memory" described in the document you share has
> nothing to do with the SHM pool. AFAIK that memory is used for other
> things, like MMU PTEs, and storing the base image and socket-nodes,
> thus it needs to be contiguous.

hmmm, no. it is the same memory. i.e.: we propagate the current opp 
through the shared memory so the dsp can read it if it went to sleep, 
with the proper offset you can read that variable starting from the 
mempool base address.

"storing the base image and socket-nodes" symbols is also donde in the 
shared memory but this is done in different sections of the SHM.

>
> Right now allocating contiguous memory can only be done with memblock
> (bootmem), but in the future it could be done with CMA/VCMM. But the
> cacheability is a separate issue.

yes

>
> I don't see any problem flushing the SHM area when needed, which
> probably has performance implications when mmaping/unmapping buffers,
> at which point you need to flush bigger memory areas anyway, so that's
> not an issue.

well, you would have to flush when loading the base image, or allocating 
a socket node, but also minor flushes for opp propagation, SHM messages 
to DSP, chnl params, those are the ones I can quickly think of.

>
> Anyway, we will not know for sure until we try... Right?

yes we can try, at least we now for sure arm side can be done.

Regards,

omar

[RFC] tidspbridge: use a parameter to allocate shared memory

[Felipe Contreras]

On Thu, Oct 7, 2010 at 10:16 PM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> On 10/7/2010 1:22 PM, Felipe Contreras wrote:
> ...
>>
>> Note that the "shared memory" described in the document you share has
>> nothing to do with the SHM pool. AFAIK that memory is used for other
>> things, like MMU PTEs, and storing the base image and socket-nodes,
>> thus it needs to be contiguous.
>
> hmmm, no. it is the same memory. i.e.: we propagate the current opp through
> the shared memory so the dsp can read it if it went to sleep, with the
> proper offset you can read that variable starting from the mempool base
> address.

The document mentions "shared memory" for buffer passing, normal
memory is used for that, scattered, even user-space memory, not the
SHM contiguous area.

>> I don't see any problem flushing the SHM area when needed, which
>> probably has performance implications when mmaping/unmapping buffers,
>> at which point you need to flush bigger memory areas anyway, so that's
>> not an issue.
>
> well, you would have to flush when loading the base image, or allocating a
> socket node, but also minor flushes for opp propagation, SHM messages to
> DSP, chnl params, those are the ones I can quickly think of.

All those happen when you send buffers to the DSP, and when you do
that you need to flush the buffer memory area, which is a much heavier
operation. Except maybe the opp propagation, but that would require at
most one page to be flushed, not a big deal I think, besides, it would
be better if that specific area is handled differently than the rest
of the SHM, so that we can allocate it with dma_alloc_coherent().

Cheers.

-- 
Felipe Contreras

[RFC] tidspbridge: use a parameter to allocate shared memory

[Felipe Contreras]

On Thu, Oct 7, 2010 at 10:16 PM, Omar Ramirez Luna <omar.ramirez@ti.com> wrote:
> On 10/7/2010 1:22 PM, Felipe Contreras wrote:
>> Anyway, we will not know for sure until we try... Right?
>
> yes we can try, at least we now for sure arm side can be done.

The only thing that changes is the cacheability of the ARM side
memory, so of course only the ARM side matters. The DSP side will
continue to do what it's doing and would not notice any difference if
the memory is flushed, or is non-cacheable.

-- 
Felipe Contreras

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

On 10/8/2010 3:18 AM, Felipe Contreras wrote:
> On Thu, Oct 7, 2010 at 10:16 PM, Omar Ramirez Luna<omar.ramirez@ti.com>  wrote:
>> On 10/7/2010 1:22 PM, Felipe Contreras wrote:
>> ...
>>>
>>> Note that the "shared memory" described in the document you share has
>>> nothing to do with the SHM pool. AFAIK that memory is used for other
>>> things, like MMU PTEs, and storing the base image and socket-nodes,
>>> thus it needs to be contiguous.
>>
>> hmmm, no. it is the same memory. i.e.: we propagate the current opp through
>> the shared memory so the dsp can read it if it went to sleep, with the
>> proper offset you can read that variable starting from the mempool base
>> address.
>
> The document mentions "shared memory" for buffer passing, normal
> memory is used for that, scattered, even user-space memory, not the
> SHM contiguous area.

"streaming" (meaning dsp stream) buffers are passed through the SHM 
(check page 9). The regular "big" buffers are passed through old DMM 
(page 14) which is exactly as you describe it; but that SHM portion is 
referring to stream buffers.

>
>>> I don't see any problem flushing the SHM area when needed, which
>>> probably has performance implications when mmaping/unmapping buffers,
>>> at which point you need to flush bigger memory areas anyway, so that's
>>> not an issue.
>>
>> well, you would have to flush when loading the base image, or allocating a
>> socket node, but also minor flushes for opp propagation, SHM messages to
>> DSP, chnl params, those are the ones I can quickly think of.
>
> All those happen when you send buffers to the DSP, and when you do
> that you need to flush the buffer memory area, which is a much heavier
> operation. Except maybe the opp propagation, but that would require at
> most one page to be flushed, not a big deal I think, besides, it would
> be better if that specific area is handled differently than the rest
> of the SHM, so that we can allocate it with dma_alloc_coherent().

Yes, on top of the regular buffer flush, you need to flush the specific 
parts of the shared memory that the dsp is going to read, meaning: you 
send a buffer to the dsp, then send the MBX interrupt, to let it know 
there is a SHM message, DSP is going to read that SHM message from the 
SHM memory.

You also need to invalidate if the dsp is going to write, so the next 
time you read there is no mismatch in the data, meaning: DSP sends a SHM 
message to slot 1, but you already read slot 1 from a previous 
transaction so contents might be still in cache, so you need to 
invalidate that memory and read again the new SHM message.

Now to know when to read/write you need to have 2 flags per operation, 
one to know if you have new messages and other to know if you are 
reading/writing any message (all in SHM), which are there right now for 
this specific case; but if flushing the SHM, now you will need flags for 
knowing if the first two have been already flushed... and then 2 for 
these two... and so on. So in the end a better locking mechanism would 
be needed for both ARM and DSP which is not based on memory which may or 
may not have the latest contents (going the cacheable route).

As I have been saying, we can try this changes in the ARM tidspbridge 
because we _freely_ see what is going on, but in the DSP we can't.

Regards,

Omar

[RFC] tidspbridge: use a parameter to allocate shared memory

[Omar Ramirez Luna]

On 10/8/2010 3:20 AM, Felipe Contreras wrote:
> On Thu, Oct 7, 2010 at 10:16 PM, Omar Ramirez Luna<omar.ramirez@ti.com>  wrote:
>> On 10/7/2010 1:22 PM, Felipe Contreras wrote:
>>> Anyway, we will not know for sure until we try... Right?
>>
>> yes we can try, at least we now for sure arm side can be done.
>
> The only thing that changes is the cacheability of the ARM side
> memory, so of course only the ARM side matters. The DSP side will
> continue to do what it's doing and would not notice any difference if
> the memory is flushed, or is non-cacheable.
>

Please find my reply for the previous mail:

http://marc.info/?l=linux-omap&m=128655845213913&w=2

Regards,

Omar