Skip to content

Conversation

@keyonjie
Copy link
Contributor

@keyonjie keyonjie commented Sep 9, 2021

cavs: memory: make full use of HPSRAM

Make size of the buffer zone calculated at the linking stage, to make
full use of all HPSRAM memories.

Make size of the buffer zone calculated at the linking stage, to make
full use of all HPSRAM memories.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
@keyonjie keyonjie changed the title Heap refinement -- make full use of HPSRAM in buffer zone Heap refinement Part 2 -- make full use of HPSRAM in buffer zone Sep 9, 2021
@keyonjie
Copy link
Contributor Author

keyonjie commented Sep 9, 2021

SOFCI TEST

Align to the new heap memory map and allocator, smaller SYSTEM and
RUNTIME zones are used on apollolake now.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Make the size of buffer zone calculated at linking stage, to align to
the latest cavs version.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
@lgirdwood
Copy link
Member

@kkarask @wszypelt can someone restart the CI platforms. Thanks !

17:34:16.079     Timed out after 5.0 seconds.

17:34:16.156     => Exception in test.
17:34:16.172       Exception TimeoutError occurred at 'C:\\work\\tools\\pyexecnetcache\\cavs_scripts-py\\utilities\\fw_loader_pci.py':499
                   Cleaning ROM control IPC failed. Platform restart may be required.

17:34:16.234  === END OF ITERATION === 
17:34:16.259 Test End: 2021-09-09 17:34:16.259

17:34:16.259 Test Duration: 0:00:44.746818

@kkarask
Copy link

kkarask commented Sep 10, 2021

PR passed after platform restart and rerun.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @keyonjie this increases the available blocks in the allocator, we can further refine for rc2/final if needed.

@lgirdwood lgirdwood merged commit a7367f4 into thesofproject:main Sep 10, 2021
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe some of these comments are worth addressing in a follow-up PR


. = ALIGN (HEAP_BUF_ALIGNMENT);
_buffer_heap_start = ABSOLUTE(.);
. = . + SOF_FW_END - _buffer_heap_start;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SOF_FW_END is defined as (HP_SRAM_BASE + HP_SRAM_SIZE - 0x1000) and HP_SRAM_BASE is defined to 0xbe000000, so, they both are absolute addresses. _buffer_heap_start is also an absolute address, corresponding to the (relative) current address .. So, SOF_FW_END - _buffer_heap_start is the memory size from "here" to the end of HP SRAM, right? So the above line calculates the relative address of SOF_FW_END. And below _buffer_heap_end is the respective absolute address again, i.e. it's just equal to SOF_FW_END isn't it? Wouldn't it be easier to just use _buffer_heap_end = SOF_FW_END?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, but we are using some common formats for each zone here:
xx_heap_start = ABSOLUTE(.);
. = . + XXX_SIZE;
xx_heap_end = ABSOLUTE(.);

With this format, we can change the "XXX_SIZE" easily and corresponding change to xx_heap_start/end is not needed.

OTOH, this is aligned with other non-Intel or non-cAVS platforms also, where there could be no definition of SOF_FW_END at all.


/* Heap blocks for buffers */
static SHARED_DATA struct block_hdr buf_block[HEAP_BUFFER_COUNT];
static SHARED_DATA struct block_hdr buf_block[HEAP_BUFFER_COUNT_MAX];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have an idea how much RAM this is wasting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I talked this with @lgirdwood , it is 8B/256B about 3%here.

There are some waste, the percentage is up to how much of it is inevitable: e.g.
On TGL like (~3MB HPSRAM), we have about 2/3 memory used for this BUFFER zone so only 1/3 * 3% = 1% is wasted.
On APL like (~0.5MB HPSRAM), the wasting is worst as there is about 1/3 memory used for this BUFFER zone so about 2/3 * 3% = 2% is wasted.

I previously has some calculation to improve it for small SRAM platforms, like

/* the buffer zone will not occupy more than half of the HP SRAM */
#define HEAP_BUFFER_COUNT_MAX  (HP_SRAM_SIZE / (HEAP_BUFFER_BLOCK_SIZE * 2))

This could help to bring back about 5KB memory for us, we can do this kind of fine tune incrementally.

#define SRAM_BANK_SIZE 0x10000
#define LP_SRAM_SIZE SRAM_BANK_SIZE
#define HP_SRAM_SIZE SRAM_BANK_SIZE
#define HP_SRAM_SIZE (SRAM_BANK_SIZE * 47)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

47 has no special meaning, right? It's just to match one of existing platforms? You could use 4 or 32 or 1024 here or whatever? Maybe at least add a comment there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let me add a comment for it.

@keyonjie keyonjie mentioned this pull request Sep 13, 2021
@lgirdwood
Copy link
Member

@lyakh this is a bit of a chicken and egg situation wrt to the linker script and the amount of SRAM free after TEXT, DATA and BSS sizes are known when creating the static heap block arrays. Not a problem in Zephyr as it uses lists in its allocator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants