ASoC: SOF: refine the selection of machine drivers #321

RanderWang · 2018-11-28T03:13:40Z

Now hda machine driver and no codec driver is mutually exclusive.
If hda machine driver is selected in kconfig, no codec driver would
not work. The new algorithm is: if no machine is found at pci or acpi
probe function, no codec driver is appiled first. At the hda probe
time, hda machine driver is applied if there are hda codecs except
hdmi codec.

it is for: #284
Signed-off-by: Rander Wang [email protected]

ranj063 · 2018-11-28T04:31:31Z

sound/soc/sof/intel/hda.c

@RanderWang do we need to care about freeing the memory allocated to mach before doing this?

yes, good point

keyonjie

I suggest to move the nocodec setup part to where after codec_mask detected, there we can avoid setting up no use nocodec machine.

Also, I am not clear about SPI case for the machine driver match logic.

keyonjie · 2018-11-28T05:33:22Z

sound/soc/sof/intel/hda.c

do nocodec setup here:

else sof_nocodec_setup()

please refer to sof_xxx_probe. machine driver carries some information for sof_probe.
And if no codec is found at probe function, a default codec should be used at there, no here

@RanderWang as I see it the logic is:
if force-no-codec-mode
use no-codec machine drive
else
If acpi match found
use the appropriate machine driver
else
use no-codec machine driver

if num-hda-codecs > 1
use hda machine driver

Is this correct? then does this mean we cannot force no-codec machine driver if hda is enabled?

@ranj063 please try CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE.
if hda is enable and no hda codec, it would be nocodec

Now hda machine driver and no codec driver is mutually exclusive. If hda machine driver is selected in kconfig, no codec driver would not work. The new algorithm is: if no machine is found at pci or acpi probe function, no codec driver is appiled first. At the hda probe time, hda machine driver is applied if there are hda codecs except hdmi codec. Signed-off-by: Rander Wang <[email protected]>

RanderWang · 2018-11-28T07:55:34Z

I remove the CONFIG_SND_SOC_SOF_NOCODEC in pci/acpi probe, because it must be enabled, or
mach is NULL and you would get kernel panic at the following code: mach->op = ops. And we need it to make probe go forward to sof_probe for HDA case.

plbossart

No idea how this is supposed to work, see comments below

plbossart · 2018-11-28T10:08:34Z

sound/soc/sof/intel/hda.c

