Skip to content
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.

Sign up for GitHub

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

[RFC] ASoC: SOF: ipc4-topology: reserve alh node_id for dai module #4794

Closed
wants to merge 1 commit into from
Closed

[RFC] ASoC: SOF: ipc4-topology: reserve alh node_id for dai module #4794

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,7 +1273,6 @@ static void sof_ipc4_unprepare_copier_module(struct snd_sof_widget *swidget)
}

if (ipc4_copier->dai_type == SOF_DAI_INTEL_ALH) {
struct sof_ipc4_copier_data *copier_data = &ipc4_copier->data;
struct sof_ipc4_alh_configuration_blob *blob;
unsigned int group_id;

Expand All @@ -1283,9 +1282,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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@RanderWang RanderWang Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@RanderWang RanderWang Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node_id is set by SDW hw_params, not by sof_hw_params.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

}
}

Expand Down