| Message ID | 20200305145314.32579-6-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Skylake: Fix HDaudio and Dmic | expand |
Hey, On Thu, 5 Mar 2020, Cezary Rojewski wrote: > --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c > +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c > @@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = { > { "Digital CPU Capture", NULL, "Digital Codec Capture" }, > { "codec2_in", NULL, "Alt Analog CPU Capture" }, > { "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" }, > + > + { "dmic01_hifi", NULL, "DMIC01 Rx" }, > + { "DMIC01 Rx", NULL, "DMIC AIF" }, hmm, we need to figure out something else for this. This very same table already has: » /* digital mics */ » {"DMic", NULL, "SoC DMIC"}, .. so now we have dmic entries two times in the same initializer list. But a more pressing issue is that this breaks platforms using SOF firmware: [ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi ... maybe you can align the topology to mathc so we can reuse the same widget mapping for both SOF and SST firmwares..? Br, Kai
> But a more pressing issue is that this breaks platforms using SOF > firmware: > > [ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi > [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi > > ... maybe you can align the topology to mathc so we can reuse the same > widget mapping for both SOF and SST firmwares..? Yeah, I thought this would break userspace and installed topologies and that just confirms it. Adding hard-coded routes is really not recommended. the alternate solution is what I suggested in another thread "No sound since 5.4 on skl_n88l25_s4567", we could mark this machine driver as having an incomplete topology and remove the topology checks in the core. I couldn't really test my initial patch but that that Cezary unintentionally broke SOF actually that gives me a tool to test the solution.
On 2020-03-06 16:49, Pierre-Louis Bossart wrote: >> But a more pressing issue is that this breaks platforms using SOF >> firmware: >> >> [ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink >> widget found for dmic01_hifi >> [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed >> to add route DMIC01 Rx -> direct -> dmic01_hifi >> >> ... maybe you can align the topology to mathc so we can reuse the same >> widget mapping for both SOF and SST firmwares..? > > Yeah, I thought this would break userspace and installed topologies and > that just confirms it. Adding hard-coded routes is really not recommended. > > the alternate solution is what I suggested in another thread "No sound > since 5.4 on skl_n88l25_s4567", we could mark this machine driver as > having an incomplete topology and remove the topology checks in the core. > > I couldn't really test my initial patch but that that Cezary > unintentionally broke SOF actually that gives me a tool to test the > solution. Glad to help Pierre ^)^ and thanks for the review Kai. Guys, we've simply taken long-standing working example such as skl_rt286 or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which Intel's SST topologies link against via dmic01_hifi. That has always been the case. No bad intentions, the exact opposite is true: taken old path approach to make sure nothing is broken. Turns out SOF does things differently. Thanks for spotting/ testing this out on your end. Not a problem to adjust topology on our end, though. In fact, I've already done that on your requested, tested it out and it works just fine. In consequence: - this patch will be dropped from the series - topology patch provided for alsa-ucm-conf will be updated accordingly (dmic01_hifi widget will cease to exist) Czarek
> Guys, we've simply taken long-standing working example such as skl_rt286 > or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said > machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which > Intel's SST topologies link against via dmic01_hifi. That has always > been the case. No bad intentions, the exact opposite is true: taken old > path approach to make sure nothing is broken. Turns out SOF does things > differently. Thanks for spotting/ testing this out on your end. It's actually not SOF, but recent changes in the asoc core that will stop the probe if a route cannot be added, instead of just spewing a warning. I think it's a good change to force topologies to be complete, but in cases where a previously released topology cannot be adjusted we need a backwards compatible solution. that's my plan for the rest of the afternoon. > Not a problem to adjust topology on our end, though. In fact, I've > already done that on your requested, tested it out and it works just > fine. In consequence: > - this patch will be dropped from the series > - topology patch provided for alsa-ucm-conf will be updated accordingly > (dmic01_hifi widget will cease to exist) Sounds good, thanks Cezary.
On 2020-03-06 20:49, Pierre-Louis Bossart wrote: >> Guys, we've simply taken long-standing working example such as >> skl_rt286 or bxt_rt298 and applied the missing diff between >> skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes >> BE: DMIC01 Rx which Intel's SST topologies link against via >> dmic01_hifi. That has always been the case. No bad intentions, the >> exact opposite is true: taken old path approach to make sure nothing >> is broken. Turns out SOF does things differently. Thanks for spotting/ >> testing this out on your end. > > It's actually not SOF, but recent changes in the asoc core that will > stop the probe if a route cannot be added, instead of just spewing a > warning. > > I think it's a good change to force topologies to be complete, but in > cases where a previously released topology cannot be adjusted we need a > backwards compatible solution. that's my plan for the rest of the > afternoon. Agree, missing topology routes should not be permissive. Although my point was about the fact that those 2 routes actually exist in every SST machine driver (in essence linking DMIC01 Rx with platform via internal dmic01_hifi widget). >> Not a problem to adjust topology on our end, though. In fact, I've >> already done that on your requested, tested it out and it works just >> fine. In consequence: >> - this patch will be dropped from the series >> - topology patch provided for alsa-ucm-conf will be updated >> accordingly (dmic01_hifi widget will cease to exist) > > Sounds good, thanks Cezary. Awaiting comments for the rest of the patches if any! Especially how low - kernel version wise - should the fixes be backported. Thanks in advance for your input and time. Czarek
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c index fe2d3a23a4ef..0126dfc7ac24 100644 --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c @@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = { { "Digital CPU Capture", NULL, "Digital Codec Capture" }, { "codec2_in", NULL, "Alt Analog CPU Capture" }, { "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" }, + + { "dmic01_hifi", NULL, "DMIC01 Rx" }, + { "DMIC01 Rx", NULL, "DMIC AIF" }, }; SND_SOC_DAILINK_DEF(dummy_codec,