| Message ID | 87y19xudor.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Replace dpcm_playback/capture to playback/capture_only | expand |
On 3/31/24 19:30, Kuninori Morimoto wrote: > Current soc_get_playback_capture() (A) is checking playback/capture > availability for DPCM (X) / Normal (Y) / Codec2Codec (Z) connections. > > (A) static int soc_get_playback_capture(...) > { > ... > ^ if (dai_link->dynamic || dai_link->no_pcm) { > | ... > |(a) if (dai_link->dpcm_playback) { > | ... > | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > |(*) ... > | v } > | ... > (X) } > |(b) if (dai_link->dpcm_capture) { > | ... > | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > |(*) ... > | v } > | ... > v } > } else { > ^ ^ /* Adapt stream for codec2codec links */ > |(Z) int cpu_capture = ... > | v int cpu_playback = ... > (Y) > | ^ for_each_rtd_ch_maps(rtd, i, ch_maps) { > |(*) ... > v v } > } > ... > } > > (*) part is checking each DAI's availability. > > At first, (X) part is for DPCM, and it checks playback/capture > availability if dai_link has dpcm_playback/capture flag (a)(b). > But we are already using playback/capture_only flag for Normal (Y) and > Codec2Codec (Z). We can use this flags for DPCM too. > > Before After > dpcm_playback = 1; => /* no flags */ > dpcm_capture = 1; > > dpcm_playback = 1; => playback_only = 1; > > dpcm_capture = 1; => capture_only = 1; > > dpcm_playback = 0; => error > dpcm_capture = 0; > > This patch convert dpcm_ flags to _only flag, and dpcm_ flag will be > removed if all driver switched to _only flags. > > Here, CPU <-> Codec relationship is like this > > DPCM > [CPU/dummy]-[dummy/Codec] > ^^^^ ^^^^^ > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > DPCM part (X) is checking only CPU DAI, and > Normal part (Y) is checking both CPU/Codec DAI > > Here, validation check on dummy DAI is always true, > Therefor we want to expand validation check to all cases. > > One note here is that unfortunately DPCM BE Codec had been no validation > check before, but all cases validation check breaks compatibility on > some vender's devices. Thus this patch ignore it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 90 +++++++++++++++++++-------------------------- > 1 file changed, 38 insertions(+), 52 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 77ee103b7cd1..8761ae8fc05f 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2793,7 +2793,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > int *playback, int *capture) > { > struct snd_soc_dai_link *dai_link = rtd->dai_link; > + struct snd_soc_dai_link_ch_map *ch_maps; > struct snd_soc_dai *cpu_dai; > + struct snd_soc_dai *codec_dai; > + struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc); > + int cpu_playback; > + int cpu_capture; > int has_playback = 0; > int has_capture = 0; > int i; > @@ -2803,65 +2808,46 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, > return -EINVAL; > } > > + /* REMOVE ME */ > if (dai_link->dynamic || dai_link->no_pcm) { > - int stream; > - > - if (dai_link->dpcm_playback) { > - stream = SNDRV_PCM_STREAM_PLAYBACK; > - > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - has_playback = 1; > - break; > - } > - } > - if (!has_playback) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support playback for stream %s\n", > - dai_link->stream_name); > - return -EINVAL; > - } > + if (dai_link->dpcm_playback && !dai_link->dpcm_capture) > + dai_link->playback_only = 1; > + if (!dai_link->dpcm_playback && dai_link->dpcm_capture) > + dai_link->capture_only = 1; > + if (!dai_link->dpcm_playback && !dai_link->dpcm_capture) { > + dev_err(rtd->dev, "no dpcm_playback/capture are selected\n"); > + return -EINVAL; > } > - if (dai_link->dpcm_capture) { > - stream = SNDRV_PCM_STREAM_CAPTURE; > + } > > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - has_capture = 1; > - break; > - } > - } > + /* Adapt stream for codec2codec links */ > + cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK); > + cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); > > - if (!has_capture) { > - dev_err(rtd->card->dev, > - "No CPU DAIs support capture for stream %s\n", > - dai_link->stream_name); > - return -EINVAL; > - } > - } > - } else { > - struct snd_soc_dai_link_ch_map *ch_maps; > - struct snd_soc_dai *codec_dai; > - > - /* 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); > + /* > + * see > + * soc.h :: [dai_link->ch_maps Image sample] > + */ > + 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); > > /* > - * see > - * soc.h :: [dai_link->ch_maps Image sample] > + * FIXME > + * > + * DPCM BE Codec has been no checked before. > + * It should be checked, but it breaks compatibility. > + * It ignores BE Codec here, so far. > */ > - 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(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) > - has_playback = 1; > - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) > - has_capture = 1; > - } > + if (dai_link->no_pcm) > + codec_dai = dummy_dai; > + > + if (snd_soc_dai_stream_valid(cpu_dai, cpu_playback) && > + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK)) > + has_playback = 1; > + if (snd_soc_dai_stream_valid(cpu_dai, cpu_capture) && > + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE)) > + has_capture = 1; > } The problem I have is with the following code (not shown with diff) if (dai_link->playback_only) has_capture = 0; if (dai_link->capture_only) has_playback = 0; So with this grand unification, all the loops above may make a decision that could be overridden by these two branches. This was not the case before for DPCM, all the 'has_capture' and 'has_playback' variables were used as a verification of the dai_link settings with an error thrown e.g. if the dpcm_playback was set without any DAIs supporting playback. Now the dailink settings are used unconditionally. There is one warning added if there are no settings for a dailink, but we've lost the detection of a mismatch between dailink and the set of cpu/codec dais that are part of this dailink.
Hi Pierre-Louis Thank you for your review > The problem I have is with the following code (not shown with diff) > > if (dai_link->playback_only) > has_capture = 0; > > if (dai_link->capture_only) > has_playback = 0; > > So with this grand unification, all the loops above may make a decision > that could be overridden by these two branches. > > This was not the case before for DPCM, all the 'has_capture' and > 'has_playback' variables were used as a verification of the dai_link > settings with an error thrown e.g. if the dpcm_playback was set without > any DAIs supporting playback. I could understand so far. > Now the dailink settings are used unconditionally. There is one warning > added if there are no settings for a dailink, but we've lost the > detection of a mismatch between dailink and the set of cpu/codec dais > that are part of this dailink. But sorry I could understand this. "There is one warning added if there are no settings for a dailink" By [01/16] patch ? I think no warning is added. Or do you mean by [15/16] patch ? "we've lost the detection of a mismatch between dailink and the set of cpu/codec dais that are part of this dailink" Sorry I couldn't understand about this. Which mismatch detection we lost ?? Concrete sample / code / image is very helpful for me to well understanding. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis, again > > The problem I have is with the following code (not shown with diff) > > > > if (dai_link->playback_only) > > has_capture = 0; > > > > if (dai_link->capture_only) > > has_playback = 0; > > > > So with this grand unification, all the loops above may make a decision > > that could be overridden by these two branches. > > > > This was not the case before for DPCM, all the 'has_capture' and > > 'has_playback' variables were used as a verification of the dai_link > > settings with an error thrown e.g. if the dpcm_playback was set without > > any DAIs supporting playback. Hmm... above 2 branches are used for DPCM too before. > > Now the dailink settings are used unconditionally. There is one warning > > added if there are no settings for a dailink, but we've lost the > > detection of a mismatch between dailink and the set of cpu/codec dais > > that are part of this dailink. Does this mean, for example you want to have warning/error by dpcm_playback flag if you are thinking it can use playback , but FE or BE playback was not valid ? If so, yes indeed this patch removes such flags. But I wonder why you want to get it in case of DPCM only ? I can understand if all case want to have it. Then, I think we can check _only for this purpose too ? (A) if dai_link has playback_only -> it should have has_playback (B) if dai_link has capture_only -> it should have has_capture (C) if dai_link don't have both xxx_only -> it should have both has_xxx I think we already have (A)(B) check. We want to add (C) check ? If my understanding was correct, the things dpcm_xxx flag can do is also can do by xxx_only flag. But dpcm_xxx flag is used only DPCM, but xxx_only flag is used on all cases. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/1/24 19:21, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your review > >> The problem I have is with the following code (not shown with diff) >> >> if (dai_link->playback_only) >> has_capture = 0; >> >> if (dai_link->capture_only) >> has_playback = 0; >> >> So with this grand unification, all the loops above may make a decision >> that could be overridden by these two branches. >> >> This was not the case before for DPCM, all the 'has_capture' and >> 'has_playback' variables were used as a verification of the dai_link >> settings with an error thrown e.g. if the dpcm_playback was set without >> any DAIs supporting playback. > > I could understand so far. > >> Now the dailink settings are used unconditionally. There is one warning >> added if there are no settings for a dailink, but we've lost the >> detection of a mismatch between dailink and the set of cpu/codec dais >> that are part of this dailink. > > But sorry I could understand this. > > "There is one warning added if there are no settings for a dailink" > > By [01/16] patch ? I think no warning is added. Or do you mean by [15/16] > patch ? Yes I looked at the entire series, it's just too complicated to look with diff. > > "we've lost the detection of a mismatch between dailink and the > set of cpu/codec dais that are part of this dailink" > > Sorry I couldn't understand about this. > Which mismatch detection we lost ?? Concrete sample / code / image > is very helpful for me to well understanding. With the older code, if the dpcm_playback was set for the dailink but there isn't any dai connected to support playback, an error was thrown. With the new code, if playback_only is set but there isn't any dai connected, there is no error thrown, is there? The point is that these flags are sometimes set in the machine driver, sometimes set in the framework, and the open is which one has the priority.
On 4/2/24 01:43, Kuninori Morimoto wrote: > > Hi Pierre-Louis, again > >>> The problem I have is with the following code (not shown with diff) >>> >>> if (dai_link->playback_only) >>> has_capture = 0; >>> >>> if (dai_link->capture_only) >>> has_playback = 0; >>> >>> So with this grand unification, all the loops above may make a decision >>> that could be overridden by these two branches. >>> >>> This was not the case before for DPCM, all the 'has_capture' and >>> 'has_playback' variables were used as a verification of the dai_link >>> settings with an error thrown e.g. if the dpcm_playback was set without >>> any DAIs supporting playback. > > Hmm... above 2 branches are used for DPCM too before. Not really, playback_only and capture_only were never set so those two branches were always false. > >>> Now the dailink settings are used unconditionally. There is one warning >>> added if there are no settings for a dailink, but we've lost the >>> detection of a mismatch between dailink and the set of cpu/codec dais >>> that are part of this dailink. > > Does this mean, for example you want to have warning/error by dpcm_playback > flag if you are thinking it can use playback , but FE or BE playback was > not valid ? Again we had a verification that if the dpcm_playback was set at the dailink level, it was actually supported by the dais. We seem to have lost this verification. We only have an error when there are no settings at all. > > If so, yes indeed this patch removes such flags. > But I wonder why you want to get it in case of DPCM only ? It's helpful to detect invalid configurations. And I see to reason why the removal of flags should change the functionality. > I can understand if all case want to have it. > > Then, I think we can check _only for this purpose too ? > > (A) if dai_link has playback_only -> it should have has_playback > (B) if dai_link has capture_only -> it should have has_capture > (C) if dai_link don't have both xxx_only -> it should have both has_xxx > > I think we already have (A)(B) check. We want to add (C) check ? > > If my understanding was correct, the things dpcm_xxx flag can do is also > can do by xxx_only flag. But dpcm_xxx flag is used only DPCM, but xxx_only > flag is used on all cases. I am only concerned about mismatches between dailinks and dai capabilities. The logic above is fine, but it's in the scope of the dailink only.
Hi Pierre-Louis Thank you for your feedback. I could understand your comment 80%, but not yet 100% > With the older code, if the dpcm_playback was set for the dailink but > there isn't any dai connected to support playback, an error was thrown. > > With the new code, if playback_only is set but there isn't any dai > connected, there is no error thrown, is there? (snip) > Again we had a verification that if the dpcm_playback was set at the > dailink level, it was actually supported by the dais. > > We seem to have lost this verification. We only have an error when there > are no settings at all. Pseudo code of new soc_get_playback_capture() is like this soc_get_playback_capture(...) { ... ^ for_each_rtd_ch_maps(...) { | ... (A) has_playback = xxx; | has_capture = xxx; v } ^ if (dai_link->playback_only) | has_capture = 0; (B) | if (dai_link->capture_only) v has_playback = 0; ^ if (!has_playback && !has_capture) { (C) dev_err(...); v return -EINVAL; } ... } In old/new soc_get_playback_capture(), has_xxx will be set at least if one of rtd connected DAI can handle playback/capture. In new code, it will be handled at (A). And unneeded has_xxx will be removed if xxx_only was set (B) Then, if neither has_xxx was set, it will be error (C) In new code, if playback_only is set but there isn't any dai connected, there is no error thrown, is there? If playback_only was set, has_capture will be removed at (B). And if DAI was not playback-able, this means has_playback was not set at (A). In such case, (C) will indicate error. Same things happen if capture_only too. So, old functions validation still exist in my opinion, but am I misunderstanding ? One note here is that in DPCM case, old function checks CPU only, but new function checks both CPU and Codec. 2nd note is that in current version of patch-set, if dai_link doesn't have xxx_only settings (= it should have both playback/capture), but if DAI has has_playback or has_capture only, it can't detect about it. I suggested it in previous mail, and will fix in v3 > The point is that these flags are sometimes set in the machine driver, > sometimes set in the framework, and the open is which one has the priority. I couldn't understand this. I think "machine driver" = CPU/Codec driver, but what is "these flags" which is sometimes set in machine driver, and sometimes set in framework ?? dpcm_xxx ? xxx_only ?? I don't think framework set these... Or do you mean [09/16] patch (= it will set dai_link->no_pcm) ? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/3/24 20:53, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your feedback. > I could understand your comment 80%, but not yet 100% > >> With the older code, if the dpcm_playback was set for the dailink but >> there isn't any dai connected to support playback, an error was thrown. >> >> With the new code, if playback_only is set but there isn't any dai >> connected, there is no error thrown, is there? > (snip) >> Again we had a verification that if the dpcm_playback was set at the >> dailink level, it was actually supported by the dais. >> >> We seem to have lost this verification. We only have an error when there >> are no settings at all. > > Pseudo code of new soc_get_playback_capture() is like this > > soc_get_playback_capture(...) > { > ... > ^ for_each_rtd_ch_maps(...) { > | ... > (A) has_playback = xxx; > | has_capture = xxx; > v } > > ^ if (dai_link->playback_only) > | has_capture = 0; > (B) > | if (dai_link->capture_only) > v has_playback = 0; > > ^ if (!has_playback && !has_capture) { > (C) dev_err(...); > v return -EINVAL; > } > ... > } > > In old/new soc_get_playback_capture(), has_xxx will be set at least > if one of rtd connected DAI can handle playback/capture. > In new code, it will be handled at (A). > > And unneeded has_xxx will be removed if xxx_only was set (B) The problem is that we have two sources of information 1) the dais included in the dailink (the (A) part above) 2) the dailink itself (the (B) part above) the code in A) constructs the information from the ground-up, but it's overridden by B). You can view it as 'removing unneeded has_xxx' flags, but it's also a problem is the dailink information is incorrect... In the past we would report an error if the dailink was not aligned with the dais. Now we just ignore the dai information. That's the concern, we're changing the behavior. > Then, if neither has_xxx was set, it will be error (C) That's not the concern. The concern is a discrepancy between A) and B). > > In new code, if playback_only is set but there isn't any dai > connected, there is no error thrown, is there? > > If playback_only was set, has_capture will be removed at (B). > And if DAI was not playback-able, this means has_playback was not set at (A). > In such case, (C) will indicate error. Same things happen if capture_only too. > > So, old functions validation still exist in my opinion, but am I > misunderstanding ? > > One note here is that in DPCM case, old function checks CPU only, > but new function checks both CPU and Codec. > > 2nd note is that in current version of patch-set, if dai_link doesn't > have xxx_only settings (= it should have both playback/capture), but if > DAI has has_playback or has_capture only, it can't detect about it. > I suggested it in previous mail, and will fix in v3 > >> The point is that these flags are sometimes set in the machine driver, >> sometimes set in the framework, and the open is which one has the priority. > > I couldn't understand this. > > I think "machine driver" = CPU/Codec driver, but what is "these flags" > which is sometimes set in machine driver, and sometimes set in framework ?? > dpcm_xxx ? xxx_only ?? I don't think framework set these... The has_xxx flag is set based on dai capabilities in (A) - which I call "the framework" OR by the machine driver setting the playback_only/capture_only flags, then used in (B) to override (A). When you have two sources of information competing to set a state, we have to be really careful on which one has priority/precedence.
Hi Pierre-Louis Thank you for clarifying the point > > And unneeded has_xxx will be removed if xxx_only was set (B) > > The problem is that we have two sources of information > > 1) the dais included in the dailink (the (A) part above) > 2) the dailink itself (the (B) part above) > > the code in A) constructs the information from the ground-up, but it's > overridden by B). > > You can view it as 'removing unneeded has_xxx' flags, but it's also a > problem is the dailink information is incorrect... > > In the past we would report an error if the dailink was not aligned with > the dais. Now we just ignore the dai information. Ah, OK now I could understand. Hmm... is below what you mean in summary? dpcm_xxx is used to declare that the DAI/dailink is possible to use playback/capture. For example dpcm_playback means the DAI / dailink should playback-able, if not it is error. xxx_only is used to limit the playback/capture. For example the DAI / dailink can use both playback and capture, but want to use playback only for some reasons, we can use playback_only. So these are used for different purpose. Hmm... I re-consider about it for many cases, and indeed these can't merge. But in such case, this feature is needed not only for DPCM ? Now Jerome / Amadeusz are suggesting new idea to use bitfield idea. We can use it ? #define PLAYBACK_VALID BIT(0) #define CAPTURE_VALID BIT(1) #define PLAYBACK_LIMIT BIT(2) #define CAPTURE_LIMIT BIT(3) I need to think about keeping compatibility, but maybe OK. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Pierre-Louis, again > dpcm_xxx is used to declare that the DAI/dailink is possible to use > playback/capture. For example dpcm_playback means the DAI / dailink > should playback-able, if not it is error. > > xxx_only is used to limit the playback/capture. > For example the DAI / dailink can use both playback and capture, > but want to use playback only for some reasons, we can use playback_only. My pervious patch-set was "try to merge dpcm_xxx and xxx_only flag", but next patch will be "expand assertion flag to all connection". This "assertion flag" was originaly dpcm_xxx. In next patch-set, it will assume for example current "dpcm_playback" as "playback_assertion". It can be used not only for DPCM, but all connection, but is not mandatory option. Its pseudo code is like below, but what do you think ? soc_get_playback_capture(...) { ... /* * get HW / DAI availability */ for_each_rtd_ch_maps(...) { ... has_playback = xxx; has_capture = xxx; } /* * "xxx_assersion" was "dpcm_xxx" before, but expand to * all connection. It is not mandatory option. * It will be error if dai_link has xxx_assersion flag, * but DAI was not valid */ if (dai_link->playback_assertion && !has_playback) { dev_err(rtd->dev, ...); return -EINVAL; } if (dai_link->capture_assertion && !has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } /* * xxx_only flag limits availability. It will indicate warning * if DAI was not valid. */ if (dai_link->playback_only) { if (!has_capture) dev_warn(rtd->dev, ...); has_capture = 0; } if (dai_link->capture_only) { if (!has_playback) dev_warn(rtd->dev, ...); has_playback = 0; } /* * No Playback, No Capture is error */ if (!has_playback && !has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } ... } Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/7/24 22:55, Kuninori Morimoto wrote: > > Hi Pierre-Louis, again > >> dpcm_xxx is used to declare that the DAI/dailink is possible to use >> playback/capture. For example dpcm_playback means the DAI / dailink >> should playback-able, if not it is error. >> >> xxx_only is used to limit the playback/capture. >> For example the DAI / dailink can use both playback and capture, >> but want to use playback only for some reasons, we can use playback_only. > > My pervious patch-set was "try to merge dpcm_xxx and xxx_only flag", > but next patch will be "expand assertion flag to all connection". > This "assertion flag" was originaly dpcm_xxx. > > In next patch-set, it will assume for example current "dpcm_playback" > as "playback_assertion". It can be used not only for DPCM, but > all connection, but is not mandatory option. > > Its pseudo code is like below, but what do you think ? > > soc_get_playback_capture(...) > { > ... > /* > * get HW / DAI availability > */ > for_each_rtd_ch_maps(...) { > ... > has_playback = xxx; > has_capture = xxx; > } > > /* > * "xxx_assersion" was "dpcm_xxx" before, but expand to > * all connection. It is not mandatory option. > * It will be error if dai_link has xxx_assersion flag, > * but DAI was not valid > */ > if (dai_link->playback_assertion && !has_playback) { > dev_err(rtd->dev, ...); > return -EINVAL; > } > if (dai_link->capture_assertion && !has_capture) { > dev_err(rtd->dev, ...); > return -EINVAL; > } > > /* > * xxx_only flag limits availability. It will indicate warning > * if DAI was not valid. > */ > if (dai_link->playback_only) { > if (!has_capture) > dev_warn(rtd->dev, ...); > has_capture = 0; > } > > if (dai_link->capture_only) { > if (!has_playback) > dev_warn(rtd->dev, ...); > has_playback = 0; > } > > /* > * No Playback, No Capture is error > */ > if (!has_playback && !has_capture) { > dev_err(rtd->dev, ...); > return -EINVAL; > } > ... > } The code looks fine, but what are we trying to achieve? I thought the idea was to have a single field at the dailink, and with the example above we would still have two - just like today. This looks like a lot of code churn in many drivers for limited benefits. Or I am missing something?
Hi Pierre-Louis Thank you for your feedback > The code looks fine, but what are we trying to achieve? > I thought the idea was to have a single field at the dailink, and with > the example above we would still have two - just like today. > This looks like a lot of code churn in many drivers for limited > benefits. Or I am missing something? Yeah. Main purpose of this patch-set is cleanup soc-pcm code which is very complex today. After sending mail, I noticed that xxx_only flag can be merged into new xxx_assertion flag. For example "playback_only" means it must playback available. One note here is that xxx_assertion flag is not mandatory dpcm_playback -> playabck_assertion = 1 dpcm_capture -> capture_assertion = 1 playback_only -> playback_assertion = 1 capture_assertion = 0 capture_only -> playback_assertion = 0 capture_assertion = 1 /* * Assertion check * * xxx_assertion flag is not mandatory */ if (dai_link->playback_assertion) { if (!has_playback) { dev_err(rtd->dev, ...); return -EINVAL; } /* makes it plyaback only */ if (!dai_link->capture_assertion) has_capture = 0; } if (dai_link->capture_assertion) { if (!has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } /* makes it capture only */ if (!dai_link->playback_assertion) has_playback = 0; } /* * Detect Mismatch */ if (!has_playback && !has_capture) { dev_err(rtd->dev, ...); return -EINVAL; } 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 77ee103b7cd1..8761ae8fc05f 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2793,7 +2793,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int *playback, int *capture) { struct snd_soc_dai_link *dai_link = rtd->dai_link; + struct snd_soc_dai_link_ch_map *ch_maps; struct snd_soc_dai *cpu_dai; + struct snd_soc_dai *codec_dai; + struct snd_soc_dai *dummy_dai = snd_soc_find_dai(&snd_soc_dummy_dlc); + int cpu_playback; + int cpu_capture; int has_playback = 0; int has_capture = 0; int i; @@ -2803,65 +2808,46 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, return -EINVAL; } + /* REMOVE ME */ if (dai_link->dynamic || dai_link->no_pcm) { - int stream; - - if (dai_link->dpcm_playback) { - stream = SNDRV_PCM_STREAM_PLAYBACK; - - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - has_playback = 1; - break; - } - } - if (!has_playback) { - dev_err(rtd->card->dev, - "No CPU DAIs support playback for stream %s\n", - dai_link->stream_name); - return -EINVAL; - } + if (dai_link->dpcm_playback && !dai_link->dpcm_capture) + dai_link->playback_only = 1; + if (!dai_link->dpcm_playback && dai_link->dpcm_capture) + dai_link->capture_only = 1; + if (!dai_link->dpcm_playback && !dai_link->dpcm_capture) { + dev_err(rtd->dev, "no dpcm_playback/capture are selected\n"); + return -EINVAL; } - if (dai_link->dpcm_capture) { - stream = SNDRV_PCM_STREAM_CAPTURE; + } - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - has_capture = 1; - break; - } - } + /* Adapt stream for codec2codec links */ + cpu_playback = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_PLAYBACK); + cpu_capture = snd_soc_get_stream_cpu(dai_link, SNDRV_PCM_STREAM_CAPTURE); - if (!has_capture) { - dev_err(rtd->card->dev, - "No CPU DAIs support capture for stream %s\n", - dai_link->stream_name); - return -EINVAL; - } - } - } else { - struct snd_soc_dai_link_ch_map *ch_maps; - struct snd_soc_dai *codec_dai; - - /* 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); + /* + * see + * soc.h :: [dai_link->ch_maps Image sample] + */ + 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); /* - * see - * soc.h :: [dai_link->ch_maps Image sample] + * FIXME + * + * DPCM BE Codec has been no checked before. + * It should be checked, but it breaks compatibility. + * It ignores BE Codec here, so far. */ - 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(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - has_playback = 1; - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - has_capture = 1; - } + if (dai_link->no_pcm) + codec_dai = dummy_dai; + + if (snd_soc_dai_stream_valid(cpu_dai, cpu_playback) && + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK)) + has_playback = 1; + if (snd_soc_dai_stream_valid(cpu_dai, cpu_capture) && + snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE)) + has_capture = 1; } if (dai_link->playback_only)
Current soc_get_playback_capture() (A) is checking playback/capture availability for DPCM (X) / Normal (Y) / Codec2Codec (Z) connections. (A) static int soc_get_playback_capture(...) { ... ^ if (dai_link->dynamic || dai_link->no_pcm) { | ... |(a) if (dai_link->dpcm_playback) { | ... | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { |(*) ... | v } | ... (X) } |(b) if (dai_link->dpcm_capture) { | ... | ^ for_each_rtd_cpu_dais(rtd, i, cpu_dai) { |(*) ... | v } | ... v } } else { ^ ^ /* Adapt stream for codec2codec links */ |(Z) int cpu_capture = ... | v int cpu_playback = ... (Y) | ^ for_each_rtd_ch_maps(rtd, i, ch_maps) { |(*) ... v v } } ... } (*) part is checking each DAI's availability. At first, (X) part is for DPCM, and it checks playback/capture availability if dai_link has dpcm_playback/capture flag (a)(b). But we are already using playback/capture_only flag for Normal (Y) and Codec2Codec (Z). We can use this flags for DPCM too. Before After dpcm_playback = 1; => /* no flags */ dpcm_capture = 1; dpcm_playback = 1; => playback_only = 1; dpcm_capture = 1; => capture_only = 1; dpcm_playback = 0; => error dpcm_capture = 0; This patch convert dpcm_ flags to _only flag, and dpcm_ flag will be removed if all driver switched to _only flags. Here, CPU <-> Codec relationship is like this DPCM [CPU/dummy]-[dummy/Codec] ^^^^ ^^^^^ Normal [CPU/Codec] ^^^^^^^^^^^ DPCM part (X) is checking only CPU DAI, and Normal part (Y) is checking both CPU/Codec DAI Here, validation check on dummy DAI is always true, Therefor we want to expand validation check to all cases. One note here is that unfortunately DPCM BE Codec had been no validation check before, but all cases validation check breaks compatibility on some vender's devices. Thus this patch ignore it. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- sound/soc/soc-pcm.c | 90 +++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 52 deletions(-)