regression: bad sound card type detection in audio graph

regression: bad sound card type detection in audio graph

[Arnaud Pouliquen]

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/dts/stm32mp157c-ev1.dts)

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

Re: regression: bad sound card type detection in audio graph

[Kuninori Morimoto]

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/dts/stm32mp157c-ev1.dts)
> 
> 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.

Re: regression: bad sound card type detection in audio graph

[Arnaud Pouliquen]

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/dts/stm32mp157c-ev1.dts)
>> 
>> 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





_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: regression: bad sound card type detection in audio graph

[Kuninori Morimoto]

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

Re: regression: bad sound card type detection in audio graph

[Arnaud Pouliquen]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: regression: bad sound card type detection in audio graph

[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

Re: regression: bad sound card type detection in audio graph

[Arnaud Pouliquen]

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/196201973b7048ccf75aa63ac3c3673f8b6ee1c1/arch/arm/boot/dts/stm32mp157a-dk1.dts#L212

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

Re: regression: bad sound card type detection in audio graph

[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