[RFC] ASoC: SOF: ipc4-topology: reserve alh node_id for dai module #4794
Conversation
RanderWang
requested review from
lgirdwood,
plbossart,
ranj063,
kv2019i,
dbaluta,
bardliao and
ujfalusi
as code owners
| @@ -1283,9 +1283,6 @@ static void sof_ipc4_unprepare_copier_module(struct snd_sof_widget *swidget) | |||
| ALH_MULTI_GTW_BASE; | |||
| ida_free(&alh_group_ida, group_id); | |||
| } | |||
|
|
|||
| /* clear the node ID */ | |||
| copier_data->gtw_cfg.node_id &= ~SOF_IPC4_NODE_INDEX_MASK; | |||
There was a problem hiding this comment.
The commit message is rather convoluted: there is nothing remotely close to something being 'reserved'.
you are not clearing a value, but that does not quite explain the problem, and it's weird to leave something initialized unconditionally.
I wonder if this value should be cleared when the use_count becomes zero?
There was a problem hiding this comment.
The node_id was cleared by this commit : fixup! ASoC: SOF: sof-audio: Fix broken early bclk feature for SSP 5e9a856. I don't get it.
The use_count is zero when the node_id is cleared, or it can't be cleared.
Let's talk about our processing
(1) stream open
(2) stream hw_params, set up fw module here, use_count ++
(3) stream free, free and unprepare the fw module here, use_count --
This bug happens with stream A at step (3), stream B at step (1), so we can't depend on use_count.
If we want to use use_count, we need to set up fw module in stream open (Asoc set up audio path in snd_pcm_open), at least part of fw module since we don't have enough hw_params information here. This is not done by both IPC3 & IPC4
There was a problem hiding this comment.
The commit message is rather convoluted: there is nothing remotely close to something being 'reserved'.
you are not clearing a value, but that does not quite explain the problem, and it's weird to leave something initialized unconditionally.
Our fw module are initialized by topology setting when SOF is loaded. As I remember node_id is initialized by topology in IPC3. @bardliao do you remember this ?
There was a problem hiding this comment.
The commit message is rather convoluted: there is nothing remotely close to something being 'reserved'.
you are not clearing a value, but that does not quite explain the problem, and it's weird to leave something initialized unconditionally.Our fw module are initialized by topology setting when SOF is loaded. As I remember node_id is initialized by topology in IPC3. @bardliao do you remember this ?
For IPC3, yes, the dai_index is from topology.
There was a problem hiding this comment.
Why don't we set the node_id unconditionally before sending a message which needs it?
If the node_id got cleared in between setting it in the message and sending it then we have way bigger problems with race...
There was a problem hiding this comment.
Why don't we set the node_id unconditionally before sending a message which needs it? If the node_id got cleared in between setting it in the message and sending it then we have way bigger problems with race...
hi @bardliao , please help to make it clear, thanks!
I think it should be fine if we set the node_id unconditionally before sending a message which needs it. But @ranj063 is the best person to answer this question.
There was a problem hiding this comment.
The node_id was cleared by this commit : fixup! ASoC: SOF: sof-audio: Fix broken early bclk feature for SSP 5e9a856. I don't get it.
The use_count is zero when the node_id is cleared, or it can't be cleared. Let's talk about our processing (1) stream open (2) stream hw_params, set up fw module here, use_count ++ (3) stream free, free and unprepare the fw module here, use_count --
This bug happens with stream A at step (3), stream B at step (1), so we can't depend on use_count.
@RanderWang I think this observation is incorrect. Is Stream A has already finished with hw_free because of which the widget has been freed and unprepared, wouldn't Stream B's hw_params take care of preparing the widget?
There was a problem hiding this comment.
@ranj063 check this https://github.com/thesofproject/sof/issues/8056#issuecomment-1899870604. Stream A free widget before Stream B's hw_params happen. If Stream B's hw_params happen before Stream A free widget, no issue.
If I misunderstand your idea, please help to explain it again. Thanks!
There was a problem hiding this comment.
The node_id is set by SDW hw_params, not by sof_hw_params.
There was a problem hiding this comment.
@RanderWang so it looks like we have a race between hw_free and hw_params for the same BE by two separate FE's isnt it? Not clearing the node ID, while it will work, seems like a hack and not an actual fix. Would it make sense to instead protect the be_dai_hw_params and be_dai_hw_free with the dpcm_mutex_lock for the be instead?
@plbossart what do you think?
RanderWang
force-pushed
the
node_id_fix
branch
from
4fcff5a to
64a73d7
Compare
Fix dai module initialization failure since node_id is zero. Two pcm streams share the same be dai module. When stream a is being closed and the other stream b is open, the dai module is first unprepared by stream a and the node_id is cleared. And then be hw_params function is called for stream b in asoc framework and found be dai component is shared by stream a and b, so the hw_params function for be dai component is not called. The node_id is always set by be dai component so it will be missed. Each BE has its own unique intel_alh_id aka node_index based on the link number and pdi number. So node_id will not change for a specific BE. This patch reserves alh node_id for dai module in firmware based on this fact. Signed-off-by: Rander Wang <[email protected]>
RanderWang
force-pushed
the
node_id_fix
branch
from
64a73d7 to
37d4eba
Compare
RanderWang
changed the title
|
@RanderWang can u please check if #4816 fixes the issues you're trying to address? |
@ranj063 The bug rarely happen so I can't 100% reproduce it. CI test only caught it every a few weeks. |
Fix dai module initialization failure since node_id is zero. Two pcm streams share the same be dai module. When stream a is being closed and the other stream b is open, the dai module is first unprepared by stream a and the node_id is cleared. And then be hw_params function is called for stream b in asoc framework and found be dai component is shared by stream a and b, so the hw_params functon for be dai component is not called. The node_id is always set by be dai component so it will be missed.
Each BE has its own unique intel_alh_id aka node_index based on the link number and pdi number. So node_id will not change for a specific BE. This patch reserves alh node_id for dai module in firmware based on this fact.
Fix #4832