ASoC: SOF: topology: config ALH dai in sof_widget_load_dai by bardliao · Pull Request #1976 · thesofproject/linux

bardliao · 2020-04-02T08:56:37Z

sof_link_load() is only invoked for only one FE dai With multi-cpu DAI link
supported. Therefore, we need to config the dai when it is loaded.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

sof_link_load() is only invoked for only one FE dai With multi-cpu DAI link supported. Therefore, we need to config the dai when it is loaded. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart

@bardliao need more explanations on why ALH configuration is done in different places, that doesn't seem right to me

plbossart · 2020-04-02T12:40:50Z

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


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

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

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?

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

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

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.

@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

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

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

lyakh

just minor nitpicks, but it looks like there might be larger changes coming to this PR

lyakh · 2020-04-03T06:50:27Z

+			ret);
+		return ret;
+	}
+OUT:


I don't think we use capitalised labels.

lyakh · 2020-04-03T06:51:09Z

+	 * ipc for them
+	 */
+	if (!config.alh.rate && !config.alh.channels)
+		goto OUT;


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

lyakh · 2020-04-03T06:51:49Z

 	}

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


just a return should do.

bardliao · 2020-04-06T07:46:43Z

just minor nitpicks, but it looks like there might be larger changes coming to this PR

Thanks @lyakh for the review. Yes, we might have a larger change so I am not going to modify this PR before we have consensus :)

bardliao · 2020-04-07T06:00:15Z

Replaced by #1982

Fix the following crash due to erroneous page refcounting: [ 32.445919] BUG: Bad page state in process swapper/1 pfn:11f65a [ 32.447409] page:00000000938f0632 refcount:0 mapcount:-128 mapping:0000000000000000 index:0x0 pfn:0x11f65a [ 32.449605] flags: 0x8000000000000000() [ 32.450421] raw: 8000000000000000 ffffffff825b0148 ffffea00045ae988 0000000000000000 [ 32.451795] raw: 0000000000000000 0000000000000001 00000000ffffff7f 0000000000000000 [ 32.452999] page dumped because: nonzero mapcount [ 32.453888] Modules linked in: [ 32.454492] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.11.0-rc2+ #1976 [ 32.455695] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-1.fc33 04/01/2014 [ 32.457157] Call Trace: [ 32.457636] <IRQ> [ 32.457993] dump_stack+0x77/0x97 [ 32.458576] bad_page.cold+0x65/0x96 [ 32.459198] get_page_from_freelist+0x46a/0x11f0 [ 32.460008] __alloc_pages_nodemask+0x10a/0x2b0 [ 32.460794] mt7601u_rx_tasklet+0x651/0x720 [ 32.461505] tasklet_action_common.constprop.0+0x6b/0xd0 [ 32.462343] __do_softirq+0x152/0x46c [ 32.462928] asm_call_irq_on_stack+0x12/0x20 [ 32.463610] </IRQ> [ 32.463953] do_softirq_own_stack+0x5b/0x70 [ 32.464582] irq_exit_rcu+0x9f/0xe0 [ 32.465028] common_interrupt+0xae/0x1a0 [ 32.465536] asm_common_interrupt+0x1e/0x40 [ 32.466071] RIP: 0010:default_idle+0x18/0x20 [ 32.468981] RSP: 0018:ffffc90000077f00 EFLAGS: 00000246 [ 32.469648] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000 [ 32.470550] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff81aac3dd [ 32.471463] RBP: ffff88810022ab00 R08: 0000000000000001 R09: 0000000000000001 [ 32.472335] R10: 0000000000000046 R11: 0000000000005aa0 R12: 0000000000000000 [ 32.473235] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 32.474139] ? default_idle_call+0x4d/0x200 [ 32.474681] default_idle_call+0x74/0x200 [ 32.475192] do_idle+0x1d5/0x250 [ 32.475612] cpu_startup_entry+0x19/0x20 [ 32.476114] secondary_startup_64_no_verify+0xb0/0xbb [ 32.476765] Disabling lock debugging due to kernel taint Fixes: c869f77 ("add mt7601u driver") Co-developed-by: Felix Fietkau <nbd@nbd.name> Signed-off-by: Felix Fietkau <nbd@nbd.name> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Acked-by: Jakub Kicinski <kubakici@wp.pl> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Link: https://lore.kernel.org/r/62b2380c8c2091834cfad05e1059b55f945bd114.1610643952.git.lorenzo@kernel.org

