Conversation
|
Can one of the admins verify this patch?
|
|
test this please |
lgirdwood
left a comment
There was a problem hiding this comment.
@checkupup are you able to add more inline comments describing the flow/mechanism and log when we have to fallback to pass through mode. How would priority be set by users, i.e. using topology tuples would mean same dax/sof binaries could be used but with different priorities depending on target.
5af4619 to
a50b365
Compare
Sure, see my latest commit. |
When multiple DAX components are running simultaneously, instance resources may be insufficient. In this case, DAX instance manager ensures that higher-priority DAX components are always granted instance resources. For DAX components that cannot obtain instance resources in a timely manner, `dax_process` will operate in pass-through mode. Signed-off-by: Jun Lai <jun.lai@dolby.com>
a50b365 to
2b0800b
Compare
|
To maintain code readability, I have extracted the entire instance resource handling logic into the file |
|
Code Logic: Assume that instance 1 is created first. While Instance 1 is running, Instance 2 requests to be created. If Instance 2 has a higher priority than Instance 1, then during Instance 1's process loop, Once Instance 2 finishes its work and releases the memory, if Instance 1 is still active, Note: The speaker instance has a higher priority than the headphone instance. |
| if (inst_mgr.users[i].id != DAX_USER_ID_INVALID && | ||
| !inst_mgr.users[i].allocated) { | ||
| inst_mgr.available_inst_cnt++; | ||
| } |
There was a problem hiding this comment.
Hmm, is this a bit racy? Consider: A single DAX component registers, its first establish_instance call hits ENOMEM (e.g., tight memory). The code sets available_inst_cnt = 1 (one non-allocated user). On the next process loop, check_priority_l sees allocated_cnt=0 < available_inst_cnt=1, returns true, and the allocation is retried — on every call.
There was a problem hiding this comment.
Oh, thanks, @kv2019i , the condition shoud be:
if (inst_mgr.users[i].id != DAX_USER_ID_INVALID && inst_mgr.users[i].allocated) {...}That's my bad.
| * | ||
| * @return 0 on success, negative error code on failure. | ||
| */ | ||
| int register_user(struct processing_module *mod); |
There was a problem hiding this comment.
Consider adding a "dax_" prefix. This is added to global namespace and "register_user" could coflict with other software modules.
There was a problem hiding this comment.
Thanks, good ideas
| * | ||
| * @return 0 on success, negative error code on failure. | ||
| */ | ||
| int unregister_user(struct processing_module *mod); |
| * | ||
| * @return 0 on success, negative error code on failure. | ||
| */ | ||
| int check_and_update_instance(struct processing_module *mod); |
| */ | ||
| int32_t available_inst_cnt; | ||
|
|
||
| struct k_spinlock lock; |
There was a problem hiding this comment.
This is a more future-proofing type of comment and not blocking for the PR. We have multiple efforts ongoing to support running some of the audio modules in Zephyr user-space threads on future SOF platforms. We don't yet have documentation on the requirements to audio modules on this. But, but, spinlocks won't be available to user threads and currently the audio code is almost free of spinlock use (except for KPB and now potentially DAX). One practical alternative is mutexes (rtos/lock.h , wraps native Zephyr locks). But just a consideration, as said, this is purely a future-oriented comment.
There was a problem hiding this comment.
Since a lock is acquired on every loop process, the context-switching overhead introduced by a mutex may not be efficient.
| ret = establish_instance(mod); | ||
| else | ||
| ret = destroy_instance(mod); | ||
|
|
There was a problem hiding this comment.
As the lock is dropped here, it would be good to add a comment how the code handles the scenario where priority circumstances have changed. E.g. while running the blocking "establisb_instance()", the "has_priority" priority has become stale. Not necessarily a problem, but good to add a comment you've considered this.
There was a problem hiding this comment.
The comment will be added:
/* This section of logic do not need lock protection because
* it does not involve access to any shared resources. `has_priority`
* may be stale when using in if condition but it is OK, `update_allocation_l`
* will synchronize instance state which will be used in the next check phase.
*/
When multiple DAX components are running simultaneously, instance resources may be insufficient. In this case, DAX instance manager ensures that higher-priority DAX components are always granted instance resources. For DAX components that cannot obtain instance resources in a timely manner,
dax_processwill operate in pass-through mode.