Skip to content

ASoC: SOF: ipc3-topology: overwrite ALH dai index #4036

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

Closed
bardliao wants to merge 1 commit into from
Closed

ASoC: SOF: ipc3-topology: overwrite ALH dai index #4036

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
5 changes: 5 additions & 0 deletions sound/soc/sof/ipc3-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2081,8 +2081,13 @@ static int sof_ipc3_dai_config(struct snd_sof_dev *sdev, struct snd_sof_widget *
break;
case SOF_DAI_INTEL_ALH:
if (data) {
struct sof_dai_private_data *dai_data = dai->private;
struct sof_ipc_comp_dai *comp_dai = dai_data->comp_dai;

config->dai_index = data->dai_index;
config->alh.stream_id = data->dai_data;
/* comp_dai->dai_index was from topology, overwrite with the right index. */
comp_dai->dai_index = config->dai_index;
Copy link
Collaborator

@ujfalusi ujfalusi Nov 24, 2022

Choose a reason for hiding this comment

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

How we are not counting dai indexes with this change? This patch does something else than the commit message tells?

Copy link
Collaborator Author

@bardliao bardliao Nov 25, 2022

Choose a reason for hiding this comment

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

How we are not counting dai indexes with this change? This patch does something else than the commit message tells?

Currently, we calculate dai index in topology, like eval(UAJ_LINK * 256 + 2). That is to match the formula data.dai_index = (params_data->link_id << 8) | d->id; in sdw_params_stream. With this commit, we will get dai_index from sdw_params_stream and that's why we don't need to calculate dai index in topology.

Copy link
Collaborator

@ujfalusi ujfalusi Nov 25, 2022

Choose a reason for hiding this comment

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

Right, but I don't see this calculation. Is it in the firmware's topology section or where?
It is confusing to mention something which is not visible.

Can there be disconnect (and issue) if the somewhere calculated dai_index is not matching with dai_index calculated and stored via sdw_params_stream?
And btw, I don't see any sdw_params_stream in this patch either..

Copy link
Collaborator Author

@bardliao bardliao Nov 25, 2022

Choose a reason for hiding this comment

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

Right, but I don't see this calculation. Is it in the firmware's topology section or where? It is confusing to mention something which is not visible.

Yes, the calculation is in topology. We use link id and data port id to calculate the dai index and the formula is dai_index = (link_id * 256 + data_port_id)
It is not visible in the patch because the calculation is already on kernel side but we didn't use it.
I will try to explain it more in the commit message.

Can there be disconnect (and issue) if the somewhere calculated dai_index is not matching with dai_index calculated and stored via sdw_params_stream? And btw, I don't see any sdw_params_stream in this patch either..

Yes, the firmware will be disconnected if the dai index is not correct. The firmware needs dai index to get SoundWire link id and data port. Just like SSP dai index matches SSP port.
sdw_params_stream is in sound/soc/sof/intel/hda.c, and the patch doesn't touch it.

Copy link
Collaborator

@ujfalusi ujfalusi Nov 25, 2022

Choose a reason for hiding this comment

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

@bardliao, hrm, so this was pretty much broken before, right?
I mean currently we use two numbers as dai_index, coming from independent sources and they are identical "by luck" (I know, it is a bit more than luck).
It would have been correct if:

struct sof_dai_private_data *dai_data = dai->private;
struct sof_ipc_comp_dai *comp_dai = dai_data->comp_dai;

config->dai_index = comp_dai->dai_index;
config->alh.stream_id = data->dai_data;

So taking the dai_index for both struct from the same source.

I would further clarify the commit message to:

ASoC: SOF: ipc3-topology: Ignore the dai_index for ALH from topology

Overwrite the dai_index in the struct sof_ipc_comp_dai from the topology file
with the platform code provided one, which is already used for the DAI config
IPC message.

With this change we can be sure that the dai_index is coming from the same
source.

or something.

Copy link
Collaborator Author

@bardliao bardliao Nov 28, 2022

Choose a reason for hiding this comment

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

I would further clarify the commit message to:

ASoC: SOF: ipc3-topology: Ignore the dai_index for ALH from topology

Overwrite the dai_index in the struct sof_ipc_comp_dai from the topology file
with the platform code provided one, which is already used for the DAI config
IPC message.

With this change we can be sure that the dai_index is coming from the same
source.

or something.

Commit message updated.

Copy link
Collaborator

@ranj063 ranj063 Nov 28, 2022

Choose a reason for hiding this comment

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

@bardliao I'd still argue that this change is not really needed. But if you insist, I'd go one step further and also remove the dai_index setting on line 1540 to absolutely make sure that we do not use the index coming from topology.

Copy link
Member

@plbossart plbossart Nov 28, 2022

Choose a reason for hiding this comment

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

@bardliao it would help to get a bit more context on what we do on topology.

I see that we use the arithmetic in multiple places

DAI_ADD(sof/pipe-dai-playback.m4,
	3, ALH, eval(AMP_1_LINK * 256 + 2),

DAI_CONFIG(ALH, eval(UAJ_LINK * 256 + 2), 0, `SDW'eval(UAJ_LINK)`-Playback',
	ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(UAJ_LINK * 256 + 2), 48000, 2)))

I have no idea which ones of those evals are required....

Copy link
Collaborator Author

@bardliao bardliao Nov 30, 2022

Choose a reason for hiding this comment

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

@bardliao it would help to get a bit more context on what we do on topology.

I see that we use the arithmetic in multiple places

DAI_ADD(sof/pipe-dai-playback.m4,
	3, ALH, eval(AMP_1_LINK * 256 + 2),

DAI_CONFIG(ALH, eval(UAJ_LINK * 256 + 2), 0, `SDW'eval(UAJ_LINK)`-Playback',
	ALH_CONFIG(ALH_CONFIG_DATA(ALH, eval(UAJ_LINK * 256 + 2), 48000, 2)))

I have no idea which ones of those evals are required....

 DAI_ADD(sof/pipe-dai-playback.m4,
 	3, ALH, eval(AMP_1_LINK * 256 + 2),

The above is for dai index, so it is required.

And the other seems to be not required. I changed it and the topology still worked.

}
break;
default:
Expand Down