commit d24c790 upstream. Fix the following crash due to erroneous page refcounting: [ 32.445919] BUG: Bad page state in process swapper/1 pfn:11f65a [ 32.447409] page:00000000938f0632 refcount:0 mapcount:-128 mapping:0000000000000000 index:0x0 pfn:0x11f65a [ 32.449605] flags: 0x8000000000000000() [ 32.450421] raw: 8000000000000000 ffffffff825b0148 ffffea00045ae988 0000000000000000 [ 32.451795] raw: 0000000000000000 0000000000000001 00000000ffffff7f 0000000000000000 [ 32.452999] page dumped because: nonzero mapcount [ 32.453888] Modules linked in: [ 32.454492] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.11.0-rc2+ thesofproject#1976 [ 32.455695] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-1.fc33 04/01/2014 [ 32.457157] Call Trace: [ 32.457636] <IRQ> [ 32.457993] dump_stack+0x77/0x97 [ 32.458576] bad_page.cold+0x65/0x96 [ 32.459198] get_page_from_freelist+0x46a/0x11f0 [ 32.460008] __alloc_pages_nodemask+0x10a/0x2b0 [ 32.460794] mt7601u_rx_tasklet+0x651/0x720 [ 32.461505] tasklet_action_common.constprop.0+0x6b/0xd0 [ 32.462343] __do_softirq+0x152/0x46c [ 32.462928] asm_call_irq_on_stack+0x12/0x20 [ 32.463610] </IRQ> [ 32.463953] do_softirq_own_stack+0x5b/0x70 [ 32.464582] irq_exit_rcu+0x9f/0xe0 [ 32.465028] common_interrupt+0xae/0x1a0 [ 32.465536] asm_common_interrupt+0x1e/0x40 [ 32.466071] RIP: 0010:default_idle+0x18/0x20 [ 32.468981] RSP: 0018:ffffc90000077f00 EFLAGS: 00000246 [ 32.469648] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000 [ 32.470550] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff81aac3dd [ 32.471463] RBP: ffff88810022ab00 R08: 0000000000000001 R09: 0000000000000001 [ 32.472335] R10: 0000000000000046 R11: 0000000000005aa0 R12: 0000000000000000 [ 32.473235] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 32.474139] ? default_idle_call+0x4d/0x200 [ 32.474681] default_idle_call+0x74/0x200 [ 32.475192] do_idle+0x1d5/0x250 [ 32.475612] cpu_startup_entry+0x19/0x20 [ 32.476114] secondary_startup_64_no_verify+0xb0/0xbb [ 32.476765] Disabling lock debugging due to kernel taint Fixes: c869f77 ("add mt7601u driver") Co-developed-by: Felix Fietkau <nbd@nbd.name> Signed-off-by: Felix Fietkau <nbd@nbd.name> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Acked-by: Jakub Kicinski <kubakici@wp.pl> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Link: https://lore.kernel.org/r/62b2380c8c2091834cfad05e1059b55f945bd114.1610643952.git.lorenzo@kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

ASoC: SOF: topology: config ALH dai in sof_widget_load_dai

afc5ab7

sof_link_load() is only invoked for only one FE dai With multi-cpu DAI link supported. Therefore, we need to config the dai when it is loaded. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao requested review from RanderWang and plbossart April 2, 2020 08:56

bardliao requested review from juimonen, ranj063 and singalsu as code owners April 2, 2020 08:56

plbossart requested changes Apr 2, 2020

View reviewed changes

lyakh requested changes Apr 3, 2020

View reviewed changes

bardliao mentioned this pull request Apr 6, 2020

ASoC: SOF: topology: send ipc for all found DAIs in sof_set_dai_config #1982

Merged

bardliao closed this Apr 7, 2020

Conversation

bardliao commented Apr 2, 2020

Uh oh!

plbossart left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

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 6, 2020

Uh oh!

bardliao commented Apr 7, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants