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