-
Notifications
You must be signed in to change notification settings - Fork 140
New memory domain logic #5537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: topic/sof-dev
Are you sure you want to change the base?
New memory domain logic #5537
Conversation
|
FYI @ranj063 @lyakh @lgirdwood |
1541ecf to
c39e6b2
Compare
sound/soc/sof/ipc4-topology.c
Outdated
|
|
||
| if (swidget->domain_id != 0) | ||
| dev_warn(sdev->dev, "%s is not DP, but domain_id %u was set in topology", | ||
| swidget->widget->name, swidget->domain_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we've going to override it anyway, do we need a warning at all? Wont it be just noise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is about to change. We need separate domain_id and pipeline_id. So this code is already obsolete, but I don't try to reimplement it yet, since it seems the target is still moving.
c39e6b2 to
c9df7c7
Compare
c9df7c7 to
cc4773a
Compare
Remove dp-prefix from all module instance's memory attributes and related data structures. The attributes are not anymore exclusively for Data Processing module instances, but generic for all module instances. However, the module init payload is still only for DP module instances. Signed-off-by: Jyri Sarha <[email protected]>
The was inconsistency with SOF_TKN_COMP_STACK_BYTES_REQUIREMENT and SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT token ids in the Linux driver code with SOF FW topology code. This commit fixes the Linux side to match tools/topology/topology2/include/common/tokens.conf See: https://github.com/thesofproject/sof/blob/788861804ed08485496e979dd9c467c1a21b30c5/tools/topology/topology2/include/common/tokens.conf#L30 Signed-off-by: Jyri Sarha <[email protected]>
cc4773a to
c503c3d
Compare
sound/soc/sof/ipc4-topology.c
Outdated
| {SOF_TKN_COMP_STACK_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32, | ||
| offsetof(struct snd_sof_widget, stack_bytes)}, | ||
| {SOF_TKN_COMP_STATIC_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32, | ||
| offsetof(struct snd_sof_widget, static_bytes)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the commit message: is it heap size for initialisation only or for the entire life-time of a single instance (estimated, possibly dependent on configuration or use-case)
| #define SOF_IPC4_GLB_PIPE_EXT_OBJ_ARRAY(x) ((x) << SOF_IPC4_GLB_PIPE_EXT_OBJ_ARRAY_SHIFT) | ||
|
|
||
| struct sof_ipc4_glb_pipe_payload { | ||
| u32 word0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you do bitfields instead e.g.
u32 payload : 24;
u32 array : 8;
? Or at least pick up a more informative name like payload_array and maybe use __bitwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Linux kernel style, at least in SOF driver, and in all other Linux drivers I've been working on. In SOF driver every other IPC message and payload structure is defined with macros, not with bitfields. I don't think I should diverge from this in PR either.
| #define SOF_IPC4_GLB_PIPE_EXT_OBJ_WORDS(x) ((x) << SOF_IPC4_GLB_PIPE_EXT_OBJ_WORDS_SHIFT) | ||
|
|
||
| struct sof_ipc4_glb_pipe_ext_object { | ||
| u32 header; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also be split into sub-fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
c503c3d to
ead2c46
Compare
|
Updated to match and work with thesofproject/sof#10281 . I will address the review comments at later time. |
|
I'll mark this ready for review, but before merging it should be checked that thesofproject/sof#10265 is also going to go in without changes to IPC payload. |
sound/soc/sof/ipc4-topology.c
Outdated
|
|
||
| dev_dbg(sdev->dev, "payload word0 %#x domain_id %u stack_bytes %u heap_bytes %u", | ||
| payload_hdr->word0, mem_data->domain_id, mem_data->stack_bytes, | ||
| mem_data->heap_bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation seems be incorrect.
include/uapi/sound/sof/tokens.h
Outdated
| #define SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT 420 | ||
| #define SOF_TKN_COMP_STACK_BYTES_REQUIREMENT 421 | ||
| #define SOF_TKN_COMP_STACK_BYTES_REQUIREMENT 420 | ||
| #define SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT 421 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this INTERIM_HEAP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can. These tokens were already part of 2.14, but then again they were not really used for anything then.
include/uapi/sound/sof/tokens.h
Outdated
| #define SOF_TKN_COMP_DOMAIN_ID 419 | ||
| #define SOF_TKN_COMP_STACK_BYTES_REQUIREMENT 420 | ||
| #define SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT 421 | ||
| #define SOF_TKN_COMP_LIFETIME_BYTES_REQUIREMENT 422 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and SOF_TKN_COMP_LIFETIME_HEAP_BYTES_REQUIREMENT
Add token SOF_TKN_COMP_LIFETIME_HEAP_BYTES_REQUIREMENT for the amount of allocated memory a module instance needs for the whole of its life time from init to free, and SOF_TKN_COMP_SHARED_BYTES_REQUIREMENT for the amount of shared memory it needs to allocate. Also renames the SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT to SOF_TKN_COMP_INTERIM_HEAP_BYTES_REQUIREMENT to follow FW implementation terms. Signed-off-by: Jyri Sarha <[email protected]>
Add to lifetime_bytes and shared_bytes snd_sof_widget. They are with SOF_TKN_COMP_LIFETIME_HEAP_BYTES_REQUIREMENT topology tokens and SOF_TKN_COMP_SHARED_BYTES_REQUIREMENT topology token's values. Also renames heap_bytes to interim_bytes to follow the term used in FW side implementation. Signed-off-by: Jyri Sarha <[email protected]>
Adds SOF_IPC4_GLB_PIPE_EXT_OBJ_ARRAY macros to set extension bit in SOF_IPC4_GLB_CREATE_PIPELINE indicating presence of the payload, and all necessary macros and structs to create the payload. Signed-off-by: Jyri Sarha <[email protected]>
Start adding payloads to pipeline create messages. The payload contains information for payload specific memory configuration. All non DP module instances within the same pipeline share the same memory attributes and access the same resources. The new logic sums interim, lifetime, and shared heap memory requirements together and picks the highest stack requirement of all module instances belonging to a pipeline. These pipeline specific attributes are sent as struct sof_ipc4_glb_pipe_payload payload in pipeline's create message. The idea is to pass common memory configuration for all the Low Latency modules in the pipeline in pipeline create message payload. The Data Processing module instances will still have an individual memory configuration in struct sof_ipc4_mod_init_ext_dp_memory_data payloads as before. In their payload everything is as it was before, all attributes are copied directly from their topology attributes. Signed-off-by: Jyri Sarha <[email protected]>
ead2c46 to
91c9b6e
Compare
|
The review comments are now addressed, but there is one loose end. The module init payload still uses the old DP memory data in the payload. Its easy change that too, but I need to work on the FW side for it to understand it and to be able to test the code. |
This is my first step towards implementing pipeline specific heaps. There is brand new create message payload definition and implementation. However, the target has already moved so that this payload is not enough anymore.
There is also a problem that it looks like the FW receives some pipeline create messages with payload bit set, but the payload is not there. Firmware receives two messages where linux driver only logs about sending only one. This probably has something to do with forwarding the messages form core 0 to core 2. I have no idea how this can happen, but as I should move to work on kernel heap, I leave the debugging for now.
The FW side code is here: thesofproject/sof#10265