[alsa-devel] regression: bad sound card type detection in audio graph
Hello Kuninori,
We are seeing a regression while rebasing on kernel V5.1 (from v4.19). The regression is linked to your update of the audio graph to merge the support of the normal and DPCM sound cards.
In our sound card (described here: https://github.com/STMicroelectronics/linux/blob/v4.19-stm32mp/arch/arm/boot...)
We declare a normal sound card using the WM8994 codec:
Playback: CPU-DAI (SAI2a)-> CODEC-DAI/port0 Record: CPU-DAI (SAI2B)<- CODEC-DAI/port1 ...
The WM8994 codec is declared with 2 ports as it supports 2 interfaces.
When we probe the sound card we are detecting a wrong dai link type because the condition if (of_get_child_count(codec_port) > 1) return true. The dai link is detected as DPCM instead of the expected normal link.
I checked your last commits sent on mailing list, seems that this criteria is still valid.
Please could you elaborate a little bit on this criteria for the normal/DPCM selection?
What is you feeling about this limitation. Do you see a way to refine the condition to allow normal sound card with multi-port codecs. Or do we have to migrate our sound card to DPCM.
Thanks in advance for your answer
Regards Arnaud
Hi Arnaud
Thank you for checking/testing driver
In our sound card (described here: https://github.com/STMicroelectronics/linux/blob/v4.19-stm32mp/arch/arm/boot...)
We declare a normal sound card using the WM8994 codec:
Playback: CPU-DAI (SAI2a)-> CODEC-DAI/port0 Record: CPU-DAI (SAI2B)<- CODEC-DAI/port1 ...
The WM8994 codec is declared with 2 ports as it supports 2 interfaces.
When we probe the sound card we are detecting a wrong dai link type because the condition if (of_get_child_count(codec_port) > 1) return true. The dai link is detected as DPCM instead of the expected normal link.
It seems difficult to use WM8994 on my test environment. And I tried to add 2 ports on my sound codec driver/DT, but I can't reproduce your issue. But, I want to solve your issue.
In my understanding, in your case, cpu/codec will be below.
for Playback cpu_ep = sai2a_endpoint codec_ep = wm8994_tx_endpoint codec_port = wm8994_tx_port
for Record cpu_ep = sai2b_endpoint codec_ep = wm8994_rx_endpoint codec_port = wm8994_rx_port
Please could you elaborate a little bit on this criteria for the normal/DPCM selection?
What is you feeling about this limitation. Do you see a way to refine the condition to allow normal sound card with multi-port codecs. Or do we have to migrate our sound card to DPCM.
If it was DPCM, 1 port is connected from multiple "endpoint". So, audio-graph-card is checking if it was "multi-endpoint" or not. "multi-port" is not related to this judgment.
And in your case, if my understanding was correct, wm8994_tx/rx only have single-endpoint. so, if (of_get_child_count(codec_port) > 1) return true is a little bit strange for me. Can you check what is this codec_port ? I think you can use printk("%pOF\n", codec_port) for it.
Hi kuninori,
On 3/28/19 1:55 AM, Kuninori Morimoto wrote:
Hi Arnaud
Thank you for checking/testing driver
In our sound card (described here: https://github.com/STMicroelectronics/linux/blob/v4.19-stm32mp/arch/arm/boot...)
We declare a normal sound card using the WM8994 codec:
Playback: CPU-DAI (SAI2a)-> CODEC-DAI/port0 Record: CPU-DAI (SAI2B)<- CODEC-DAI/port1 ...
The WM8994 codec is declared with 2 ports as it supports 2 interfaces.
When we probe the sound card we are detecting a wrong dai link type because the condition if (of_get_child_count(codec_port) > 1) return true. The dai link is detected as DPCM instead of the expected normal link.
It seems difficult to use WM8994 on my test environment. And I tried to add 2 ports on my sound codec driver/DT, but I can't reproduce your issue. But, I want to solve your issue.
In my understanding, in your case, cpu/codec will be below.
for Playback cpu_ep = sai2a_endpoint codec_ep = wm8994_tx_endpoint codec_port = wm8994_tx_port
for Record cpu_ep = sai2b_endpoint codec_ep = wm8994_rx_endpoint codec_port = wm8994_rx_port
Please could you elaborate a little bit on this criteria for the normal/DPCM selection?
What is you feeling about this limitation. Do you see a way to refine the condition to allow normal sound card with multi-port codecs. Or do we have to migrate our sound card to DPCM.
If it was DPCM, 1 port is connected from multiple "endpoint". So, audio-graph-card is checking if it was "multi-endpoint" or not. "multi-port" is not related to this judgment.
And in your case, if my understanding was correct, wm8994_tx/rx only have single-endpoint. so, if (of_get_child_count(codec_port) > 1) return true is a little bit strange for me. Can you check what is this codec_port ? I think you can use printk("%pOF\n", codec_port) for it.
Thanks for your answer and my apologize. I found the issue: it was located between my keyboard and my seat...
I merged a bad branch, which contains following declaration for the the wolfson node:
ports { #address-cells = <1>; #size-cells = <0>; wm8994_tx_endpoint: endpoint@0 { reg = <0>; remote-endpoint = <&sai2a_endpoint>; }; wm8994_rx_endpoint: endpoint@1 { reg = <1>; remote-endpoint = <&sai2b_endpoint>; }; };
This was probing before the audio graph update, not now, but this is not a valid way to declare it.
With the configuration described in the github, it is perfectly working.
This trig me an extra question: how to manage a configuration using only one codec DAI (bidir) connected to 2 unidirectional CPU-DAI? Playback: CPU-DAI (slave)-> (master)CODEC-DAI/port0 Record: CPU-DAI (slave)<- (master)CODEC-DAI/port0
In this case it seems logic to declare for the codec, one port with 2 endpoints so this imply to use DPCM mechanism... right? In this case we should use a static routing to conncet FE and BE...? Do you know if this kind of configuration has been tested?
FYi, This configuration is available on the stm32mp157c-dk2 board. Today we have not an upstreamable solution (we have hacked the codec to duplicate the DAI to create 2 ports with one endpoint, instead of one port with 2 endpoints).
Thanks,
Arnaud
Hi Arnaud
Thanks for your answer and my apologize. I found the issue: it was located between my keyboard and my seat...
OK, no problem.
ports { #address-cells = <1>; #size-cells = <0>; wm8994_tx_endpoint: endpoint@0 { reg = <0>; remote-endpoint = <&sai2a_endpoint>; }; wm8994_rx_endpoint: endpoint@1 { reg = <1>; remote-endpoint = <&sai2b_endpoint>; }; };
Not a big deal, but I think you want to use "port" not "ports" here.
This trig me an extra question: how to manage a configuration using only one codec DAI (bidir) connected to 2 unidirectional CPU-DAI? Playback: CPU-DAI (slave)-> (master)CODEC-DAI/port0 Record: CPU-DAI (slave)<- (master)CODEC-DAI/port0
A little bit confused.
// 1 CPU, 1 Codec Playback: CPU-DAI(slave) -> (master)CODEC-DAI/port0 Record: CPU-DAI(slave) <- (master)CODEC-DAI/port0 or // 2 CPU, 1 Codec Playback: CPU-A-DAI(slave) -> (master)CODEC-DAI/port0 Record: CPU-B-DAI(slave) <- (master)CODEC-DAI/port0
1st one is very normal at ALSA SoC I think. 1 endpoint is 1 DAI, and it has 1 playback and 1 capture. 2nd one is indeed special case.
In this case it seems logic to declare for the codec, one port with 2 endpoints so this imply to use DPCM mechanism... right?
So far, yes
In this case we should use a static routing to conncet FE and BE...? Do you know if this kind of configuration has been tested?
I assume it is above "2nd" case, unfortunately, this case is not assumed on audio-graph-card so far.
FYi, This configuration is available on the stm32mp157c-dk2 board. Today we have not an upstreamable solution (we have hacked the codec to duplicate the DAI to create 2 ports with one endpoint, instead of one port with 2 endpoints).
I'm not sure, but if we can judge "it has multi endpoints, but it is not a DPCM connection" somehow, we can solve your issue ? The issue here is this judgment should be DT acceptable.
Hmm... Easy solution I noticed now is that compatible = "audio-graph-card" case should only select non DPCM case, same as before. compatible = "audio-graph-scu-card" can be both "non DPCM" and "DPCM". I think only user of "audio-graph-scu-card" is me, and it is local use. I think it is still DT backward compatible (?) I didn't test this patch, but maybe similar can be solution ?
------------------ diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index b75b0f1..0b39d85 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -402,6 +402,7 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, struct device_node *codec_port_old = NULL; struct asoc_simple_data adata; int rc, ret; + int dpcm_selectable = (int)of_device_get_match_data(dev);
/* loop for all listed CPU port */ of_for_each_phandle(&it, rc, node, "dais", NULL, 0) { @@ -431,7 +432,8 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, * if Codec port has many endpoints, * or has convert-xxx property */ - if ((of_get_child_count(codec_port) > 1) || + if ((dpcm_selectable && + of_get_child_count(codec_port) > 1) || adata.convert_rate || adata.convert_channels) ret = func_dpcm(priv, cpu_ep, codec_ep, li, (codec_port_old == codec_port)); @@ -664,8 +666,8 @@ static int graph_remove(struct platform_device *pdev) }
static const struct of_device_id graph_of_match[] = { - { .compatible = "audio-graph-card", }, - { .compatible = "audio-graph-scu-card", }, + { .compatible = "audio-graph-card", .data = 0 }, + { .compatible = "audio-graph-scu-card", .data = 1 }, {}, }; MODULE_DEVICE_TABLE(of, graph_of_match); ------------------
Best regards --- Kuninori Morimoto
On 3/29/19 3:03 AM, Kuninori Morimoto wrote:
Hi Arnaud
Thanks for your answer and my apologize. I found the issue: it was located between my keyboard and my seat...
OK, no problem.
ports { #address-cells = <1>; #size-cells = <0>; wm8994_tx_endpoint: endpoint@0 { reg = <0>; remote-endpoint = <&sai2a_endpoint>; }; wm8994_rx_endpoint: endpoint@1 { reg = <1>; remote-endpoint = <&sai2b_endpoint>; }; };
Not a big deal, but I think you want to use "port" not "ports" here.
This trig me an extra question: how to manage a configuration using only one codec DAI (bidir) connected to 2 unidirectional CPU-DAI? Playback: CPU-DAI (slave)-> (master)CODEC-DAI/port0 Record: CPU-DAI (slave)<- (master)CODEC-DAI/port0
A little bit confused.
// 1 CPU, 1 Codec Playback: CPU-DAI(slave) -> (master)CODEC-DAI/port0 Record: CPU-DAI(slave) <- (master)CODEC-DAI/port0 or // 2 CPU, 1 Codec Playback: CPU-A-DAI(slave) -> (master)CODEC-DAI/port0 Record: CPU-B-DAI(slave) <- (master)CODEC-DAI/port0
1st one is very normal at ALSA SoC I think. 1 endpoint is 1 DAI, and it has 1 playback and 1 capture. 2nd one is indeed special case.
In this case it seems logic to declare for the codec, one port with 2 endpoints so this imply to use DPCM mechanism... right?
So far, yes
In this case we should use a static routing to conncet FE and BE...? Do you know if this kind of configuration has been tested?
I assume it is above "2nd" case, unfortunately, this case is not assumed on audio-graph-card so far.
Yes here i was speaking about the second case. This confirms that we need to look deeper in code on our side to find and propose a solution for this type of links.
FYi, This configuration is available on the stm32mp157c-dk2 board. Today we have not an upstreamable solution (we have hacked the codec to duplicate the DAI to create 2 ports with one endpoint, instead of one port with 2 endpoints).
I'm not sure, but if we can judge "it has multi endpoints, but it is not a DPCM connection" somehow, we can solve your issue ? The issue here is this judgment should be DT acceptable.> Hmm... Easy solution I noticed now is that compatible = "audio-graph-card" case should only select non DPCM case, same as before. compatible = "audio-graph-scu-card" can be both "non DPCM" and "DPCM". I think only user of "audio-graph-scu-card" is me, and it is local use. I think it is still DT backward compatible (?) I didn't test this patch, but maybe similar can be solution ?
Yes i think that is a good proposal with advantage of ensuring compatibility with existing code.
Thanks for your clarifications!
Best regards, Arnaud
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index b75b0f1..0b39d85 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -402,6 +402,7 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, struct device_node *codec_port_old = NULL; struct asoc_simple_data adata; int rc, ret; + int dpcm_selectable = (int)of_device_get_match_data(dev); /* loop for all listed CPU port */ of_for_each_phandle(&it, rc, node, "dais", NULL, 0) { @@ -431,7 +432,8 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, * if Codec port has many endpoints, * or has convert-xxx property */ - if ((of_get_child_count(codec_port) > 1) || + if ((dpcm_selectable && + of_get_child_count(codec_port) > 1) || adata.convert_rate || adata.convert_channels) ret = func_dpcm(priv, cpu_ep, codec_ep, li, (codec_port_old == codec_port)); @@ -664,8 +666,8 @@ static int graph_remove(struct platform_device *pdev) } static const struct of_device_id graph_of_match[] = { - { .compatible = "audio-graph-card", }, - { .compatible = "audio-graph-scu-card", }, + { .compatible = "audio-graph-card", .data = 0 }, + { .compatible = "audio-graph-scu-card", .data = 1 }, {}, }; MODULE_DEVICE_TABLE(of, graph_of_match);
Best regards
Kuninori Morimoto
Hi Arnaud
Easy solution I noticed now is that compatible = "audio-graph-card" case should only select non DPCM case, same as before. compatible = "audio-graph-scu-card" can be both "non DPCM" and "DPCM". I think only user of "audio-graph-scu-card" is me, and it is local use. I think it is still DT backward compatible (?) I didn't test this patch, but maybe similar can be solution ?
Yes i think that is a good proposal with advantage of ensuring compatibility with existing code.
Thanks for your clarifications!
Good ! Can you test and post patch ? I think my proposed patch is not good enough for you (?). I want to test your tested patch on my side.
Best regards --- Kuninori Morimoto
Hi Kuninori,
On 4/1/19 2:26 AM, Kuninori Morimoto wrote:
Hi Arnaud
Easy solution I noticed now is that compatible = "audio-graph-card" case should only select non DPCM case, same as before. compatible = "audio-graph-scu-card" can be both "non DPCM" and "DPCM". I think only user of "audio-graph-scu-card" is me, and it is local use. I think it is still DT backward compatible (?) I didn't test this patch, but maybe similar can be solution ?
Yes i think that is a good proposal with advantage of ensuring compatibility with existing code.
Thanks for your clarifications!
Good ! Can you test and post patch ? I think my proposed patch is not good enough for you (?). I want to test your tested patch on my side.
I tested the patch you proposed it works perfectly. For your information , we have posted in parallel a patch to support multi endpoint for the cs42l51 codec. https://www.spinics.net/lists/alsa-devel/msg88655.html This patch as been accepted by Marc and should be part on the next pull request. Associated DT (need to be upstreamed) is visible here:https://github.com/STMicroelectronics/linux/blob/196201973b7048ccf75aa63ac3c...
For sure I can propose the patch in audio graph but it is your idea, and i have no major update/fix on it. So if you prefer you can post it and i will ack it.
@ -20,10 +20,12 @@ #include <linux/string.h> #include <sound/simple_card_utils.h>
#define PREFIX "audio-graph-card,"
+#define DPCM_SELECTABLE 1 + static int graph_outdrv_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { struct snd_soc_dapm_context *dapm = w->dapm; @@ -414,10 +416,12 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, struct device_node *codec_ep; struct device_node *codec_port; struct device_node *codec_port_old = NULL; struct asoc_simple_data adata; int rc, ret; + unsigned int dpcm_selectable = + (unsigned int)of_device_get_match_data(dev);
/* loop for all listed CPU port */ of_for_each_phandle(&it, rc, node, "dais", NULL, 0) { cpu_port = it.node; cpu_ep = NULL; @@ -443,12 +447,13 @@ static int graph_for_each_link(struct asoc_simple_priv *priv, /* * It is DPCM * if Codec port has many endpoints, * or has convert-xxx property */ - if ((of_get_child_count(codec_port) > 1) || - adata.convert_rate || adata.convert_channels) + if (dpcm_selectable && + ((of_get_child_count(codec_port) > 1) || + adata.convert_rate || adata.convert_channels)) ret = func_dpcm(priv, cpu_ep, codec_ep, li, (codec_port_old == codec_port)); /* else normal sound */ else ret = func_noml(priv, cpu_ep, codec_ep, li); @@ -677,11 +682,13 @@ static int graph_remove(struct platform_device *pdev) return asoc_simple_clean_reference(card); }
static const struct of_device_id graph_of_match[] = { { .compatible = "audio-graph-card", }, - { .compatible = "audio-graph-scu-card", }, + { .compatible = "audio-graph-scu-card", + .data = (void *)DPCM_SELECTABLE + }, {}, }; MODULE_DEVICE_TABLE(of, graph_of_match);
static struct platform_driver graph_card = {
Thanks, Arnaud
Best regards
Kuninori Morimoto
Hi Arnaud
I tested the patch you proposed it works perfectly. For your information , we have posted in parallel a patch to support multi endpoint for the cs42l51 codec. https://www.spinics.net/lists/alsa-devel/msg88655.html
Yeah, I have noticed about it. I'm happy because it is new use-case of of_xlate_dai_id :)
For sure I can propose the patch in audio graph but it is your idea, and i have no major update/fix on it. So if you prefer you can post it and i will ack it.
OK, thanks !! I will post patch for both simple-card / audio-graph
Best regards --- Kuninori Morimoto
participants (2)
-
Arnaud Pouliquen -
Kuninori Morimoto