Add rate and channels to ALH config by bardliao · Pull Request #1943 · thesofproject/linux

bardliao · 2020-03-27T11:23:12Z

FW needs to know rate and channels for ALH dai. @slawblauciak will create the corresponding PR on FW side.

singalsu

Thanks!

A similar update is needed also for HDA.

slawblauciak · 2020-03-27T12:06:29Z

I will push the FW change next monday. Our CI is still being in the process of clean up.

plbossart · 2020-03-27T14:08:53Z

Can someone please explain why this is needed for ALH, and why it is needed for the HDaudio link DMA? we've worked for other 6 months without the information, why is this needed and why now?

kv2019i · 2020-03-27T14:16:25Z

@singalsu wrote:

Can someone please explain why this is needed for ALH, and why it is needed for the HDaudio link DMA? we've worked for other 6 months without the information, why is this needed and why now?

@singalsu can explain better, but I've understood these parameters are used by some audio processing elements in the pipeline, e.g. ASRC. So if you don't have those in the pipeline, things have worked ok. But e.g. AHL + ASRC pipeline will hit this.

plbossart · 2020-03-27T14:18:56Z

@singalsu wrote:
Can someone please explain why this is needed for ALH, and why it is needed for the HDaudio link DMA? we've worked for other 6 months without the information, why is this needed and why now?
@singalsu can explain better, but I've understood these parameters are used by some audio processing elements in the pipeline, e.g. ASRC. So if you don't have those in the pipeline, things have worked ok. But e.g. AHL + ASRC pipeline will hit this.

We are just trying to make ALH work for now, ASRC is not needed in the foreseable future on such platforms, all clocks are owned by the Intel chip. same for HDAudio, the clock is owned by the controller so why worry about ASRC?

slawblauciak · 2020-03-27T14:20:59Z

@plbossart demux also suffers from this. Not just ASRC. And we need demux for synchronized playback.

singalsu · 2020-03-27T14:33:47Z

@plbossart Also normal SRC needs the parameters from downstream. DAI parameters are propagated to pipeline from downstream.

plbossart · 2020-03-27T16:10:46Z

 	config.dai_index = (link_id << 8) | (d->id);
 	config.alh.stream_id = alh_stream_id;
+	config.alh.rate = params_rate(params_data->hw_params);
+	config.alh.channels = params_channels(params_data->hw_params);


@bardliao who configures the rate and channels present in the hw_params? it's not part of the topology definitions, so where does this come from? the front-end?
Say e.g. that you want to push 1ch on ALH1.2 and 1ch on ALH 2.2, how would this work?
I am not against providing the info to the firmware, but it seems a bit too magical for now. Thanks for details.

@bardliao who configures the rate and channels present in the hw_params? it's not part of the topology definitions, so where does this come from? the front-end?
Say e.g. that you want to push 1ch on ALH1.2 and 1ch on ALH 2.2, how would this work?
It comes from soc core. The flow is soc_pcm_hw_params() -> soc_soc_dai_hw_params() ->intel_hw_params() -> intel_params_stream() -> sdw_params_stream().
All ALH dais on the same PCM will get the same hw_params. But, we can configure it to what we want in sdw_params_stream().

@bardliao aren't you making big assumptions about what the demux does here and how it's configured? The SOC code does not have any notion of demux/aggregration, so if the demux configuration was to pass 1 channel to ALH1.2 and 1 channel to ALH2.2, then what would the firmware do?
We also need to consider the capture case, if you have multiple devices that capture from 2 or more devices, then do you really want to send to each ALH DAI the configuration that comes from the SOC core?
I must be missing something here.

@bardliao aren't you making big assumptions about what the demux does here and how it's configured? The SOC code does not have any notion of demux/aggregration, so if the demux configuration was to pass 1 channel to ALH1.2 and 1 channel to ALH2.2, then what would the firmware do?
We also need to consider the capture case, if you have multiple devices that capture from 2 or more devices, then do you really want to send to each ALH DAI the configuration that comes from the SOC core?
I must be missing something here.
Let's config it on topology :)

kv2019i

@bardliao Code change looks sensible to me. Where's the matching FW change? I couldn't find it in fw PR list and it's not listed in ABI classifier.

slawblauciak · 2020-03-30T09:11:40Z

FW change wasn't up yet. It is now:
thesofproject/sof#2666

slawblauciak · 2020-04-02T10:05:34Z

@bardliao definitely at least channels. Maybe rate too. Dunno about bit depth, I'm not familiar with HDA enough to be able to tell.

kv2019i · 2020-04-03T09:05:03Z

@slawblauciak wrote:

@bardliao definitely at least channels. Maybe rate too. Dunno about bit depth, I'm not familiar with HDA enough to be able to tell.

Ok that raises a concern. For @singalsu as well. If the FW arch assumes that DAIs can tell the channel count and rate to pipeline components to work correctly, wouldn't these parameters belong to sof_ipc_dai_config? I mean if we now fix ALH and HDA to have this data, how about IMX DAIs (@dbaluta )?

bardliao · 2020-04-09T09:39:57Z

I added the HDA change to this PR with ABI change. @lgirdwood Could you confirm the ABI number?

