ASoC: SOF: topology: refine multi-core core status management #888
Conversation
|
@keyonjie we dont do anything if the core is already enabled. Look at linux/sound/soc/sof/intel/hda-dsp.c Line 210 in af71673 As for powering down the core, yes we do try to power it down multiple times if multitple pipelines are scheduled on the same core. But having said that,I do think we should add refcounts for the core and power it down only if the refcount is 0 |
|
@keyonjie it's not clear to me what 'chaos' you are referring to and what practical problem you are trying to fix? We shouldn't put too much logic in the core anyways, different platforms may have different ways of dealing with multiple cores, it's best to do the required housekeeping in platform-related stuff. |
|
@plbossart I issue I want to address here is something like below in dmesg, the DSP core is power down many many times(you can refer to #887 for detail): |
yes, I am concerning about that we might power down Core 0 too early in our case, in module unload/reload. |
There is already a mask to indicate what cores need to be on or off, so what are you asking for? 'some new function' does not describe what the problem is and how it needs to be fixed. |
|
@keyonjie FW is already keeping the count of enabled cores, so this probably won't fix anything, but at least it'll spare us the additional IPC. |
sound/soc/sof/topology.c
Outdated
| * TODO: add ref counts for each core, only power off it when | ||
| * ref counts is decreased by 0. | ||
| */ | ||
| if (!(sdev->enabled_cores_mask & (1 << pipeline->core)) || |
There was a problem hiding this comment.
Do you need locking here around mask ? what happens if a pipeline is started when another topology is being loaded ?
There was a problem hiding this comment.
@lgirdwood that's good point, we might need locking at both load and unload, though we never had that yet.
|
This patch fixes #724 |
keyonjie
requested review from
dbaluta,
juimonen,
lyakh and
singalsu
as code owners
keyonjie
changed the title
|
@plbossart The management to DSP cores could be generic, to implement ef_cnt for each DSP cores to manage the power status in sof core part in #887 should be next step. Here it is only bug fix for the existed solution. Please help review. |
|
On 5/13/19 2:31 AM, Keyon wrote:
@plbossart <https://github.com/plbossart> The management to DSP cores
could be generic, to implement ef_cnt for each DSP cores to manage the
power status in sof core part in #887
<#887> should be next step.
Here it is only bug fix for the existed solution. Please help review.
I don't like these incremental solutions, especially now that the code
is upstream. We should have a patchset that fixes all known multicore
issues in one shot.
|
|
I will refine this later. |
|
Hi all @plbossart @tlauda @ranj063 @RanderWang @xiulipan @lgirdwood the PR is updated to support core ref count, they are verified on boot, fw runtime PM, and module unloading/reloading, please help review. |
I previously used that, @plbossart reminded and I agreed that we don't need the heavier kref here as we already have mutex to protect the same resource, the kref itself doesn't help much here for me:
The kref_put_mutex() can help somewhat here with release callback for power down, but this is the only benefit I can imagine. So here, I think the light int ref count looks better to me. @lyakh @plbossart thoughts? |
|
@keyonjie |
This should be related with FW PR#1394, kernel part already merged, we need merge FW part also, otherwise, this issue will always happen. |
|
Hi @plbossart so do you still have anything unclear about this? |
|
|
||
| return ret; | ||
| /* increase ref count of the DSP core */ | ||
| return snd_sof_dsp_core_get(sdev, pipeline->core); |
There was a problem hiding this comment.
this doesn't look like a very sensible power management idea. You power-up all the cores when the topology is loaded and release them when we unload the topology. They should be power-up when they are used!
There was a problem hiding this comment.
That's true, powering up cores as late as possible and powering down them as soon as possible sounds fantastic, but I am not sure we can support this inside FW ATM, we even haven't verified that configuring a pipeline run on a non-0 core via topology can work as we expected(@tlauda please correct me if I am wrong).
To me, the changes of where we are calling snd_sof_dsp_core_get() is relative simple, we can change that when it is aligned on FW and driver and verified work.
There was a problem hiding this comment.
@keyonjie your write-up suggests that you have not tested this patch in a multi-core configuration?
Also I don't see why we should first do something and then change it because it's a bad design. If you've tested multi-core then you can do the change directly. If you haven't tested and this is an enabler patch then test it further before we merge it.
There was a problem hiding this comment.
@keyonjie your write-up suggests that you have not tested this patch in a multi-core configuration?
Also I don't see why we should first do something and then change it because it's a bad design. If you've tested multi-core then you can do the change directly. If you haven't tested and this is an enabler patch then test it further before we merge it.
Agree, let me close this PR and revisit it when multi-core feature is required and verified work on FW side.
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Need to address comments and powering-up cores on startup does not look like a smart idea. Should be when DAPM signals the pipeline is in use.
Still "Unclear" status, sorry.
|
also the branch has topic/sof-dev merged for some reason, please fix this as well. |
if what you meant is need rebase to latest topic/sof-dev, now it is done. |
|
This is very long, but I'll add a quick note that this PR (at least the part that keeps core0 powered following driver runtime-PM status) would be very useful for common debugging tasks. See #887 (comment) |
Thanks for acking @kv2019i . Actually, I always run on my platforms with the PR applied. :-) |
|
@keyonjie Thanks, this work helped me to trace a topology load time issue in FW. |
@plbossart updated above. |
keyonjie
force-pushed
the
multi_core_rfc
branch
from
49986c3 to
3c0f719
Compare
Add atomic ref counts core_refs for each audio DSP cores, for power management of each core based on usage on it, this is preparation for audio DSP multi-core support. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
We are changing enabled_cores_mask dynamically during pipeline load/unload, there might be race for enabled_cores_mask when a pipeline loading and another pipeline(run on the same core) is unloading. Here introduces a mutex to protect this enabled_cores_mask and core_refs, and use it when never we need to read/write to it. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Initialize ref counts to DSP cores to be 0s, at start of probing. Once FW booted, update the ref counts of dsp cores, that is, for each powered on core, set the ref count of it to be 1. Reset ref counts of DSP cores to be 0s at suspend, to align with the status before next FW boot at resume. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add a general DSP core enable/disable IPC interface, this is now aligned with FW that sending the whole enable_mask, may consider changing to send IPC for specific core only with each IPC in the future when FW change is ready (ABI bump needed). Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
We need get/put interfaces to manage the ref counts of DSP cores, call the real core power up/down, and send ipc to align the core power status with FW, the interfaces are typically used when new modules/pipelines which are assigned to run on a specific DSP core. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Refine DSP multi-core support to use the new defined APIs directlly. For pipeline loading, call snd_sof_dsp_core_get(), it will increase ref count of the specific DSP core, and power on the core when necessary (it is on powered off stage). For pipeline(scheduler widget) unloading, call snd_sof_dsp_core_put(), it will decrease ref count of the specific DSP core, and power off it when necessary (no more usage of it). Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
|
closing since there's been no activity since August 5 |
10 participants
This fixes #724 and #887
We should not power up DSP core when it is already on, vice versa, don't
power down it when it is already off, here add checks to fix it.
Here also add ref_cnt for each DSP cores to manage the power status of those
cores.