linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
@ 2020-03-11 18:04 Srinivas Kandagatla
  2020-03-11 18:04 ` [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree Srinivas Kandagatla
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-03-11 18:04 UTC (permalink / raw)
  To: broonie; +Cc: alsa-devel, lgirdwood, perex, linux-kernel, Srinivas Kandagatla

QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
however the default routing do not honour this DT configuration making sound
card fail to probe. FE dais are also not fully honouring device tree configuration.
Fix both of them.

Originally  issue was reported by Vinod Koul

Srinivas Kandagatla (2):
  ASoC: qdsp6: q6asm-dai: only enable dais from device tree
  ASoC: qdsp6: q6routing: remove default routing

 sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
 sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
 2 files changed, 23 insertions(+), 26 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree
  2020-03-11 18:04 [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Srinivas Kandagatla
@ 2020-03-11 18:04 ` Srinivas Kandagatla
  2020-03-12 13:13   ` Applied "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" to the asoc tree Mark Brown
  2020-03-11 18:04 ` [PATCH 2/2] ASoC: qdsp6: q6routing: remove default routing Srinivas Kandagatla
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-03-11 18:04 UTC (permalink / raw)
  To: broonie; +Cc: alsa-devel, lgirdwood, perex, linux-kernel, Srinivas Kandagatla

Existing code enables all the playback and capture dais even
if there is no device tree entry. This can lead to
un-necessary dais in the system which will never be used.
So honour whats specfied in device tree.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index c0d422d0ab94..8b48815ff918 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -69,6 +69,8 @@ struct q6asm_dai_rtd {
 };
 
 struct q6asm_dai_data {
+	struct snd_soc_dai_driver *dais;
+	int num_dais;
 	long long int sid;
 };
 
@@ -889,7 +891,7 @@ static const struct snd_soc_component_driver q6asm_fe_dai_component = {
 	.compr_ops	= &q6asm_dai_compr_ops,
 };
 
-static struct snd_soc_dai_driver q6asm_fe_dais[] = {
+static struct snd_soc_dai_driver q6asm_fe_dais_template[] = {
 	Q6ASM_FEDAI_DRIVER(1),
 	Q6ASM_FEDAI_DRIVER(2),
 	Q6ASM_FEDAI_DRIVER(3),
@@ -903,10 +905,22 @@ static struct snd_soc_dai_driver q6asm_fe_dais[] = {
 static int of_q6asm_parse_dai_data(struct device *dev,
 				    struct q6asm_dai_data *pdata)
 {
-	static struct snd_soc_dai_driver *dai_drv;
+	struct snd_soc_dai_driver *dai_drv;
 	struct snd_soc_pcm_stream empty_stream;
 	struct device_node *node;
-	int ret, id, dir;
+	int ret, id, dir, idx = 0;
+
+
+	pdata->num_dais = of_get_child_count(dev->of_node);
+	if (!pdata->num_dais) {
+		dev_err(dev, "No dais found in DT\n");
+		return -EINVAL;
+	}
+
+	pdata->dais = devm_kcalloc(dev, pdata->num_dais, sizeof(*dai_drv),
+				   GFP_KERNEL);
+	if (!pdata->dais)
+		return -ENOMEM;
 
 	memset(&empty_stream, 0, sizeof(empty_stream));
 
@@ -917,7 +931,8 @@ static int of_q6asm_parse_dai_data(struct device *dev,
 			continue;
 		}
 
-		dai_drv = &q6asm_fe_dais[id];
+		dai_drv = &pdata->dais[idx++];
+		*dai_drv = q6asm_fe_dais_template[id];
 
 		ret = of_property_read_u32(node, "direction", &dir);
 		if (ret)
@@ -955,11 +970,12 @@ static int q6asm_dai_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, pdata);
 
-	of_q6asm_parse_dai_data(dev, pdata);
+	rc = of_q6asm_parse_dai_data(dev, pdata);
+	if (rc)
+		return rc;
 
 	return devm_snd_soc_register_component(dev, &q6asm_fe_dai_component,
-					q6asm_fe_dais,
-					ARRAY_SIZE(q6asm_fe_dais));
+					       pdata->dais, pdata->num_dais);
 }
 
 static const struct of_device_id q6asm_dai_device_id[] = {
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] ASoC: qdsp6: q6routing: remove default routing
  2020-03-11 18:04 [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Srinivas Kandagatla
  2020-03-11 18:04 ` [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree Srinivas Kandagatla
@ 2020-03-11 18:04 ` Srinivas Kandagatla
  2020-03-12 13:13   ` Applied "ASoC: qdsp6: q6routing: remove default routing" to the asoc tree Mark Brown
  2020-03-12 10:41 ` [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Vinod Koul
  2020-04-17 11:24 ` Stephan Gerhold
  3 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-03-11 18:04 UTC (permalink / raw)
  To: broonie
  Cc: alsa-devel, lgirdwood, perex, linux-kernel, Srinivas Kandagatla,
	Vinod Koul

Frontend dais can be configured to rx or tx or both, however having default
routes without considering this configuration can lead to failures during
card probe as below for compress rx only case. These routing have to come
from sound card routing table in device tree.

"routing: ASoC: Failed to add route MM_UL1 -> direct -> MultiMedia1 Capture
msm-snd-sdm845 sound: ASoC: failed to instantiate card -19
"

Reported-by: Vinod Koul <vinod.koul@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c
index 20724102e85a..4d5915b9a06d 100644
--- a/sound/soc/qcom/qdsp6/q6routing.c
+++ b/sound/soc/qcom/qdsp6/q6routing.c
@@ -918,25 +918,6 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"MM_UL6", NULL, "MultiMedia6 Mixer"},
 	{"MM_UL7", NULL, "MultiMedia7 Mixer"},
 	{"MM_UL8", NULL, "MultiMedia8 Mixer"},
-
-	{"MM_DL1",  NULL, "MultiMedia1 Playback" },
-	{"MM_DL2",  NULL, "MultiMedia2 Playback" },
-	{"MM_DL3",  NULL, "MultiMedia3 Playback" },
-	{"MM_DL4",  NULL, "MultiMedia4 Playback" },
-	{"MM_DL5",  NULL, "MultiMedia5 Playback" },
-	{"MM_DL6",  NULL, "MultiMedia6 Playback" },
-	{"MM_DL7",  NULL, "MultiMedia7 Playback" },
-	{"MM_DL8",  NULL, "MultiMedia8 Playback" },
-
-	{"MultiMedia1 Capture", NULL, "MM_UL1"},
-	{"MultiMedia2 Capture", NULL, "MM_UL2"},
-	{"MultiMedia3 Capture", NULL, "MM_UL3"},
-	{"MultiMedia4 Capture", NULL, "MM_UL4"},
-	{"MultiMedia5 Capture", NULL, "MM_UL5"},
-	{"MultiMedia6 Capture", NULL, "MM_UL6"},
-	{"MultiMedia7 Capture", NULL, "MM_UL7"},
-	{"MultiMedia8 Capture", NULL, "MM_UL8"},
-
 };
 
 static int routing_hw_params(struct snd_soc_component *component,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-03-11 18:04 [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Srinivas Kandagatla
  2020-03-11 18:04 ` [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree Srinivas Kandagatla
  2020-03-11 18:04 ` [PATCH 2/2] ASoC: qdsp6: q6routing: remove default routing Srinivas Kandagatla
@ 2020-03-12 10:41 ` Vinod Koul
  2020-04-17 11:24 ` Stephan Gerhold
  3 siblings, 0 replies; 12+ messages in thread
From: Vinod Koul @ 2020-03-12 10:41 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood

On 11-03-20, 18:04, Srinivas Kandagatla wrote:
> QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
> however the default routing do not honour this DT configuration making sound
> card fail to probe. FE dais are also not fully honouring device tree configuration.
> Fix both of them.
> 
> Originally  issue was reported by Vinod Koul

Thanks Srini for the these :) I have tested on DB845c. And they work
just fine after adding the DTS bits.

Tested-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Applied "ASoC: qdsp6: q6routing: remove default routing" to the asoc tree
  2020-03-11 18:04 ` [PATCH 2/2] ASoC: qdsp6: q6routing: remove default routing Srinivas Kandagatla
@ 2020-03-12 13:13   ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2020-03-12 13:13 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, broonie, lgirdwood, linux-kernel, Mark Brown, Vinod Koul

The patch

   ASoC: qdsp6: q6routing: remove default routing

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From f864edff110d3e6f8995636a9a604f6b98eaa308 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Wed, 11 Mar 2020 18:04:22 +0000
Subject: [PATCH] ASoC: qdsp6: q6routing: remove default routing

Frontend dais can be configured to rx or tx or both, however having default
routes without considering this configuration can lead to failures during
card probe as below for compress rx only case. These routing have to come
from sound card routing table in device tree.

"routing: ASoC: Failed to add route MM_UL1 -> direct -> MultiMedia1 Capture
msm-snd-sdm845 sound: ASoC: failed to instantiate card -19
"

Reported-by: Vinod Koul <vinod.koul@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Link: https://lore.kernel.org/r/20200311180422.28363-3-srinivas.kandagatla@linaro.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c
index 20724102e85a..4d5915b9a06d 100644
--- a/sound/soc/qcom/qdsp6/q6routing.c
+++ b/sound/soc/qcom/qdsp6/q6routing.c
@@ -918,25 +918,6 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"MM_UL6", NULL, "MultiMedia6 Mixer"},
 	{"MM_UL7", NULL, "MultiMedia7 Mixer"},
 	{"MM_UL8", NULL, "MultiMedia8 Mixer"},
-
-	{"MM_DL1",  NULL, "MultiMedia1 Playback" },
-	{"MM_DL2",  NULL, "MultiMedia2 Playback" },
-	{"MM_DL3",  NULL, "MultiMedia3 Playback" },
-	{"MM_DL4",  NULL, "MultiMedia4 Playback" },
-	{"MM_DL5",  NULL, "MultiMedia5 Playback" },
-	{"MM_DL6",  NULL, "MultiMedia6 Playback" },
-	{"MM_DL7",  NULL, "MultiMedia7 Playback" },
-	{"MM_DL8",  NULL, "MultiMedia8 Playback" },
-
-	{"MultiMedia1 Capture", NULL, "MM_UL1"},
-	{"MultiMedia2 Capture", NULL, "MM_UL2"},
-	{"MultiMedia3 Capture", NULL, "MM_UL3"},
-	{"MultiMedia4 Capture", NULL, "MM_UL4"},
-	{"MultiMedia5 Capture", NULL, "MM_UL5"},
-	{"MultiMedia6 Capture", NULL, "MM_UL6"},
-	{"MultiMedia7 Capture", NULL, "MM_UL7"},
-	{"MultiMedia8 Capture", NULL, "MM_UL8"},
-
 };
 
 static int routing_hw_params(struct snd_soc_component *component,
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Applied "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" to the asoc tree
  2020-03-11 18:04 ` [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree Srinivas Kandagatla
@ 2020-03-12 13:13   ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2020-03-12 13:13 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, broonie, lgirdwood, linux-kernel, Mark Brown

The patch

   ASoC: qdsp6: q6asm-dai: only enable dais from device tree

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 9b60441692d94effcd37a141035c6106a91ddf8c Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Wed, 11 Mar 2020 18:04:21 +0000
Subject: [PATCH] ASoC: qdsp6: q6asm-dai: only enable dais from device tree

Existing code enables all the playback and capture dais even
if there is no device tree entry. This can lead to
un-necessary dais in the system which will never be used.
So honour whats specfied in device tree.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Link: https://lore.kernel.org/r/20200311180422.28363-2-srinivas.kandagatla@linaro.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index c0d422d0ab94..8b48815ff918 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -69,6 +69,8 @@ struct q6asm_dai_rtd {
 };
 
 struct q6asm_dai_data {
+	struct snd_soc_dai_driver *dais;
+	int num_dais;
 	long long int sid;
 };
 
@@ -889,7 +891,7 @@ static const struct snd_soc_component_driver q6asm_fe_dai_component = {
 	.compr_ops	= &q6asm_dai_compr_ops,
 };
 
-static struct snd_soc_dai_driver q6asm_fe_dais[] = {
+static struct snd_soc_dai_driver q6asm_fe_dais_template[] = {
 	Q6ASM_FEDAI_DRIVER(1),
 	Q6ASM_FEDAI_DRIVER(2),
 	Q6ASM_FEDAI_DRIVER(3),
@@ -903,10 +905,22 @@ static struct snd_soc_dai_driver q6asm_fe_dais[] = {
 static int of_q6asm_parse_dai_data(struct device *dev,
 				    struct q6asm_dai_data *pdata)
 {
-	static struct snd_soc_dai_driver *dai_drv;
+	struct snd_soc_dai_driver *dai_drv;
 	struct snd_soc_pcm_stream empty_stream;
 	struct device_node *node;
-	int ret, id, dir;
+	int ret, id, dir, idx = 0;
+
+
+	pdata->num_dais = of_get_child_count(dev->of_node);
+	if (!pdata->num_dais) {
+		dev_err(dev, "No dais found in DT\n");
+		return -EINVAL;
+	}
+
+	pdata->dais = devm_kcalloc(dev, pdata->num_dais, sizeof(*dai_drv),
+				   GFP_KERNEL);
+	if (!pdata->dais)
+		return -ENOMEM;
 
 	memset(&empty_stream, 0, sizeof(empty_stream));
 
@@ -917,7 +931,8 @@ static int of_q6asm_parse_dai_data(struct device *dev,
 			continue;
 		}
 
-		dai_drv = &q6asm_fe_dais[id];
+		dai_drv = &pdata->dais[idx++];
+		*dai_drv = q6asm_fe_dais_template[id];
 
 		ret = of_property_read_u32(node, "direction", &dir);
 		if (ret)
@@ -955,11 +970,12 @@ static int q6asm_dai_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, pdata);
 
-	of_q6asm_parse_dai_data(dev, pdata);
+	rc = of_q6asm_parse_dai_data(dev, pdata);
+	if (rc)
+		return rc;
 
 	return devm_snd_soc_register_component(dev, &q6asm_fe_dai_component,
-					q6asm_fe_dais,
-					ARRAY_SIZE(q6asm_fe_dais));
+					       pdata->dais, pdata->num_dais);
 }
 
 static const struct of_device_id q6asm_dai_device_id[] = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-03-11 18:04 [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Srinivas Kandagatla
                   ` (2 preceding siblings ...)
  2020-03-12 10:41 ` [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Vinod Koul
@ 2020-04-17 11:24 ` Stephan Gerhold
  2020-04-17 13:02   ` Srinivas Kandagatla
  3 siblings, 1 reply; 12+ messages in thread
From: Stephan Gerhold @ 2020-04-17 11:24 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood

Hi Srini,

On Wed, Mar 11, 2020 at 06:04:20PM +0000, Srinivas Kandagatla wrote:
> QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
> however the default routing do not honour this DT configuration making sound
> card fail to probe. FE dais are also not fully honouring device tree configuration.
> Fix both of them.
> 

I discovered this patch set when QDSP6 audio stopped working after
upgrading to Linux 5.7-rc1. As far as I understand, device tree bindings
should attempt to be backwards compatible wherever possible.
This isn't the case here, although this is not the reason for my mail.
(I don't mind updating my device tree, especially since it is not
upstream yet...)

I have a general question about the design here.

I understand the original motivation for this patch set: Attempting to
configure a TX/RX-only DAI was not possible due to the default routing.
In my opinion this is only relevant for the compressed DAI case.

If we ignore the compressed DAIs for a moment (which can be
unidirectional only), I think we shouldn't care how userspace uses the
available FE/MultiMedia DAIs. We have this huge routing matrix in q6routing,
with 800+ mixers that can be configured in any way possible from userspace.

In "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" you mention:

> This can lead to un-necessary dais in the system which will never be
> used. So honour whats specfied in device tree.

but IMO the FE DAIs are a negligible overhead compared to the routing 
matrix and the many BE DAIs that are really never going to be used
(because nothing is physically connected to the ports).

Even if you restrict FE DAIs to RX/TX only, or disable them entirely,
all the routing mixers still exist for them. They will just result in
configurations that are not usable in any way. IMO the only thing we
gain by restricting the FE DAIs is that the available mixers no longer
match possible configurations.

Before this patch set I used a slightly different approach in my device
tree for MSM8916: I kept all FE DAIs bi-directional, and added DAI links
for all of them. This means that I actually had 8 bi-directional PCM
devices in userspace.

I didn't use all of them - my ALSA UCM configuration only uses
MultiMedia1 for playback and MultiMedia2 for capture.
However, some other userspace (let's say Android) could have chosen
different FE DAIs for whatever reason. We have the overhead for the
routing matrix anyway, so we might as well expose it in my opinion.

My question is: In what way are the FE DAIs really board-specific?

If we expose only some FE DAIs with intended usage per board,
e.g. MultiMedia1 for HDMI, MultiMedia2 for slimbus playback,
     MultiMedia3 for slimbus capture,
I could almost argue that we don't need DPCM at all.
The FE DAIs are always going to be used for the same backend anyway.

This is a bit exaggerated - for example if you have a single compress
DAI per board you probably intend to use it for both HDMI/slimbus.
But this is the feeling I get if we configure the FE DAIs separately
for each board.

I wonder if we should leave configuration of the FE DAIs up to userspace
(e.g. ALSA UCM), and expose the same full set of FE DAIs for each board.

I think this is mostly a matter of convention for configuring FE DAIs
in the device tree - I have some ideas how to make that work
with the existing device tree bindings and for compressed DAIs.
But this mail is already long enough as-is. ;)

I also don't mind if we keep everything as-is
- I just wanted to share what I have been thinking about.

What do you think?

Thanks for reading! ;)
Stephan

> Originally  issue was reported by Vinod Koul
> 
> Srinivas Kandagatla (2):
>   ASoC: qdsp6: q6asm-dai: only enable dais from device tree
>   ASoC: qdsp6: q6routing: remove default routing
> 
>  sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
>  sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
>  2 files changed, 23 insertions(+), 26 deletions(-)
> 
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-04-17 11:24 ` Stephan Gerhold
@ 2020-04-17 13:02   ` Srinivas Kandagatla
  2020-04-17 13:09     ` Mark Brown
  2020-04-17 15:35     ` Stephan Gerhold
  0 siblings, 2 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-04-17 13:02 UTC (permalink / raw)
  To: Stephan Gerhold; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood



On 17/04/2020 12:24, Stephan Gerhold wrote:
> Hi Srini,
> 
> On Wed, Mar 11, 2020 at 06:04:20PM +0000, Srinivas Kandagatla wrote:
>> QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
>> however the default routing do not honour this DT configuration making sound
>> card fail to probe. FE dais are also not fully honouring device tree configuration.
>> Fix both of them.
>>
> 
> I discovered this patch set when QDSP6 audio stopped working after
> upgrading to Linux 5.7-rc1. As far as I understand, device tree bindings
> should attempt to be backwards compatible wherever possible.
> This isn't the case here, although this is not the reason for my mail.
> (I don't mind updating my device tree, especially since it is not
> upstream yet...)
> 
> I have a general question about the design here.
> 
> I understand the original motivation for this patch set: Attempting to
> configure a TX/RX-only DAI was not possible due to the default routing.
> In my opinion this is only relevant for the compressed DAI case.
> 
> If we ignore the compressed DAIs for a moment (which can be
> unidirectional only), I think we shouldn't care how userspace uses the
> available FE/MultiMedia DAIs. We have this huge routing matrix in q6routing,
> with 800+ mixers that can be configured in any way possible from userspace.
> 
> In "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" you mention:
> 
>> This can lead to un-necessary dais in the system which will never be
>> used. So honour whats specfied in device tree.
> 
> but IMO the FE DAIs are a negligible overhead compared to the routing
> matrix and the many BE DAIs that are really never going to be used
> (because nothing is physically connected to the ports).

Two things, one unnecessary mixers, second thing is we need to know how 
many FE dais are in the system, which should be derived from the number 
of dai child nodes. These can potentially be SoC specific or firmware 
specific.

My plan is to cleanup the BE DAIs as well!, any patches welcome!

> 
> Even if you restrict FE DAIs to RX/TX only, or disable them entirely,

I think this is mistake from myside. Alteast according to bindings 
direction property is only "Required for Compress offload dais", code 
should have explicitly ignored it. Here is change that should fix it.

--------------------------->cut<---------------------------------
diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c 
b/sound/soc/qcom/qdsp6/q6asm-dai.c
index 125af00bba53..31f46b25978e 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -1067,6 +1067,11 @@ static int of_q6asm_parse_dai_data(struct device 
*dev,
                 dai_drv = &pdata->dais[idx++];
                 *dai_drv = q6asm_fe_dais_template[id];

+               if (of_property_read_bool(node, "is-compress-dai"))
+                       dai_drv->compress_new = snd_soc_new_compress;
+               else
+                       continue;
+
                 ret = of_property_read_u32(node, "direction", &dir);
                 if (ret)
                         continue;
@@ -1076,8 +1081,6 @@ static int of_q6asm_parse_dai_data(struct device *dev,
                 else if (dir == Q6ASM_DAI_TX)
                         dai_drv->playback = empty_stream;

-               if (of_property_read_bool(node, "is-compress-dai"))
-                       dai_drv->compress_new = snd_soc_new_compress;
         }

         return 0;

--------------------------->cut<---------------------------------

Thanks,
srini

> all the routing mixers still exist for them. They will just result in
> configurations that are not usable in any way. IMO the only thing we
> gain by restricting the FE DAIs is that the available mixers no longer
> match possible configurations.
> 
> Before this patch set I used a slightly different approach in my device
> tree for MSM8916: I kept all FE DAIs bi-directional, and added DAI links
> for all of them. This means that I actually had 8 bi-directional PCM
> devices in userspace.
> 
> I didn't use all of them - my ALSA UCM configuration only uses
> MultiMedia1 for playback and MultiMedia2 for capture.
> However, some other userspace (let's say Android) could have chosen
> different FE DAIs for whatever reason. We have the overhead for the
> routing matrix anyway, so we might as well expose it in my opinion.
> 
> My question is: In what way are the FE DAIs really board-specific?
> 
> If we expose only some FE DAIs with intended usage per board,
> e.g. MultiMedia1 for HDMI, MultiMedia2 for slimbus playback,
>       MultiMedia3 for slimbus capture,
> I could almost argue that we don't need DPCM at all.
> The FE DAIs are always going to be used for the same backend anyway.
> 
> This is a bit exaggerated - for example if you have a single compress
> DAI per board you probably intend to use it for both HDMI/slimbus.
> But this is the feeling I get if we configure the FE DAIs separately
> for each board.
> 
> I wonder if we should leave configuration of the FE DAIs up to userspace
> (e.g. ALSA UCM), and expose the same full set of FE DAIs for each board.
> 
> I think this is mostly a matter of convention for configuring FE DAIs
> in the device tree - I have some ideas how to make that work
> with the existing device tree bindings and for compressed DAIs.
> But this mail is already long enough as-is. ;)
> 
> I also don't mind if we keep everything as-is
> - I just wanted to share what I have been thinking about.
> 
> What do you think?
> 
> Thanks for reading! ;)
> Stephan
> 
>> Originally  issue was reported by Vinod Koul
>>
>> Srinivas Kandagatla (2):
>>    ASoC: qdsp6: q6asm-dai: only enable dais from device tree
>>    ASoC: qdsp6: q6routing: remove default routing
>>
>>   sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
>>   sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
>>   2 files changed, 23 insertions(+), 26 deletions(-)
>>
>> -- 
>> 2.21.0
>>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-04-17 13:02   ` Srinivas Kandagatla
@ 2020-04-17 13:09     ` Mark Brown
  2020-04-17 15:35     ` Stephan Gerhold
  1 sibling, 0 replies; 12+ messages in thread
From: Mark Brown @ 2020-04-17 13:09 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: Stephan Gerhold, linux-kernel, alsa-devel, lgirdwood

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Fri, Apr 17, 2020 at 02:02:08PM +0100, Srinivas Kandagatla wrote:
> On 17/04/2020 12:24, Stephan Gerhold wrote:

> > but IMO the FE DAIs are a negligible overhead compared to the routing
> > matrix and the many BE DAIs that are really never going to be used
> > (because nothing is physically connected to the ports).

> Two things, one unnecessary mixers, second thing is we need to know how many
> FE dais are in the system, which should be derived from the number of dai
> child nodes. These can potentially be SoC specific or firmware specific.

You shouldn't be worrying about unused mixers, ideally we'd be walking
the DAPM graph and masking things - this isn't something that should be
open coded at individual driver levels.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-04-17 13:02   ` Srinivas Kandagatla
  2020-04-17 13:09     ` Mark Brown
@ 2020-04-17 15:35     ` Stephan Gerhold
  2020-04-20  8:32       ` Srinivas Kandagatla
  1 sibling, 1 reply; 12+ messages in thread
From: Stephan Gerhold @ 2020-04-17 15:35 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood

On Fri, Apr 17, 2020 at 02:02:08PM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 17/04/2020 12:24, Stephan Gerhold wrote:
> > Hi Srini,
> > 
> > On Wed, Mar 11, 2020 at 06:04:20PM +0000, Srinivas Kandagatla wrote:
> > > QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
> > > however the default routing do not honour this DT configuration making sound
> > > card fail to probe. FE dais are also not fully honouring device tree configuration.
> > > Fix both of them.
> > > 
> > 
> > I discovered this patch set when QDSP6 audio stopped working after
> > upgrading to Linux 5.7-rc1. As far as I understand, device tree bindings
> > should attempt to be backwards compatible wherever possible.
> > This isn't the case here, although this is not the reason for my mail.
> > (I don't mind updating my device tree, especially since it is not
> > upstream yet...)
> > 
> > I have a general question about the design here.
> > 
> > I understand the original motivation for this patch set: Attempting to
> > configure a TX/RX-only DAI was not possible due to the default routing.
> > In my opinion this is only relevant for the compressed DAI case.
> > 
> > If we ignore the compressed DAIs for a moment (which can be
> > unidirectional only), I think we shouldn't care how userspace uses the
> > available FE/MultiMedia DAIs. We have this huge routing matrix in q6routing,
> > with 800+ mixers that can be configured in any way possible from userspace.
> > 
> > In "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" you mention:
> > 
> > > This can lead to un-necessary dais in the system which will never be
> > > used. So honour whats specfied in device tree.
> > 
> > but IMO the FE DAIs are a negligible overhead compared to the routing
> > matrix and the many BE DAIs that are really never going to be used
> > (because nothing is physically connected to the ports).
> 
> Two things, one unnecessary mixers, second thing is we need to know how many
> FE dais are in the system, which should be derived from the number of dai
> child nodes. These can potentially be SoC specific or firmware specific.
> 

So there are SoCs/firmwares that just support e.g. MultiMedia1-4 and not
all 8 MultiMedia FE DAIs?

> My plan is to cleanup the BE DAIs as well!, any patches welcome!
> 
> > 
> > Even if you restrict FE DAIs to RX/TX only, or disable them entirely,
> 
> I think this is mistake from myside. Alteast according to bindings direction
> property is only "Required for Compress offload dais", code should have
> explicitly ignored it. Here is change that should fix it.
> 

This would make the MultiMedia1-3 bi-directional in sdm845-db845c,
but MultiMedia5-8 would still be disabled.

My question here would then be similar as above:
Is this an arbitrary selection of a reasonable amount of FE DAIs,
or actually based on some firmware limitations?

As I described in the rest of my mail (below your diff),
before this patch set it was simple to just expose all 8 FE DAIs.
At least on MSM8916 all of them work in exactly the same way,
there is no difference between any of them.

If we list what is working in SoC/firmware in the device tree,
would I just list all 8 FE DAIs?

Basically I'm still trying to understand why we limit the number of
FE/MultiMedia DAIs that we expose, when all of them would be working
fine. :)

Thanks,
Stephan

> --------------------------->cut<---------------------------------
> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c
> b/sound/soc/qcom/qdsp6/q6asm-dai.c
> index 125af00bba53..31f46b25978e 100644
> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> @@ -1067,6 +1067,11 @@ static int of_q6asm_parse_dai_data(struct device
> *dev,
>                 dai_drv = &pdata->dais[idx++];
>                 *dai_drv = q6asm_fe_dais_template[id];
> 
> +               if (of_property_read_bool(node, "is-compress-dai"))
> +                       dai_drv->compress_new = snd_soc_new_compress;
> +               else
> +                       continue;
> +
>                 ret = of_property_read_u32(node, "direction", &dir);
>                 if (ret)
>                         continue;
> @@ -1076,8 +1081,6 @@ static int of_q6asm_parse_dai_data(struct device *dev,
>                 else if (dir == Q6ASM_DAI_TX)
>                         dai_drv->playback = empty_stream;
> 
> -               if (of_property_read_bool(node, "is-compress-dai"))
> -                       dai_drv->compress_new = snd_soc_new_compress;
>         }
> 
>         return 0;
> 
> --------------------------->cut<---------------------------------
> 
> Thanks,
> srini
> 
> > all the routing mixers still exist for them. They will just result in
> > configurations that are not usable in any way. IMO the only thing we
> > gain by restricting the FE DAIs is that the available mixers no longer
> > match possible configurations.
> > 
> > Before this patch set I used a slightly different approach in my device
> > tree for MSM8916: I kept all FE DAIs bi-directional, and added DAI links
> > for all of them. This means that I actually had 8 bi-directional PCM
> > devices in userspace.
> > 
> > I didn't use all of them - my ALSA UCM configuration only uses
> > MultiMedia1 for playback and MultiMedia2 for capture.
> > However, some other userspace (let's say Android) could have chosen
> > different FE DAIs for whatever reason. We have the overhead for the
> > routing matrix anyway, so we might as well expose it in my opinion.
> > 
> > My question is: In what way are the FE DAIs really board-specific?
> > 
> > If we expose only some FE DAIs with intended usage per board,
> > e.g. MultiMedia1 for HDMI, MultiMedia2 for slimbus playback,
> >       MultiMedia3 for slimbus capture,
> > I could almost argue that we don't need DPCM at all.
> > The FE DAIs are always going to be used for the same backend anyway.
> > 
> > This is a bit exaggerated - for example if you have a single compress
> > DAI per board you probably intend to use it for both HDMI/slimbus.
> > But this is the feeling I get if we configure the FE DAIs separately
> > for each board.
> > 
> > I wonder if we should leave configuration of the FE DAIs up to userspace
> > (e.g. ALSA UCM), and expose the same full set of FE DAIs for each board.
> > 
> > I think this is mostly a matter of convention for configuring FE DAIs
> > in the device tree - I have some ideas how to make that work
> > with the existing device tree bindings and for compressed DAIs.
> > But this mail is already long enough as-is. ;)
> > 
> > I also don't mind if we keep everything as-is
> > - I just wanted to share what I have been thinking about.
> > 
> > What do you think?
> > 
> > Thanks for reading! ;)
> > Stephan
> > 
> > > Originally  issue was reported by Vinod Koul
> > > 
> > > Srinivas Kandagatla (2):
> > >    ASoC: qdsp6: q6asm-dai: only enable dais from device tree
> > >    ASoC: qdsp6: q6routing: remove default routing
> > > 
> > >   sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
> > >   sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
> > >   2 files changed, 23 insertions(+), 26 deletions(-)
> > > 
> > > -- 
> > > 2.21.0
> > > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-04-17 15:35     ` Stephan Gerhold
@ 2020-04-20  8:32       ` Srinivas Kandagatla
  2020-04-20 12:39         ` Stephan Gerhold
  0 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-04-20  8:32 UTC (permalink / raw)
  To: Stephan Gerhold; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood



On 17/04/2020 16:35, Stephan Gerhold wrote:
> On Fri, Apr 17, 2020 at 02:02:08PM +0100, Srinivas Kandagatla wrote:
>>
>>
>> On 17/04/2020 12:24, Stephan Gerhold wrote:
>>> Hi Srini,
>>>
>>> On Wed, Mar 11, 2020 at 06:04:20PM +0000, Srinivas Kandagatla wrote:
>>>> QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
>>>> however the default routing do not honour this DT configuration making sound
>>>> card fail to probe. FE dais are also not fully honouring device tree configuration.
>>>> Fix both of them.
>>>>
>>>
>>> I discovered this patch set when QDSP6 audio stopped working after
>>> upgrading to Linux 5.7-rc1. As far as I understand, device tree bindings
>>> should attempt to be backwards compatible wherever possible.
>>> This isn't the case here, although this is not the reason for my mail.
>>> (I don't mind updating my device tree, especially since it is not
>>> upstream yet...)
>>>
>>> I have a general question about the design here.
>>>
>>> I understand the original motivation for this patch set: Attempting to
>>> configure a TX/RX-only DAI was not possible due to the default routing.
>>> In my opinion this is only relevant for the compressed DAI case.
>>>
>>> If we ignore the compressed DAIs for a moment (which can be
>>> unidirectional only), I think we shouldn't care how userspace uses the
>>> available FE/MultiMedia DAIs. We have this huge routing matrix in q6routing,
>>> with 800+ mixers that can be configured in any way possible from userspace.
>>>
>>> In "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" you mention:
>>>
>>>> This can lead to un-necessary dais in the system which will never be
>>>> used. So honour whats specfied in device tree.
>>>
>>> but IMO the FE DAIs are a negligible overhead compared to the routing
>>> matrix and the many BE DAIs that are really never going to be used
>>> (because nothing is physically connected to the ports).
>>
>> Two things, one unnecessary mixers, second thing is we need to know how many
>> FE dais are in the system, which should be derived from the number of dai
>> child nodes. These can potentially be SoC specific or firmware specific.
>>
> 
> So there are SoCs/firmwares that just support e.g. MultiMedia1-4 and not
> all 8 MultiMedia FE DAIs?
> 

This all depends on vendor customization to the firmware.
Normally Q6ASM supports up to 8 concurrent streams.

>> My plan is to cleanup the BE DAIs as well!, any patches welcome!
>>
>>>
>>> Even if you restrict FE DAIs to RX/TX only, or disable them entirely,
>>
>> I think this is mistake from myside. Alteast according to bindings direction
>> property is only "Required for Compress offload dais", code should have
>> explicitly ignored it. Here is change that should fix it.
>>
> 
> This would make the MultiMedia1-3 bi-directional in sdm845-db845c,
> but MultiMedia5-8 would still be disabled.
> 
> My question here would then be similar as above:
> Is this an arbitrary selection of a reasonable amount of FE DAIs,
> or actually based on some firmware limitations?
Yes, it is purely firmware limitation.

> 
> As I described in the rest of my mail (below your diff),
> before this patch set it was simple to just expose all 8 FE DAIs.
> At least on MSM8916 all of them work in exactly the same way,
> there is no difference between any of them.
> 
> If we list what is working in SoC/firmware in the device tree,
> would I just list all 8 FE DAIs?

That is the direction, which should also include any dai specific 
properties like compressed case.

> 
> Basically I'm still trying to understand why we limit the number of
> FE/MultiMedia DAIs that we expose, when all of them would be working
> fine. :)
I don't think we need to limit this from Linux side, but Its important 
to take care of the max allowed Q6ASM dais w.r.t DSP.

--srini

> 
> Thanks,
> Stephan
> 
>> --------------------------->cut<---------------------------------
>> diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c
>> b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> index 125af00bba53..31f46b25978e 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
>> @@ -1067,6 +1067,11 @@ static int of_q6asm_parse_dai_data(struct device
>> *dev,
>>                  dai_drv = &pdata->dais[idx++];
>>                  *dai_drv = q6asm_fe_dais_template[id];
>>
>> +               if (of_property_read_bool(node, "is-compress-dai"))
>> +                       dai_drv->compress_new = snd_soc_new_compress;
>> +               else
>> +                       continue;
>> +
>>                  ret = of_property_read_u32(node, "direction", &dir);
>>                  if (ret)
>>                          continue;
>> @@ -1076,8 +1081,6 @@ static int of_q6asm_parse_dai_data(struct device *dev,
>>                  else if (dir == Q6ASM_DAI_TX)
>>                          dai_drv->playback = empty_stream;
>>
>> -               if (of_property_read_bool(node, "is-compress-dai"))
>> -                       dai_drv->compress_new = snd_soc_new_compress;
>>          }
>>
>>          return 0;
>>
>> --------------------------->cut<---------------------------------
>>
>> Thanks,
>> srini
>>
>>> all the routing mixers still exist for them. They will just result in
>>> configurations that are not usable in any way. IMO the only thing we
>>> gain by restricting the FE DAIs is that the available mixers no longer
>>> match possible configurations.
>>>
>>> Before this patch set I used a slightly different approach in my device
>>> tree for MSM8916: I kept all FE DAIs bi-directional, and added DAI links
>>> for all of them. This means that I actually had 8 bi-directional PCM
>>> devices in userspace.
>>>
>>> I didn't use all of them - my ALSA UCM configuration only uses
>>> MultiMedia1 for playback and MultiMedia2 for capture.
>>> However, some other userspace (let's say Android) could have chosen
>>> different FE DAIs for whatever reason. We have the overhead for the
>>> routing matrix anyway, so we might as well expose it in my opinion.
>>>
>>> My question is: In what way are the FE DAIs really board-specific?
>>>
>>> If we expose only some FE DAIs with intended usage per board,
>>> e.g. MultiMedia1 for HDMI, MultiMedia2 for slimbus playback,
>>>        MultiMedia3 for slimbus capture,
>>> I could almost argue that we don't need DPCM at all.
>>> The FE DAIs are always going to be used for the same backend anyway.
>>>
>>> This is a bit exaggerated - for example if you have a single compress
>>> DAI per board you probably intend to use it for both HDMI/slimbus.
>>> But this is the feeling I get if we configure the FE DAIs separately
>>> for each board.
>>>
>>> I wonder if we should leave configuration of the FE DAIs up to userspace
>>> (e.g. ALSA UCM), and expose the same full set of FE DAIs for each board.
>>>
>>> I think this is mostly a matter of convention for configuring FE DAIs
>>> in the device tree - I have some ideas how to make that work
>>> with the existing device tree bindings and for compressed DAIs.
>>> But this mail is already long enough as-is. ;)
>>>
>>> I also don't mind if we keep everything as-is
>>> - I just wanted to share what I have been thinking about.
>>>
>>> What do you think?
>>>
>>> Thanks for reading! ;)
>>> Stephan
>>>
>>>> Originally  issue was reported by Vinod Koul
>>>>
>>>> Srinivas Kandagatla (2):
>>>>     ASoC: qdsp6: q6asm-dai: only enable dais from device tree
>>>>     ASoC: qdsp6: q6routing: remove default routing
>>>>
>>>>    sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
>>>>    sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
>>>>    2 files changed, 23 insertions(+), 26 deletions(-)
>>>>
>>>> -- 
>>>> 2.21.0
>>>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
  2020-04-20  8:32       ` Srinivas Kandagatla
@ 2020-04-20 12:39         ` Stephan Gerhold
  0 siblings, 0 replies; 12+ messages in thread
From: Stephan Gerhold @ 2020-04-20 12:39 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: broonie, linux-kernel, alsa-devel, lgirdwood

On Mon, Apr 20, 2020 at 09:32:18AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 17/04/2020 16:35, Stephan Gerhold wrote:
> > On Fri, Apr 17, 2020 at 02:02:08PM +0100, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 17/04/2020 12:24, Stephan Gerhold wrote:
> > > > Hi Srini,
> > > > 
> > > > On Wed, Mar 11, 2020 at 06:04:20PM +0000, Srinivas Kandagatla wrote:
> > > > > QDSP6 Frontend dais can be configured to work in rx or tx or both rx/tx mode,
> > > > > however the default routing do not honour this DT configuration making sound
> > > > > card fail to probe. FE dais are also not fully honouring device tree configuration.
> > > > > Fix both of them.
> > > > > 
> > > > 
> > > > I discovered this patch set when QDSP6 audio stopped working after
> > > > upgrading to Linux 5.7-rc1. As far as I understand, device tree bindings
> > > > should attempt to be backwards compatible wherever possible.
> > > > This isn't the case here, although this is not the reason for my mail.
> > > > (I don't mind updating my device tree, especially since it is not
> > > > upstream yet...)
> > > > 
> > > > I have a general question about the design here.
> > > > 
> > > > I understand the original motivation for this patch set: Attempting to
> > > > configure a TX/RX-only DAI was not possible due to the default routing.
> > > > In my opinion this is only relevant for the compressed DAI case.
> > > > 
> > > > If we ignore the compressed DAIs for a moment (which can be
> > > > unidirectional only), I think we shouldn't care how userspace uses the
> > > > available FE/MultiMedia DAIs. We have this huge routing matrix in q6routing,
> > > > with 800+ mixers that can be configured in any way possible from userspace.
> > > > 
> > > > In "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" you mention:
> > > > 
> > > > > This can lead to un-necessary dais in the system which will never be
> > > > > used. So honour whats specfied in device tree.
> > > > 
> > > > but IMO the FE DAIs are a negligible overhead compared to the routing
> > > > matrix and the many BE DAIs that are really never going to be used
> > > > (because nothing is physically connected to the ports).
> > > 
> > > Two things, one unnecessary mixers, second thing is we need to know how many
> > > FE dais are in the system, which should be derived from the number of dai
> > > child nodes. These can potentially be SoC specific or firmware specific.
> > > 
> > 
> > So there are SoCs/firmwares that just support e.g. MultiMedia1-4 and not
> > all 8 MultiMedia FE DAIs?
> > 
> 
> This all depends on vendor customization to the firmware.
> Normally Q6ASM supports up to 8 concurrent streams.
> 

I see, thank you!

> > > My plan is to cleanup the BE DAIs as well!, any patches welcome!
> > > 
> > > > 
> > > > Even if you restrict FE DAIs to RX/TX only, or disable them entirely,
> > > 
> > > I think this is mistake from myside. Alteast according to bindings direction
> > > property is only "Required for Compress offload dais", code should have
> > > explicitly ignored it. Here is change that should fix it.
> > > 
> > 
> > This would make the MultiMedia1-3 bi-directional in sdm845-db845c,
> > but MultiMedia5-8 would still be disabled.
> > 
> > My question here would then be similar as above:
> > Is this an arbitrary selection of a reasonable amount of FE DAIs,
> > or actually based on some firmware limitations?
> Yes, it is purely firmware limitation.
> 
> > 
> > As I described in the rest of my mail (below your diff),
> > before this patch set it was simple to just expose all 8 FE DAIs.
> > At least on MSM8916 all of them work in exactly the same way,
> > there is no difference between any of them.
> > 
> > If we list what is working in SoC/firmware in the device tree,
> > would I just list all 8 FE DAIs?
> 
> That is the direction, which should also include any dai specific properties
> like compressed case.
> 
> > 
> > Basically I'm still trying to understand why we limit the number of
> > FE/MultiMedia DAIs that we expose, when all of them would be working
> > fine. :)
> I don't think we need to limit this from Linux side, but Its important to
> take care of the max allowed Q6ASM dais w.r.t DSP.
> 

I think the approach you added in this patch set makes sense if there
are DSP firmwares that offer less FE/MultiMedia DAIs.

For the future it would be nice if we could make this less "confusing"
by hiding/removing the invalid mixers, e.g. if you have only
MultiMedia1 enabled you don't see all the mixers for MultiMedia2-8.
For that it doesn't matter if we do it in q6routing (might become
quite complex with all the macros there), or in a generic way by walking
the DAPM graph as Mark mentioned.

I was also thinking if we can somehow restore the default routing
in a conditional way, so that you don't need to duplicate the
information in the device tree:

  e.g. right now you need to add

	&q6asm {
		dai@0 {
			reg = <0>;
		};
	};

  *and* the routing

	sound {
		audio-routing =
			"MM_DL1", "MultiMedia1 Playback",
			"MultiMedia1 Capture", "MM_UL1";
	};

When I made these changes I needed several tries until I had it correct,
because sometimes I forgot one of the routes, sometimes I mixed up some
numbers etc. And especially when you add all 8 FE DAIs this becomes
really noisy for an information that we could (theoretically) infer
only based on the part under &q6asm.

I will think about it some more and see if I come up with something nice.

Thanks,
Stephan

> > > --------------------------->cut<---------------------------------
> > > diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c
> > > b/sound/soc/qcom/qdsp6/q6asm-dai.c
> > > index 125af00bba53..31f46b25978e 100644
> > > --- a/sound/soc/qcom/qdsp6/q6asm-dai.c
> > > +++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
> > > @@ -1067,6 +1067,11 @@ static int of_q6asm_parse_dai_data(struct device
> > > *dev,
> > >                  dai_drv = &pdata->dais[idx++];
> > >                  *dai_drv = q6asm_fe_dais_template[id];
> > > 
> > > +               if (of_property_read_bool(node, "is-compress-dai"))
> > > +                       dai_drv->compress_new = snd_soc_new_compress;
> > > +               else
> > > +                       continue;
> > > +
> > >                  ret = of_property_read_u32(node, "direction", &dir);
> > >                  if (ret)
> > >                          continue;
> > > @@ -1076,8 +1081,6 @@ static int of_q6asm_parse_dai_data(struct device *dev,
> > >                  else if (dir == Q6ASM_DAI_TX)
> > >                          dai_drv->playback = empty_stream;
> > > 
> > > -               if (of_property_read_bool(node, "is-compress-dai"))
> > > -                       dai_drv->compress_new = snd_soc_new_compress;
> > >          }
> > > 
> > >          return 0;
> > > 
> > > --------------------------->cut<---------------------------------
> > > 
> > > Thanks,
> > > srini
> > > 
> > > > all the routing mixers still exist for them. They will just result in
> > > > configurations that are not usable in any way. IMO the only thing we
> > > > gain by restricting the FE DAIs is that the available mixers no longer
> > > > match possible configurations.
> > > > 
> > > > Before this patch set I used a slightly different approach in my device
> > > > tree for MSM8916: I kept all FE DAIs bi-directional, and added DAI links
> > > > for all of them. This means that I actually had 8 bi-directional PCM
> > > > devices in userspace.
> > > > 
> > > > I didn't use all of them - my ALSA UCM configuration only uses
> > > > MultiMedia1 for playback and MultiMedia2 for capture.
> > > > However, some other userspace (let's say Android) could have chosen
> > > > different FE DAIs for whatever reason. We have the overhead for the
> > > > routing matrix anyway, so we might as well expose it in my opinion.
> > > > 
> > > > My question is: In what way are the FE DAIs really board-specific?
> > > > 
> > > > If we expose only some FE DAIs with intended usage per board,
> > > > e.g. MultiMedia1 for HDMI, MultiMedia2 for slimbus playback,
> > > >        MultiMedia3 for slimbus capture,
> > > > I could almost argue that we don't need DPCM at all.
> > > > The FE DAIs are always going to be used for the same backend anyway.
> > > > 
> > > > This is a bit exaggerated - for example if you have a single compress
> > > > DAI per board you probably intend to use it for both HDMI/slimbus.
> > > > But this is the feeling I get if we configure the FE DAIs separately
> > > > for each board.
> > > > 
> > > > I wonder if we should leave configuration of the FE DAIs up to userspace
> > > > (e.g. ALSA UCM), and expose the same full set of FE DAIs for each board.
> > > > 
> > > > I think this is mostly a matter of convention for configuring FE DAIs
> > > > in the device tree - I have some ideas how to make that work
> > > > with the existing device tree bindings and for compressed DAIs.
> > > > But this mail is already long enough as-is. ;)
> > > > 
> > > > I also don't mind if we keep everything as-is
> > > > - I just wanted to share what I have been thinking about.
> > > > 
> > > > What do you think?
> > > > 
> > > > Thanks for reading! ;)
> > > > Stephan
> > > > 
> > > > > Originally  issue was reported by Vinod Koul
> > > > > 
> > > > > Srinivas Kandagatla (2):
> > > > >     ASoC: qdsp6: q6asm-dai: only enable dais from device tree
> > > > >     ASoC: qdsp6: q6routing: remove default routing
> > > > > 
> > > > >    sound/soc/qcom/qdsp6/q6asm-dai.c | 30 +++++++++++++++++++++++-------
> > > > >    sound/soc/qcom/qdsp6/q6routing.c | 19 -------------------
> > > > >    2 files changed, 23 insertions(+), 26 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.21.0
> > > > > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-04-20 13:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 18:04 [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Srinivas Kandagatla
2020-03-11 18:04 ` [PATCH 1/2] ASoC: qdsp6: q6asm-dai: only enable dais from device tree Srinivas Kandagatla
2020-03-12 13:13   ` Applied "ASoC: qdsp6: q6asm-dai: only enable dais from device tree" to the asoc tree Mark Brown
2020-03-11 18:04 ` [PATCH 2/2] ASoC: qdsp6: q6routing: remove default routing Srinivas Kandagatla
2020-03-12 13:13   ` Applied "ASoC: qdsp6: q6routing: remove default routing" to the asoc tree Mark Brown
2020-03-12 10:41 ` [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings Vinod Koul
2020-04-17 11:24 ` Stephan Gerhold
2020-04-17 13:02   ` Srinivas Kandagatla
2020-04-17 13:09     ` Mark Brown
2020-04-17 15:35     ` Stephan Gerhold
2020-04-20  8:32       ` Srinivas Kandagatla
2020-04-20 12:39         ` Stephan Gerhold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).