slawblauciak · 2020-04-09T09:42:42Z

 /* SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 15
+#define SOF_ABI_MINOR 16


The FW change was made with minor version 15

The FW change was made with minor version 15
Isn't minor version 15 for "ipc: cc_version: use fixed length for CC_DESC"? And I think the HDA change is not ready on FW side, right? I am not sure if we can use the same minor version with more than one ipc changes.

The FW change for ALH has already been merged and it uses minor version 15. Maybe better to split this change in that case.

This can still go in 15. Please update to use 15.

This can still go in 15. Please update to use 15.
Got it. Thanks @lgirdwood

Aligned with FW change. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1943) Signed-off-by: Hui Wang <hui.wang@canonical.com>

FW will need these params. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1943) Signed-off-by: Hui Wang <hui.wang@canonical.com>

bardliao · 2020-04-10T10:24:43Z

Updated @plbossart @kv2019i @ranj063 @slawblauciak @singalsu

Aligned with FW change. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

FW will need these params Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2020-04-13T01:10:14Z

HDA part removed. @plbossart @kv2019i @ranj063 I think the PR is ready for merge now. @lgirdwood has confirmed that this can still go in ABI 3.15.0. I will create another PR for the HDA part.

kv2019i · 2020-04-14T12:46:14Z

For some reason the FW change does not show on the ABI classifier, but it's indeed merged: thesofproject/sof#2666
So looks ok, let's merge this.

kv2019i · 2020-04-14T13:03:03Z

Note to @bardliao -- I didn't notice this before, but the git commit messages are really, really short. I think we should have a summary of why the change is needed, on the Linux side commits as well. Now we have good description of the problem and solution in FW side commit messages, but on Linux side we just have a "FW needs these params" with no reference to what problem is being solved.

As I didn't catch this before and other reviewer's seem ok, I'll go ahead and merge. But let's pay more attention this ins future patches.

BugLink: https://bugs.launchpad.net/bugs/1872916 Aligned with FW change. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1943) Signed-off-by: Hui Wang <hui.wang@canonical.com>

BugLink: https://bugs.launchpad.net/bugs/1872916 FW will need these params. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1943) Signed-off-by: Hui Wang <hui.wang@canonical.com>

bardliao added the ABI involves ABI changes, need extra attention label Mar 27, 2020

bardliao requested review from RanderWang, plbossart and slawblauciak March 27, 2020 11:23

bardliao requested review from dbaluta, juimonen, kv2019i, lgirdwood, ranj063 and singalsu as code owners March 27, 2020 11:23

slawblauciak previously approved these changes Mar 27, 2020
View reviewed changes

singalsu previously approved these changes Mar 27, 2020
View reviewed changes

plbossart reviewed Mar 27, 2020
View reviewed changes

ranj063 mentioned this pull request Mar 27, 2020
ASRC: SRC: Fail params() when DAI does not provide sink stream format information thesofproject/sof#2642
Merged

sinahuang mentioned this pull request Mar 28, 2020
[BUG][CML]Aplay0,2 rt1308 right speaker have sound output when muted speaker in alsamixer. #1953
Closed

RanderWang previously approved these changes Mar 30, 2020
View reviewed changes

YvonneYang2 mentioned this pull request Mar 30, 2020
[BUG][CML]DSP Panic occurred when pause/resume on aplay0,2(RT1308) thesofproject/sof#2658
Closed

lyakh previously approved these changes Mar 30, 2020
View reviewed changes

kv2019i requested changes Mar 30, 2020
View reviewed changes

bardliao dismissed stale reviews from lyakh, RanderWang, singalsu, and slawblauciak via 85744b5March 31, 2020 12:00

This was referenced Apr 7, 2020
[BUG][CML][tplg]"error: ipc error for 0x60010000 size 20" happened after booting up successfully. thesofproject/sof#2690
Closed
[BUG][CML-H]" error: ipc error for 0x60010000 size 20" display when aplay via speaker0,2. thesofproject/sof#2633
Closed

bardliao force-pushed the alh-params branch from 54f875d to ae17c88Compare April 9, 2020 09:35

bardliao requested review from RanderWang, lyakh, plbossart, singalsu and slawblauciak April 9, 2020 09:40

slawblauciak reviewed Apr 9, 2020
View reviewed changes

bardliao force-pushed the alh-params branch from ae17c88 to 53f9b67Compare April 10, 2020 10:20

bardliao added 2 commits April 13, 2020 09:00

ASoC: SOF: align sof_ipc_dai_alh_params with FW
6d15fe0
Aligned with FW change. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: SOF: topology: Get ALH rate amd channels from topology
78c0ca1
FW will need these params Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the alh-params branch from 53f9b67 to 78c0ca1Compare April 13, 2020 01:01

bardliao mentioned this pull request Apr 13, 2020
Add rate and channels to HDA config #2007
Merged

plbossart approved these changes Apr 13, 2020
View reviewed changes

lyakh approved these changes Apr 14, 2020
View reviewed changes

slawblauciak approved these changes Apr 14, 2020
View reviewed changes

kv2019i approved these changes Apr 14, 2020
View reviewed changes

kv2019i merged commit d9b571b into thesofproject:topic/sof-devApr 14, 2020

Uh oh!

Conversation

bardliao commented Mar 27, 2020

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

slawblauciak commented Mar 27, 2020

Uh oh!

plbossart commented Mar 27, 2020

Uh oh!

kv2019i commented Mar 27, 2020

Uh oh!

plbossart commented Mar 27, 2020

Uh oh!

slawblauciak commented Mar 27, 2020

Uh oh!

singalsu commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

slawblauciak commented Mar 30, 2020

Uh oh!

slawblauciak commented Apr 2, 2020

Uh oh!

kv2019i commented Apr 3, 2020

Uh oh!

bardliao commented Apr 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Apr 10, 2020

Uh oh!

bardliao commented Apr 13, 2020

Uh oh!

kv2019i commented Apr 14, 2020

Uh oh!

kv2019i commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants