On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote:
On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:
I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.
I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.
Right, so, I've looked at this again today, and I've sort-of got it working.
It would appear that the problem is that the AIF widgets are not finding their stream - the implicit routes are not being created. It _appears_ that the strean name in widgets are only ever connected to the streams within the same DAPM context.
Hence, to get this stuff right, you _have_ to know about the DAPM internals, and you _have_ to know that it's more than just "a graph walk". Documentation would help there...
Anyway, with that fixed, the widgets get powered up - great, finally some forward progress. Not quite, because it still doesn't work right. ASoC still ends up filling my disk with lots of:
S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1
messages. Throwing a WARN_ON(1) just after that message reveals:
[<c032d334>] (dpcm_fe_dai_prepare+0xe0/0xf0) from [<c030d324>] (snd_pcm_do_prepare+0x14/0x2c) [<c030d324>] (snd_pcm_do_prepare+0x14/0x2c) from [<c030cedc>] (snd_pcm_action_single+0x38/0x78) [<c030cedc>] (snd_pcm_action_single+0x38/0x78) from [<c030e8e0>] (snd_pcm_action_nonatomic+0x60/0x78) [<c030e8e0>] (snd_pcm_action_nonatomic+0x60/0x78) from [<c0311040>] (snd_pcm_common_ioctl1+0x2b4/0x5bc) [<c0311040>] (snd_pcm_common_ioctl1+0x2b4/0x5bc) from [<c0311398>] (snd_pcm_capture_ioctl1+0x50/0x308) [<c0311398>] (snd_pcm_capture_ioctl1+0x50/0x308) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60) [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48)
So, it's because the capture isn't wired up. The capture isn't wired up on this platform, so how do I tell ASoC not to bother with the capture side with DPCM? Creating a link for the capture side to the dummy codec is wrong, because that allows capture to be brought up when in fact there is no capture wired up on the board (and means that we'll end up trying to use unconnected inputs.)
I think the easiest "solution" to that is to just comment out the damned warning in the core code for the moment, until a proper solution can be thought up.
There is also this which seems to be a core ALSA problem - I get two of these on every boot:
WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0x128() proc_dir_entry 'asound/oss' already registered Modules linked in: snd_soc_spdif_tx m25p80 mtd orion_wdt snd_soc_kirkwood snd_soc_kirkwood_spdif CPU: 0 PID: 388 Comm: kworker/u2:2 Not tainted 3.10.0+ #649 Workqueue: deferwq deferred_probe_work_func [<c0013d7c>] (unwind_backtrace+0x0/0xf8) from [<c0011bec>] (show_stack+0x10/0x14) [<c0011bec>] (show_stack+0x10/0x14) from [<c0048b80>] (warn_slowpath_common+0x4c/0x68) [<c0048b80>] (warn_slowpath_common+0x4c/0x68) from [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) from [<c0124804>] (proc_register+0xac/0x128) [<c0124804>] (proc_register+0xac/0x128) from [<c0124910>] (proc_create_data+0x90/0xac) [<c0124910>] (proc_create_data+0x90/0xac) from [<c0302b48>] (snd_info_register+0x54/0xf0) [<c0302b48>] (snd_info_register+0x54/0xf0) from [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) from [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) from [<c03069c8>] (snd_device_register_all+0x40/0x80) [<c03069c8>] (snd_device_register_all+0x40/0x80) from [<c030192c>] (snd_card_register+0x24/0x228) [<c030192c>] (snd_card_register+0x24/0x228) from [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) from [<c03255c4>] (snd_soc_register_card+0x26c/0x324) [<c03255c4>] (snd_soc_register_card+0x26c/0x324) from [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) from [<c0259468>] (platform_drv_probe+0x18/0x1c) [<c0259468>] (platform_drv_probe+0x18/0x1c) from [<c0258144>] (really_probe+0x74/0x1f4) [<c0258144>] (really_probe+0x74/0x1f4) from [<c02583d8>] (driver_probe_device+0x30/0x48) [<c02583d8>] (driver_probe_device+0x30/0x48) from [<c0256a48>] (bus_for_each_drv+0x60/0x8c) [<c0256a48>] (bus_for_each_drv+0x60/0x8c) from [<c0258368>] (device_attach+0x80/0xa4) [<c0258368>] (device_attach+0x80/0xa4) from [<c02577a8>] (bus_probe_device+0x88/0xac) [<c02577a8>] (bus_probe_device+0x88/0xac) from [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) from [<c0060a74>] (process_one_work+0x190/0x3f4) [<c0060a74>] (process_one_work+0x190/0x3f4) from [<c0062544>] (worker_thread+0xf4/0x318) [<c0062544>] (worker_thread+0xf4/0x318) from [<c006800c>] (kthread+0xa8/0xb4) [<c006800c>] (kthread+0xa8/0xb4) from [<c000e4a8>] (ret_from_fork+0x14/0x2c)
I suspect if I put another codec into the mix, I'll get four of them (one pair for each backend stream.)
And /proc/asound ends up looking like this:
total 0 lrwxrwxrwx 1 root root 5 Aug 10 16:14 SPDIF -> card0 dr-xr-xr-x 4 root root 0 Aug 10 16:14 card0 -r--r--r-- 1 root root 0 Aug 10 16:14 cards -r--r--r-- 1 root root 0 Aug 10 16:14 devices -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -r--r--r-- 1 root root 0 Aug 10 16:14 pcm -r--r--r-- 1 root root 0 Aug 10 16:14 timers -r--r--r-- 1 root root 0 Aug 10 16:14 version
Yes, three 'oss'es, and /proc/asound/pcm looks like this:
00-00: IEC958 Playback (*) : : playback 1 : capture 1 00-01: ((null)) : : playback 1 : capture 1
which doesn't look very healthy. That ((null)) seems to be down to the lack of a .stream_name for the backend DAI link - but then Liam doesn't have that either. Giving it a stream name fixes the /proc/asound/pcm output:
00-00: Audio Playback (*) : : playback 1 : capture 1 00-01: (IEC958 Playback) : : playback 1 : capture 1
but doesn't stop those warnings (not really surprising, because it isn't the card level which is being complained about). Probably the easiest way to stop that appearing is to just disable OSS compatibility.
That's something which should be documented until it is fixed - that DPCM is currently incompatible with OSS.
So, there are two issues which still need resolving: 1. The registration of Codec widgets from the platform introduced by Liam in be09ad90e17b79fdb0d513a31e814ff4d42e3dff ASoC: core: Add platform DAI widget mapping
2. How to deal with disconnected front end streams which have no backend provided by their connected codec (in this case, front end can do capture and playback, back end can only do playback.)
Both I think are for Liam to solve.
The last issue which doesn't block this provided it is documented is: 3. The core ALSA developers need to comment on the multiple /proc/asound/oss creation problem.
Here's the hacky patch against my patch set which gets DPCM "working":
diff --git a/sound/soc/codecs/spdif_transciever.c b/sound/soc/codecs/spdif_transciever.c index 553708d..fde0150 100644 --- a/sound/soc/codecs/spdif_transciever.c +++ b/sound/soc/codecs/spdif_transciever.c @@ -36,7 +36,7 @@ static const struct snd_soc_dapm_widget dit_widgets[] = { };
static const const struct snd_soc_dapm_route dit_routes[] = { - { "spdif-out", NULL, "Playback" }, + { "spdif-out", NULL, "spdif-Playback" }, };
static struct snd_soc_codec_driver soc_codec_spdif_dit = { @@ -49,7 +49,7 @@ static struct snd_soc_codec_driver soc_codec_spdif_dit = { static struct snd_soc_dai_driver dit_stub_dai = { .name = "dit-hifi", .playback = { - .stream_name = "Playback", + .stream_name = "spdif-Playback", .channels_min = 1, .channels_max = 384, .rates = STUB_RATES, diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 05d977a..66fc76c 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -500,8 +500,11 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd, static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the device + */ + struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
if (SND_SOC_DAPM_EVENT_ON(event)) priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN; @@ -514,8 +517,11 @@ static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the device + */ + struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
if (SND_SOC_DAPM_EVENT_ON(event)) priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN; @@ -553,8 +559,7 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai) }
/* It appears that these can't be attached to the CPU DAI */ - snd_soc_dapm_new_controls(&dai->platform->dapm, - widgets, ARRAY_SIZE(widgets)); + snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets));
/* put system in a "safe" state : */ /* disable audio interrupts */ diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c index e5257ec..d1ee8f7 100644 --- a/sound/soc/kirkwood/kirkwood-spdif.c +++ b/sound/soc/kirkwood/kirkwood-spdif.c @@ -57,7 +57,7 @@ static struct snd_soc_ops kirkwood_spdif_ops = { };
static const struct snd_soc_dapm_route routes[] = { - { "Playback", NULL, "spdifdo" }, + { "spdif-Playback", NULL, "spdifdo" }, };
static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { @@ -75,9 +75,18 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { { .name = "S/PDIF1", - .stream_name = "IEC958 Playback", + .stream_name = "Audio Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "mvebu-audio.1", + .codec_name = "snd-soc-dummy", + .codec_dai_name = "snd-soc-dummy-dai", + .dynamic = 1, + }, { + .name = "Codec", + .stream_name = "IEC958 Playback", + .cpu_dai_name = "snd-soc-dummy-dai", + .platform_name = "snd-soc-dummy", + .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, @@ -108,7 +117,7 @@ static int kirkwood_spdif_probe(struct platform_device *pdev) card->dai_link = kirkwood_spdif_dai0; else card->dai_link = kirkwood_spdif_dai1; - card->num_links = 1; + card->num_links = 2; card->dapm_routes = routes; card->num_dapm_routes = ARRAY_SIZE(routes); card->dev = &pdev->dev; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 48e883e..9176815 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1158,6 +1158,7 @@ static int soc_probe_platform(struct snd_soc_card *card, snd_soc_dapm_new_controls(&platform->dapm, driver->dapm_widgets, driver->num_dapm_widgets);
+#if 0 /* This breaks DAPM on Kirkwood */ /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) @@ -1165,6 +1166,7 @@ static int soc_probe_platform(struct snd_soc_card *card,
snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } +#endif
platform->dapm.idle_bias_off = 1;
@@ -1362,7 +1364,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) return -ENODEV;
list_add(&cpu_dai->dapm.list, &card->dapm_list); -// snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); + snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); }
if (cpu_dai->driver->probe) { diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ccb6be4..381df28 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1634,8 +1634,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
/* there is no point preparing this FE if there are no BEs */ if (list_empty(&fe->dpcm[stream].be_clients)) { - dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n", - fe->dai_link->name); +// dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n", +// fe->dai_link->name); ret = -EINVAL; goto out; }