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
