Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.

[Stephan Gerhold]

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
> > >