Uh oh!
There was an error while loading. Please reload this page.
memory: Add virtual memory regions gathering and structs - #6931
Conversation
e635312 to
47129a2ComparePleasing checkpatch is hard when adding new files. This is tricky and comes up every time someone adds new files, examples in thesofproject#6284, thesofproject#6796, thesofproject#6931 , etc. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
commented
Jan 10, 2023
To please checkpatch and fix https://github.com/thesofproject/sof/actions/runs/3886647798/jobs/6631992187 please have a look at brand new |
marc-hb
commented
Jan 10, 2023
Not a single configuration compiled in https://github.com/thesofproject/sof/actions/runs/3886647797/jobs/6631994411 Please submit as a Github "draft" and keep it a draft until compilation issues are fixed to avoid spamming reviewers, thanks! |
00e938f to
8d7538fComparedabekjakub
commented
Jan 10, 2023
I can build the changes localy with no problem that is why i pushed it - was doing checkpatch errors first didn't think it would spam reviewers - sorry. |
Configuration differences happen no matter how hard we try to chase them: that's one of the reasons why I start ALL my pull requests as "drafts". It's a no-brainer, "draft" should be the default. |
fa76b91 to
cccbfbaComparedabekjakub
commented
Jan 11, 2023
Current Ci problems seem to be generic - I am currently resolving those offline with CI owners. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
You're overwriting vm_heaps[i].memory_caps in each of these ifs, right? Can .attr have more than one of these flags set? If so, it seems that MEM_REG_ATTR_OPPORTUNISTIC_MEMORY has the highest priority, since it overrides the other two, and MEM_REG_ATTR_CORE_HEAP has the lowest priority. So you revert the order and use else if to make the priorities clearer.
There was a problem hiding this comment.
Each of those regions is a distinct entity so none of these has a priority perse. Zephyr provides information and i translate it here so we can use it further in allocation.
Those Attributes are work in progress - they are due to change if future implementation uses theme. However elseif is a good point.
There was a problem hiding this comment.
but you should clearly define (1) whether those attributes are mutually exclusive and (2) what are their priorities. The way you had it implemented in the previous version meant, that they possibly weren't mutually exclusive and the priorities were (from the highest to the lowest):
MEM_REG_ATTR_OPPORTUNISTIC_MEMORY
MEM_REG_ATTR_SHARED_HEAP
MEM_REG_ATTR_CORE_HEAP
In your today's version you exactly reverted priorities. Is this intended? OTOH, if these attributes are mutually exclusive, then priorities don't matter, then I would suggest to do
switch (virtual_memory_regions[i].attr & MEM_REG_ATTR_MASK) {
case MEM_REG_ATTR_SHARED_HEAP:
...
There was a problem hiding this comment.
Yea they are actually mutually exclusive and there are no priorities - need to change zephyr implementation to enum later or is there any other way You suggest ?
Uh oh!
There was an error while loading. Please reload this page.
Add virtual memory regions info gathering from zephyr and introduce structs agregating this information. Add proper defines for memory management. Signed-off-by: Jakub Dabek <jakub.dabek@intel.com>
| i++) { | ||
| vm_heaps[i].virtual_region = &virtual_memory_regions[i]; | ||
| switch (virtual_memory_regions[i].attr) { |
There was a problem hiding this comment.
oh, so it's not just a bit set in the attribute, the attribute is actually equal to one of those bits. I thought you'd need to do something like
#define MEM_REG_ATTR_MASK (MEM_REG_ATTR_OPPORTUNISTIC_MEMORY | MEM_REG_ATTR_SHARED_HEAP | MEM_REG_ATTR_CORE_HEAP)
switch (virtual_memory_regions[i].attr & MEM_REG_ATTR_MASK) {
but if you don't - all the better
dabekjakub
commented
Jan 17, 2023
Can anyone rerun the CI on this pr ? @lgirdwood@mengdonglin I am 99% sure that it was an issue with CI not with the commit. (CI passed some time back but failed again after cosmetic changes) |
kv2019i
commented
Jan 17, 2023
@dabekjakub wrote:
I think we can merge without a full rerun. The CI failures are known failures in the ADLP SDW configuration and not related to this PR. |
Add virtual memory regions info gathering from zephyr and introduce structs agregating this information.
Add proper defines for memory managment.
Signed-off-by: Jakub Dabek jakub.dabek@intel.com