Skip to content

ASoC: SOF: topology: config ALH dai in sof_widget_load_dai #1976

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: topology: config ALH dai in sof_widget_load_dai #1976

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
39 changes: 39 additions & 0 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,6 +1323,9 @@ static int sof_widget_load_dai(struct snd_soc_component *scomp, int index,
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_tplg_private *private = &tw->priv;
struct sof_ipc_comp_dai comp_dai;
struct sof_ipc_dai_config config;
struct sof_ipc_reply reply;
u32 size = sizeof(config);
int ret;

/* configure dai IPC message */
Expand Down Expand Up @@ -1364,6 +1367,42 @@ static int sof_widget_load_dai(struct snd_soc_component *scomp, int index,
memcpy(&dai->comp_dai, &comp_dai, sizeof(comp_dai));
}

if (comp_dai.type != SOF_DAI_INTEL_ALH)
goto OUT;

@lyakh lyakh Apr 3, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just a return should do.


/*
* Some ALH DAIs are not configured by sof_link_alh_load(), so
* configure them here
*/

@plbossart plbossart Apr 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?? I really wasn't able to understand this and why we need to have ALH code in here. That looks rather bad to be honest.

@bardliao bardliao Apr 2, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart I must admit that is a little hacky. The issue is that sof_link_load() -> sof_link_alh_load() will only be called when a FE link is created. But in our multi cpu dai link case , there are two FE DAIs in a link. As a result the second DAI will not be configured. Maybe a better solution is to move all DAI config from sof_link_load() to sof_widget_load_dai() since it is "DAI" config not "link" config.

@plbossart plbossart Apr 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But all the DAI_CONFIG we have in topology is actually dai-link config...
I guess I need a lot more coffee today, or call the cavalry. @ranj063 what do you think?

@ranj063 ranj063 Apr 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao @plbossart sof_link_load() is called every FE and BE DAI links. I think whats missing is something like what we do in sof_link_hda_process() for all DAIs with the same name as the link name. We should not be doing this here.

@plbossart plbossart Apr 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ranj063 but this case is different from HDaudio, we have multiple cpu- and codec-dais per link, so a name-based solution cannot possibly work, can it?

@plbossart plbossart Apr 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My remark was a bit hypothetical. But it's possible that in the future we have a cpu-dai configuration to use Lane0 on one segment and Lane1 on the other. Of if we have a dailink with two synchronized SSPs (possible with TGL+), that we use different slots for different cpu-dais, specifically on capture.
I think everything from sample rate to number of channels will typically be the same, it's the link configurations that could vary.
I am a bit of the fence here on whether to deal with this now. maybe we just make this assumption for now, document it explicitly and think later about how we might want to sort this out. Unless you have a solution in the next day or so it may be the right path forward.

@ranj063 ranj063 Apr 6, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@plbossart is it only the SSPs that would have the need for different slot info? Or do we envision other types of DAIs to also have different config for the different DAIs

@ranj063 ranj063 Apr 6, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If it only the SSPs, then can we possibly think of specifying the different hw_config for different DAIs and use the ID info in the hw_config to match which DAI we want the hw_config to be applicable to instead of the default_hw_config. Just thinking out loud.

@bardliao bardliao Apr 7, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bardliao Im not sure how you could have different config for DAIs on the same DAI link.

What about a link with subwoofer amp? Something like 3 dais, two of them are 2 channels (normal amp) and one of them is 1 channel (subwoofer amp). We don't have this feature for now, but maybe our DSP can do it one day.

@plbossart but then the topology parsing logic today is flawed isnt it?

I have to say yes, unfortunately. That's why I was thinking if we should move xxx_CONFIG_DATA from DAI_CONFIG to DAI_ADD in the topology file and do the corresponding change on kernel side. But it will be a big change. So I prefer to make an assumption that all dais in the same link have the same config now and do the change when we really need it.

@ranj063 ranj063 Apr 7, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao OK lets file an enhancement issue to track this as well

memset(&config, 0, size);
config.hdr.size = size;
config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
config.type = comp_dai.type;
config.dai_index = comp_dai.dai_index;
ret = sof_parse_tokens(scomp, &config.alh, alh_tokens,
ARRAY_SIZE(alh_tokens), private->array,
le32_to_cpu(private->size));
if (ret != 0) {
dev_err(scomp->dev, "error: parse alh tokens failed %d\n",
le32_to_cpu(private->size));
return ret;
}
/*
* Not every DAI has config data here, and we don't need to send
* ipc for them
*/
if (!config.alh.rate && !config.alh.channels)
goto OUT;

@lyakh lyakh Apr 3, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see return statements above and below this goto, why not just return here too?


ret = sof_ipc_tx_message(sdev->ipc,
config.hdr.cmd, &config, size, &reply,
sizeof(reply));
if (ret != 0) {
dev_err(scomp->dev, "error: config alh ipc failed %d\n",
ret);
return ret;
}
OUT:

@lyakh lyakh Apr 3, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we use capitalised labels.

return ret;
}

Expand Down