+		 * is applied. For I2S codec + HDMI codec, there is only one
+		 * hda codec. For other cases, the value should be zero.
+		 */
+		if (codec_num > 1) {


The logic is wrong. Is there is a single codec, the skl_hda_generic_machine driver can work
this should be codec_num > 0

yes, one case

plbossart · 2018-11-28T10:11:40Z

sound/soc/sof/sof-acpi-dev.c

 	mach = snd_soc_acpi_find_machine(desc->machines);
 	if (!mach) {
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)
 		/* fallback to nocodec mode */


why are you changing this? This has nothing to do with HDaudio?

yes, but you need to add
#ifndef CONFIG_SND_SOC_SOF_NOCODEC
return error
#endif

If I don't delete it, it should be selected for HDaudio, or pci_probe or acpic_probe will quit with error. And CONFIG_SND_SOC_SOF_NOCODEC has nothing to do with HDaudio, so it is by no means to keep it here.

plbossart · 2018-11-28T10:13:07Z

sound/soc/sof/sof-acpi-dev.c

-		dev_warn(dev, "No matching ASoC machine driver found - falling back to HDA codec\n");
-		mach = snd_soc_acpi_intel_hda_machines;
-		mach->sof_fw_filename = desc->nocodec_fw_filename;
-#endif


that was a bug though, not sure why we had HDaudio codec mentioned in that file.

plbossart · 2018-11-28T10:15:52Z

sound/soc/sof/sof-pci-dev.c

 		goto release_regions;
 #else
 	/* find machine */
 	mach = snd_soc_acpi_find_machine(desc->machines);


I don't get the flow here. For HDaudio this will fail, so how do you not end-up in the error case.
Even if you set the mach variable earlier, it'll be overridden here.
And you are doing this before doing the DSP probe (the platform driver is registered later)

Does this even work?

For HDaudio, nocodec is setup here. in sof_probe, if hd codec is detected , HDaudio machine driver would be applied.

platform driver is registered after HDaudio machine driver is applied.

I test it on WHL, it works.

@plbossart , I will wait for your PR to be merged and do it based on it.

@RanderWang
I don't get what you are saying and 'it works' isn't a good explanation, sorry I don't know what 'it' means.
The ask is that we can have a single kernel with HDaudio compiled in, and the behavior is as follows
a. try to find an ACPI ID and create the relevant machine driver
b. if this fails, check if there are HDAudio codecs. if HDaudio + iDISP are found, use the generic HDaudio machine driver.
c1. if nothing is detected fall back to nocodec.
c2. If only iDISP is found, either fallback to a nocodec+iDISP configuration or to a simple iDISP configuration if nocodec is not selected.

Is this clearer?

@plbossart Please forgive me about my English, I should improve it.
I have a discuss with Mengdong, now I totally understand your idea. We need to detect hda code in pci_probe. I check the driver code and find it is impossible to finish it and validate it today.
(1) sof-pci-dev.c is in our core layer, but hda is in intel layer. Again the same issue for us like link dma setting, a interface is needed for communication.
(2) if hda codec is detected in pci_probe, the initialization and detection code should be deleted in intel layer ?

This is my idea, I am sure is not wrong. I will close my PR

Since commit e1ab9a4 ("i2c: imx: improve the error handling in i2c_imx_dma_request()") when booting with the DMA driver as module (such as CONFIG_FSL_EDMA=m) the following endless clk warnings are seen: [ 153.077831] ------------[ cut here ]------------ [ 153.082528] WARNING: CPU: 0 PID: 15 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 [ 153.093077] i2c0 already disabled [ 153.096416] Modules linked in: [ 153.099521] CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G W 5.2.0+ #321 [ 153.107290] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) [ 153.113772] Workqueue: events deferred_probe_work_func [ 153.118979] [<c0019560>] (unwind_backtrace) from [<c0014734>] (show_stack+0x10/0x14) [ 153.126778] [<c0014734>] (show_stack) from [<c083f8dc>] (dump_stack+0x9c/0xd4) [ 153.134051] [<c083f8dc>] (dump_stack) from [<c0031154>] (__warn+0xf8/0x124) [ 153.141056] [<c0031154>] (__warn) from [<c0031248>] (warn_slowpath_fmt+0x38/0x48) [ 153.148580] [<c0031248>] (warn_slowpath_fmt) from [<c040fde0>] (clk_core_disable_lock+0x18/0x24) [ 153.157413] [<c040fde0>] (clk_core_disable_lock) from [<c058f520>] (i2c_imx_probe+0x554/0x6ec) [ 153.166076] [<c058f520>] (i2c_imx_probe) from [<c04b9178>] (platform_drv_probe+0x48/0x98) [ 153.174297] [<c04b9178>] (platform_drv_probe) from [<c04b7298>] (really_probe+0x1d8/0x2c0) [ 153.182605] [<c04b7298>] (really_probe) from [<c04b7554>] (driver_probe_device+0x5c/0x174) [ 153.190909] [<c04b7554>] (driver_probe_device) from [<c04b58c8>] (bus_for_each_drv+0x44/0x8c) [ 153.199480] [<c04b58c8>] (bus_for_each_drv) from [<c04b746c>] (__device_attach+0xa0/0x108) [ 153.207782] [<c04b746c>] (__device_attach) from [<c04b65a4>] (bus_probe_device+0x88/0x90) [ 153.215999] [<c04b65a4>] (bus_probe_device) from [<c04b6a04>] (deferred_probe_work_func+0x60/0x90) [ 153.225003] [<c04b6a04>] (deferred_probe_work_func) from [<c004f190>] (process_one_work+0x204/0x634) [ 153.234178] [<c004f190>] (process_one_work) from [<c004f618>] (worker_thread+0x20/0x484) [ 153.242315] [<c004f618>] (worker_thread) from [<c0055c2c>] (kthread+0x118/0x150) [ 153.249758] [<c0055c2c>] (kthread) from [<c00090b4>] (ret_from_fork+0x14/0x20) [ 153.257006] Exception stack(0xdde43fb0 to 0xdde43ff8) [ 153.262095] 3fa0: 00000000 00000000 00000000 00000000 [ 153.270306] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 153.278520] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 153.285159] irq event stamp: 3323022 [ 153.288787] hardirqs last enabled at (3323021): [<c0861c4c>] _raw_spin_unlock_irq+0x24/0x2c [ 153.297261] hardirqs last disabled at (3323022): [<c040d7a0>] clk_enable_lock+0x10/0x124 [ 153.305392] softirqs last enabled at (3322092): [<c000a504>] __do_softirq+0x344/0x540 [ 153.313352] softirqs last disabled at (3322081): [<c00385c0>] irq_exit+0x10c/0x128 [ 153.320946] ---[ end trace a506731ccd9bd703 ]--- This endless clk warnings behaviour is well explained by Andrey Smirnov: "Allocating DMA after registering I2C adapter can lead to infinite probing loop, for example, consider the following scenario: 1. i2c_imx_probe() is called and successfully registers an I2C adapter via i2c_add_numbered_adapter() 2. As a part of i2c_add_numbered_adapter() new I2C slave devices are added from DT which results in a call to driver_deferred_probe_trigger() 3. i2c_imx_probe() continues and calls i2c_imx_dma_request() which due to lack of proper DMA driver returns -EPROBE_DEFER 4. i2c_imx_probe() fails, removes I2C adapter and returns -EPROBE_DEFER, which places it into deferred probe list 5. Deferred probe work triggered in #2 above kicks in and calls i2c_imx_probe() again thus bringing us to step #1" So revert commit e1ab9a4 ("i2c: imx: improve the error handling in i2c_imx_dma_request()") and restore the old behaviour, in order to avoid regressions on existing setups. Cc: <[email protected]> Reported-by: Andrey Smirnov <[email protected]> Reported-by: Russell King <[email protected]> Fixes: e1ab9a4 ("i2c: imx: improve the error handling in i2c_imx_dma_request()") Signed-off-by: Fabio Estevam <[email protected]> Signed-off-by: Wolfram Sang <[email protected]>

gtp hashtable size is received by user-space. So, this hashtable size could be too large. If so, kmalloc will internally print a warning message. This warning message is actually not necessary for the gtp module. So, this patch adds __GFP_NOWARN to avoid this message. Splat looks like: [ 2171.200049][ T1860] WARNING: CPU: 1 PID: 1860 at mm/page_alloc.c:4713 __alloc_pages_nodemask+0x2f3/0x740 [ 2171.238885][ T1860] Modules linked in: gtp veth openvswitch nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv] [ 2171.262680][ T1860] CPU: 1 PID: 1860 Comm: gtp-link Not tainted 5.5.0+ #321 [ 2171.263567][ T1860] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 2171.264681][ T1860] RIP: 0010:__alloc_pages_nodemask+0x2f3/0x740 [ 2171.265332][ T1860] Code: 64 fe ff ff 65 48 8b 04 25 c0 0f 02 00 48 05 f0 12 00 00 41 be 01 00 00 00 49 89 47 0 [ 2171.267301][ T1860] RSP: 0018:ffff8880b51af1f0 EFLAGS: 00010246 [ 2171.268320][ T1860] RAX: ffffed1016a35e43 RBX: 0000000000000000 RCX: 0000000000000000 [ 2171.269517][ T1860] RDX: 0000000000000000 RSI: 000000000000000b RDI: 0000000000000000 [ 2171.270305][ T1860] RBP: 0000000000040cc0 R08: ffffed1018893109 R09: dffffc0000000000 [ 2171.275973][ T1860] R10: 0000000000000001 R11: ffffed1018893108 R12: 1ffff11016a35e43 [ 2171.291039][ T1860] R13: 000000000000000b R14: 000000000000000b R15: 00000000000f4240 [ 2171.292328][ T1860] FS: 00007f53cbc83740(0000) GS:ffff8880da000000(0000) knlGS:0000000000000000 [ 2171.293409][ T1860] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 2171.294586][ T1860] CR2: 000055f540014508 CR3: 00000000b49f2004 CR4: 00000000000606e0 [ 2171.295424][ T1860] Call Trace: [ 2171.295756][ T1860] ? mark_held_locks+0xa5/0xe0 [ 2171.296659][ T1860] ? __alloc_pages_slowpath+0x21b0/0x21b0 [ 2171.298283][ T1860] ? gtp_encap_enable_socket+0x13e/0x400 [gtp] [ 2171.298962][ T1860] ? alloc_pages_current+0xc1/0x1a0 [ 2171.299475][ T1860] kmalloc_order+0x22/0x80 [ 2171.299936][ T1860] kmalloc_order_trace+0x1d/0x140 [ 2171.300437][ T1860] __kmalloc+0x302/0x3a0 [ 2171.300896][ T1860] gtp_newlink+0x293/0xba0 [gtp] [ ... ] Fixes: 459aa66 ("gtp: add initial driver for datapath of GPRS Tunneling Protocol (GTP-U)") Signed-off-by: Taehee Yoo <[email protected]> Signed-off-by: David S. Miller <[email protected]>

