| Message ID | 87jzjpe5vh.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: grace time for DPCM cleanup | expand |
On 5/19/24 18:31, Kuninori Morimoto wrote: > Current DCPM is checking CPU side availability only, but it should also > check Codec availability. But because of long DPCM operation history, > it is possible that the some Codec driver check have been bypassed. > > It should be error, but let's add grace time to update driver. > > This patch indicates warning in above case. Each applicable driver need > to update during this grace time. ... > + /* > + * REMOVE ME > + * > + * Current DPCM is checking CPU side only, but both CPU and Codec should be > + * checked. Indicate warning if there was CPU / Codec mismatch. > + * To keep compatibility, warning only for now. > + */ > + if ((dai_link->dpcm_playback || dai_link->playback_only) && > + !has_playback_both) > + dev_warn(rtd->card->dev, > + "System requests playback, but not available (%s)." > + " Please update Codec driver\n", > + dai_link->stream_name); > + if ((dai_link->dpcm_capture || dai_link->capture_only) && > + !has_capture_both) > + dev_warn(rtd->card->dev, > + "System requests capture, but not available (%s)." > + " Please update Codec driver\n", > + dai_link->stream_name); > + I mentioned in my previous feedback that this isn't quite right. There are cases where the CPU dai reports capabilities that the codec DAI does not support - e.g. when the AEC reference is generated in firmware on the host DSP. And sure enough we get that warning in the first test: https://sof-ci.01.org/linuxpr/PR5005/build3040/devicetest/index.html?model=GLK_BOB_DA7219-ipc3&testcase=verify-kernel-boot-log May 20 13:35:38 jf-glk-bob-da7219-1 kernel: sof_da7219 glk_da7219_def: System requests capture, but not available (SSP1-Codec). Please update Codec driver For those systems, trying to match CPU and codec dais is not going to work. Either we skip this verification or we have an escape mechanism to avoid triggering errors.
Hi Pierre-Louis Thank you for your feedback > I mentioned in my previous feedback that this isn't quite right. There > are cases where the CPU dai reports capabilities that the codec DAI does > not support - e.g. when the AEC reference is generated in firmware on > the host DSP. Hmm... I thought all issue was solved... > For those systems, trying to match CPU and codec dais is not going to > work. Either we skip this verification or we have an escape mechanism to > avoid triggering errors. Sorry, but I'm not 100% understand about your situation. Why Codec can't have channels_min ? If the Codec can flexibly adjust to paired CPU, Codec can have full channels support, like dummy DAI ? This means verification is based on CPU only. Is it not enough ? and/or Can you show me the driver ? static struct snd_soc_dai_driver dummy_dai = { ... .playback = { => .channels_min = 1, => .channels_max = 384, ... }, .capture = { ... => .channels_min = 1, => .channels_max = 384, ... }, ... }; Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
>> I mentioned in my previous feedback that this isn't quite right. There >> are cases where the CPU dai reports capabilities that the codec DAI does >> not support - e.g. when the AEC reference is generated in firmware on >> the host DSP. > > Hmm... I thought all issue was solved... > >> For those systems, trying to match CPU and codec dais is not going to >> work. Either we skip this verification or we have an escape mechanism to >> avoid triggering errors. > > Sorry, but I'm not 100% understand about your situation. > Why Codec can't have channels_min ? > If the Codec can flexibly adjust to paired CPU, Codec can have full channels > support, like dummy DAI ? This means verification is based on CPU only. > Is it not enough ? and/or Can you show me the driver ? > > static struct snd_soc_dai_driver dummy_dai = { > ... > .playback = { > => .channels_min = 1, > => .channels_max = 384, > ... > }, > .capture = { > ... > => .channels_min = 1, > => .channels_max = 384, > ... > }, > ... > }; We cannot change the Maxim amplifier driver, it's used in a variety of usages and platforms, and there's no reason to create a fake capture dai just to reflect the use of a capture stream on the CPU side on some Chromebooks. The dailinks used for amplifiers in sound/soc/intel/boards/sof_boards_helpers.c set dpcm_capture always link->dpcm_capture = 1; /* feedback stream or firmware-generated echo reference */ which means that this test will fail: if ((dai_link->dpcm_capture || dai_link->capture_only) && !has_capture_both) I don't disagree that the unconditional use of dpcm_capture isn't very elegant, but it is what it is. This platform has been around since 2019 and still has about 6 or 7 years of support, so we can't break it with stricter criteria.
Hi Pierre-Louis, Mark > We cannot change the Maxim amplifier driver, it's used in a variety of > usages and platforms, and there's no reason to create a fake capture dai > just to reflect the use of a capture stream on the CPU side on some > Chromebooks. Why cannot ?? There is no effect to user if Maxim driver has full channel setting same as dammy DAI. It will be handled together with CPU, and system gets CPU channels as-is. > I don't disagree that the unconditional use of dpcm_capture isn't very > elegant, but it is what it is. This platform has been around since 2019 > and still has about 6 or 7 years of support, so we can't break it with > stricter criteria. My opinion is that working without channels settings is wrong. I can understand that it was working in long years, but it is working with wrong settings. So justify a wrong-settings is not good idea for me. And I don't think it is stricter criteria, it becomes *sane* criteria, IMO. Because it was working with wrong-settings, we need to makes it sane. This is the reason why it has grace time. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 5/20/24 20:15, Kuninori Morimoto wrote: > > Hi Pierre-Louis, Mark > >> We cannot change the Maxim amplifier driver, it's used in a variety of >> usages and platforms, and there's no reason to create a fake capture dai >> just to reflect the use of a capture stream on the CPU side on some >> Chromebooks. > > Why cannot ?? > There is no effect to user if Maxim driver has full channel setting same as > dammy DAI. It will be handled together with CPU, and system gets CPU > channels as-is. That would be changing the meaning and purpose of a 'dummy dai' A 'dummy dai' has historically been used when data was transmitted/received but the control of that DAI was done externally with a sideband interface. Here there is just no hardware for capture in the Maxim amp. Adding a pretend DAI for the sake of adding a stricter 'sanity check' does not sound good to me. >> I don't disagree that the unconditional use of dpcm_capture isn't very >> elegant, but it is what it is. This platform has been around since 2019 >> and still has about 6 or 7 years of support, so we can't break it with >> stricter criteria. > > My opinion is that working without channels settings is wrong. > I can understand that it was working in long years, but it is working with > wrong settings. So justify a wrong-settings is not good idea for me. > And I don't think it is stricter criteria, it becomes *sane* criteria, IMO. > > Because it was working with wrong-settings, we need to makes it sane. > This is the reason why it has grace time. allow me to give you another counter example, beyond the AEC reference I mentioned earlier. It's not uncommon for CPU DAIs to have loopback capabilities, which are used for tests on boards where the codec has no capture capabilities. I think it's a feature that needs to be allowed, not a 'wrong setting'.
On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote: > allow me to give you another counter example, beyond the AEC reference I > mentioned earlier. It's not uncommon for CPU DAIs to have loopback > capabilities, which are used for tests on boards where the codec has no > capture capabilities. I think it's a feature that needs to be allowed, > not a 'wrong setting'. This is something we could do properly if we had full digital routing rather than bolting things on the side of the CPU<->CODEC model - having these things where we have to take a CODEC into account even though we're not actually using it is one of the big issues with DPCM.
On 5/21/24 10:12, Mark Brown wrote: > On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote: > >> allow me to give you another counter example, beyond the AEC reference I >> mentioned earlier. It's not uncommon for CPU DAIs to have loopback >> capabilities, which are used for tests on boards where the codec has no >> capture capabilities. I think it's a feature that needs to be allowed, >> not a 'wrong setting'. > > This is something we could do properly if we had full digital routing > rather than bolting things on the side of the CPU<->CODEC model - having > these things where we have to take a CODEC into account even though > we're not actually using it is one of the big issues with DPCM. No disagreement here, these echo references and loopbacks are ugly to support with the dependency between playback and capture directions that isn't well handled, e.g. if someone starts to capture before playback started. For now we're kind of stuck, what I would suggest is just to remove the extra check that both CPU and codec dai can support a direction, and move on with the other cleanups suggested by Morimoto-san.
On Tue, May 21, 2024 at 11:03:41AM -0500, Pierre-Louis Bossart wrote: > On 5/21/24 10:12, Mark Brown wrote: > > On Tue, May 21, 2024 at 08:43:09AM -0500, Pierre-Louis Bossart wrote: > > This is something we could do properly if we had full digital routing > > rather than bolting things on the side of the CPU<->CODEC model - having > > these things where we have to take a CODEC into account even though > > we're not actually using it is one of the big issues with DPCM. > No disagreement here, these echo references and loopbacks are ugly to > support with the dependency between playback and capture directions that > isn't well handled, e.g. if someone starts to capture before playback > started. > For now we're kind of stuck, what I would suggest is just to remove the > extra check that both CPU and codec dai can support a direction, and > move on with the other cleanups suggested by Morimoto-san. Oh, I agree - my point is that as things stand the framework really can't cope with what needs expressing so we need these things that don't look quite right.
Hi Pierre-Louis, Mark Thank you for clarifying the issue. > allow me to give you another counter example, beyond the AEC reference I > mentioned earlier. It's not uncommon for CPU DAIs to have loopback > capabilities, which are used for tests on boards where the codec has no > capture capabilities. I think it's a feature that needs to be allowed, > not a 'wrong setting'. This helped me to understand the situation. > > For now we're kind of stuck, what I would suggest is just to remove the > > extra check that both CPU and codec dai can support a direction, and > > move on with the other cleanups suggested by Morimoto-san. > > Oh, I agree - my point is that as things stand the framework really > can't cope with what needs expressing so we need these things that don't > look quite right. I think same situation can be happen not only DPCM, but normal connection, too ? And my personally want to have validation check as much as possible, and automatically validation without Card/rtd flags. In case of ignoring Codec check on DPCM, it allows unexpected direction, I think. For example if it is not using dummy DAI, and in case of CPU can playback/capture, Codec can playback, and user expect playback only, in this case unexpected "capture" will be available. He need to add playback_only flag, but it is not good for me. Can we avoid validation check if DAI has some kind of new flag, like ignore_pair ? pseudo code is like this if (valid(cpu, PLAYBACK)) has_cpu_playback = 1; if (valid(cpu, CAPTURE)) has_cpu_capture = 1; if (valid(codec, PLAYBACK)) has_codec_playback = 1; if (valid(codec, CAPTURE)) has_codec_capture = 1; if (cpu->ignore_pair) { has_codec_playback = 1; has_codec_capture = 1; } if (codec->ignore_pair) { has_cpu_playback = 1; has_cpu_capture = 1; } Or more detail ignore_pair_playback, ignore_pair_capture Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
> In case of ignoring Codec check on DPCM, it allows unexpected direction, > I think. For example if it is not using dummy DAI, and in case of CPU can > playback/capture, Codec can playback, and user expect playback only, > in this case unexpected "capture" will be available. He need to add > playback_only flag, but it is not good for me. > > Can we avoid validation check if DAI has some kind of new flag, like > ignore_pair ? > > pseudo code is like this > > if (valid(cpu, PLAYBACK)) > has_cpu_playback = 1; > > if (valid(cpu, CAPTURE)) > has_cpu_capture = 1; > > if (valid(codec, PLAYBACK)) > has_codec_playback = 1; > > if (valid(codec, CAPTURE)) > has_codec_capture = 1; > > if (cpu->ignore_pair) { > has_codec_playback = 1; > has_codec_capture = 1; > } > > if (codec->ignore_pair) { > has_cpu_playback = 1; > has_cpu_capture = 1; > } > > Or more detail ignore_pair_playback, ignore_pair_capture There are two options a) we don't even try to test the codec-cpu match in terms of capabilities. That means the same behavior as today. b) we add a chicken bit for platforms to disable the codec-cpu match if it breaks specific platforms. The problem with b) is that we don't know what platforms will break. I reported one example that was caught by our CI, but there could be additional Chromebooks impacted, who knows. My vote is a).
Hi Pierre-Louis, Mark again > I think same situation can be happen not only DPCM, but normal connection, > too ? And my personally want to have validation check as much as possible, > and automatically validation without Card/rtd flags. > > In case of ignoring Codec check on DPCM, it allows unexpected direction, > I think. For example if it is not using dummy DAI, and in case of CPU can > playback/capture, Codec can playback, and user expect playback only, > in this case unexpected "capture" will be available. He need to add > playback_only flag, but it is not good for me. This patch-set has big effect to each vender, so let use step-by-step approach for safety. I will remove Codec check from DPCM on next v4. But it is our one of problems to be solved, IMO. I will add mark there to indicate it when we remove dpcm_xxx flag. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis Thank you for your feedback > There are two options > a) we don't even try to test the codec-cpu match in terms of > capabilities. That means the same behavior as today. > b) we add a chicken bit for platforms to disable the codec-cpu match if > it breaks specific platforms. > > The problem with b) is that we don't know what platforms will break. I > reported one example that was caught by our CI, but there could be > additional Chromebooks impacted, who knows. > > My vote is a). Yeah, it is a little big problem for us. Let's keep current style for now. I will mark it on source code when we remove dpcm_xxx flag to be possible to resolve someday. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index c4d80cad59829..71db7379e08aa 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2795,6 +2795,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, { struct snd_soc_dai_link *dai_link = rtd->dai_link; struct snd_soc_dai *cpu_dai; + struct snd_soc_dai *codec_dai; struct snd_soc_dai_link_ch_map *ch_maps; int has_playback = 0; int has_capture = 0; @@ -2806,15 +2807,25 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, } if (dai_link->dynamic || dai_link->no_pcm) { + int has_playback_both = 0; + int has_capture_both = 0; for_each_rtd_ch_maps(rtd, i, ch_maps) { cpu_dai = snd_soc_rtd_to_cpu(rtd, ch_maps->cpu); + codec_dai = snd_soc_rtd_to_codec(rtd, ch_maps->codec); if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK)) has_playback = 1; if (snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE)) has_capture = 1; + + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && + snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK)) + has_playback_both = 1; + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && + snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE)) + has_capture_both = 1; } /* @@ -2850,9 +2861,39 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, has_playback = 0; } } - } else { - struct snd_soc_dai *codec_dai; + /* + * REMOVE ME + * + * Current DPCM is checking CPU side only, but both CPU and Codec should be + * checked. Indicate warning if there was CPU / Codec mismatch. + * To keep compatibility, warning only for now. + */ + if ((dai_link->dpcm_playback || dai_link->playback_only) && + !has_playback_both) + dev_warn(rtd->card->dev, + "System requests playback, but not available (%s)." + " Please update Codec driver\n", + dai_link->stream_name); + if ((dai_link->dpcm_capture || dai_link->capture_only) && + !has_capture_both) + dev_warn(rtd->card->dev, + "System requests capture, but not available (%s)." + " Please update Codec driver\n", + dai_link->stream_name); + + /* + * REMOVE ME + * + * In case of there was no dpcm_xxx flag, and CPU / Codec mismatch, + * follow new style + */ + if (!dai_link->dpcm_playback && has_playback) + has_playback = has_playback_both; + if (!dai_link->dpcm_capture && has_capture) + has_capture = has_capture_both; + + } else { /* Adapt stream for codec2codec links */ int cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); int cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK);