From: Stephan Gerhold <stephan@gerhold.net>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linux-kernel@vger.kernel.org, lgirdwood@gmail.com
Subject: Re: [PATCH 0/2] ASoC: qdsp6: fix default FE dais and routings.
Date: Fri, 17 Apr 2020 17:35:34 +0200 [thread overview]
Message-ID: <20200417153534.GA65143@gerhold.net> (raw)
In-Reply-To: <03d0d14c-d52c-460b-0232-184156f62eb7@linaro.org>
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
> > >
next prev parent reply other threads:[~2020-04-17 15:36 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-04-20 8:32 ` Srinivas Kandagatla
2020-04-20 12:39 ` Stephan Gerhold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200417153534.GA65143@gerhold.net \
--to=stephan@gerhold.net \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=srinivas.kandagatla@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).