RanderWang requested review from keyonjie and plbossart November 28, 2018 03:13

ranj063 reviewed Nov 28, 2018

View reviewed changes

RanderWang force-pushed the hda_machine branch from ca5f90b to d1089c1 Compare November 28, 2018 05:03

keyonjie reviewed Nov 28, 2018

View reviewed changes

RanderWang force-pushed the hda_machine branch from d1089c1 to a8d2f64 Compare November 28, 2018 07:37

plbossart requested changes Nov 28, 2018

View reviewed changes

RanderWang closed this Nov 30, 2018

jocelyn-li mentioned this pull request Dec 1, 2018

HDaudio: machine driver selection improvement #284

Closed

RanderWang mentioned this pull request Jan 8, 2019

ASOC: SOF: choose hda machine driver if hda codes detected #508

Merged

ASoC: SOF: refine the selection of machine drivers #321

ASoC: SOF: refine the selection of machine drivers #321

Uh oh!

Conversation

RanderWang commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Nov 28, 2018

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!

RanderWang Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

RanderWang Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

keyonjie Nov 28, 2018 •

edited

Loading

ranj063 Nov 28, 2018 •

edited

Loading

RanderWang Nov 29, 2018 •

edited

Loading

RanderWang Nov 30, 2018 •

edited

Loading