Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
From: Stephan Gerhold
Date: Fri Apr 17 2020 - 11:35:46 EST
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
> > >