[PATCH v2.5] ASoC: Replace dpcm_playback/capture to playback/capture_assertion
Hi Mark, Amadeusz, Jerome, Pierre-Louis
This is v2.5 patch which is preparation for v3. Because full-patch-set has over 20 patches, to avoid posting huge patch-bomb, I try to post main patch only for preparation.
In v2.5, it will use xxx_assertion flag, and exising dpcm_xxx and xxx_only flags will be converted to it. I think it can keep compatibility and possible to indicate link error same as before which was conserned by Pierre-Louis. Please let me know if it still not good match.
One of my big consern is Amadeusz/Jerome's idea which uses "XXX | YYY" style. To avoid confusion, let's name each style as below here.
[Flag style] unsigned int flags; #define SND_SOC_FLAGS_ASSERTION_PLAYBACK BIT(x) #define SND_SOC_FLAGS_ASSERTION_CAPTURE BIT(x) ...
[BitField style] unsigned int playback_assertion:1; unsigned int capture_assertion:1; ...
This v2.5 patch is using [BitField style] and I think it is not a big problem from "code point of view", but I think [Flag style] is better for "human understandable point of view", because we can define like below, for example.
#define SND_SOC_FLAGS_AVAILABLE_PLAYBACK /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_CAPTURE #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL_WITH_ASSERTION (SND_SOC_FLAGS_ASSERTION_PLAYBACK | SND_SOC_FLAGS_ASSERTION_CAPTURE) #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_ONLY SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_ONLY SND_SOC_FLAGS_ASSERTION_CAPTURE
Switch to [Flag style] is OK for me, but one consern is that in such case, people will wonder "why ASoC is using both [Flag style] and [BitField style] in the same time ?", because we are using [BitField style] for other flags.
So, my suggestion is that next v3 patch uses [Flag style]. And after that, post new patch-set to switch [BitField style] to [Flag style] for other flags. But I wonder is this good approach ?
v2 -> v2.5 - use xxx_assertion flag - dpcm_xxx -> xxx_assertion - xxx_only -> xxx_assertion - only [01/xx] patch
v1 -> v2 - based on latest ASoC branch - keep comment on Intel - tidyup patch title - tidyup DPCM BE warning output condition - Add new patch for Document
Link: https://lore.kernel.org/r/87o7b353of.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87zfuesz8y.wl-kuninori.morimoto.gx@renesas.com
Thank you for your help !!
Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
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.
(X) part is for DPCM, and it checks playback/capture availability if dai_link has dpcm_playback/capture flag (a)(b). This availability check should be available not only for DPCM, but for all connections. But it is not mandatory option. Let's name it as assertion.
In case of having assertion flag, non specific side will be disabled. For example if it has playback_assertion but not have capture_assertion, capture will be force disabled.
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
By expanding this assertion to all connections, we can use same code for all connections, this means code can be more cleanup.
Here, CPU / Codec validation check relationship is like this
DPCM [CPU/xxxx]-[yyyy/Codec] ^^^^ ^^^^ non DPCM [CPU/Codec] ^^^^^^^^^^^
DPCM part (X) is checking only CPU DAI, and non DPCM part (Y) is checking both CPU/Codec DAI
Current validation check on DPCM ignores Codec DAI, but there is no reason to do it. We should check both CPU/Codec in all connection. This patch expands validation check to all cases.
[CPU/xxxx]-[yyyy/Codec] *****
In many case (not all case), above [xxxx][yyyy] part are "dummy" DAI which is always valid for both playback/capture. But unfortunately DPCM BE Codec (**** part) had been no validation check before, and some Codec driver doesn't have necessary settings for it. This means all cases validation check breaks compatibility on some vender's drivers. Thus this patch temporary uses dummy DAI at BPCM BE Codec part, and avoid compatibility error. But it should be removed in the future.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 10 ++++ sound/soc/soc-pcm.c | 132 ++++++++++++++++++++++++-------------------- 2 files changed, 82 insertions(+), 60 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 0376f7e4c15d..931816890755 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -809,6 +809,16 @@ struct snd_soc_dai_link { unsigned int dpcm_capture:1; unsigned int dpcm_playback:1;
+ /* + * Capture / Playback support assertion. Having assertion flag is not mandatory. In case of + * having assertion flag, non specific side will be disabled. For example if it has + * playback_assertion but not have capture_assertion, capture will be force disabled + * see + * soc_get_playback_capture() + */ + unsigned int capture_assertion:1; + unsigned int playback_assertion:1; + /* DPCM used FE & BE merged format */ unsigned int dpcm_merged_format:1; /* DPCM used FE & BE merged channel */ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 77ee103b7cd1..7a27d3e110ac 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,77 +2808,84 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, return -EINVAL; }
- 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_capture) { - stream = SNDRV_PCM_STREAM_CAPTURE; + /* + * REMOVE ME + * + * dpcm_playback/capture will be used as playback/capture_assertion + */ + if (dai_link->playback_only && dai_link->capture_only) { + dev_err(rtd->dev, "both playback_only / capture_only are set\n"); + return -EINVAL; + } + if (dai_link->playback_only) + dai_link->playback_assertion = 1; + if (dai_link->capture_only) + dai_link->capture_assertion = 1; + if (dai_link->dpcm_playback) + dai_link->playback_assertion = 1; + if (dai_link->dpcm_capture) + dai_link->capture_assertion = 1;
- 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 (dai_link->playback_only) - has_capture = 0; + 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->capture_only) - has_playback = 0; + /* + * Assertion check + * + * xxx_assertion flag is not mandatory + */ + if (dai_link->playback_assertion) { + if (!has_playback) { + dev_err(rtd->dev, "%s playback assertion check error\n", dai_link->stream_name); + 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, "%s capture assertion check error\n", dai_link->stream_name); + 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, "substream %s has no playback, no capture\n", dai_link->stream_name); - return -EINVAL; }
On 4/10/2024 4:54 AM, Kuninori Morimoto wrote:
Hi Mark, Amadeusz, Jerome, Pierre-Louis
This is v2.5 patch which is preparation for v3. Because full-patch-set has over 20 patches, to avoid posting huge patch-bomb, I try to post main patch only for preparation.
In v2.5, it will use xxx_assertion flag, and exising dpcm_xxx and xxx_only flags will be converted to it. I think it can keep compatibility and possible to indicate link error same as before which was conserned by Pierre-Louis. Please let me know if it still not good match.
One of my big consern is Amadeusz/Jerome's idea which uses "XXX | YYY" style. To avoid confusion, let's name each style as below here.
[Flag style] unsigned int flags; #define SND_SOC_FLAGS_ASSERTION_PLAYBACK BIT(x) #define SND_SOC_FLAGS_ASSERTION_CAPTURE BIT(x) ...
[BitField style] unsigned int playback_assertion:1; unsigned int capture_assertion:1; ...
This v2.5 patch is using [BitField style] and I think it is not a big problem from "code point of view", but I think [Flag style] is better for "human understandable point of view", because we can define like below, for example.
#define SND_SOC_FLAGS_AVAILABLE_PLAYBACK /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_WITH_ASSERTION SND_SOC_FLAGS_ASSERTION_CAPTURE #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL /* no flag is needed */ #define SND_SOC_FLAGS_AVAILABLE_BIDIRECTIONAL_WITH_ASSERTION (SND_SOC_FLAGS_ASSERTION_PLAYBACK | SND_SOC_FLAGS_ASSERTION_CAPTURE) #define SND_SOC_FLAGS_AVAILABLE_PLAYBACK_ONLY SND_SOC_FLAGS_ASSERTION_PLAYBACK #define SND_SOC_FLAGS_AVAILABLE_CAPTURE_ONLY SND_SOC_FLAGS_ASSERTION_CAPTURE
Switch to [Flag style] is OK for me, but one consern is that in such case, people will wonder "why ASoC is using both [Flag style] and [BitField style] in the same time ?", because we are using [BitField style] for other flags.
So, my suggestion is that next v3 patch uses [Flag style]. And after that, post new patch-set to switch [BitField style] to [Flag style] for other flags. But I wonder is this good approach ?
v2 -> v2.5
- use xxx_assertion flag
- dpcm_xxx -> xxx_assertion
- xxx_only -> xxx_assertion
- only [01/xx] patch
v1 -> v2
- based on latest ASoC branch
- keep comment on Intel
- tidyup patch title
- tidyup DPCM BE warning output condition
- Add new patch for Document
Link: https://lore.kernel.org/r/87o7b353of.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87zfuesz8y.wl-kuninori.morimoto.gx@renesas.com
Thank you for your help !!
Best regards
Renesas Electronics Ph.D. Kuninori Morimoto
Hi,
I've looked a bit at why the original flags were introduced.
For dpcm_playback and dpcm_capture there is https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... it feels like it works around the fact that some DAIs didn't had set channels_min properly, which will of course not work with snd_soc_dai_stream_valid(). Perhaps if we want to remove this flag we should just set channels_min everywhere where we want to have playback or capture and just remove flag?
Similar story with playback_only and capture_only, which was introduced in two patch series to remove unsupported FEs from list of available ones: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... I feel like if driver author knows that one of directions should be unused they should just set min_channels to 0 to disable it on either BE or FE DAI (personally I would prefer FE DAI in most cases).
Following above we would have no need for any flags at all. What do you think?
Thanks, Amadeusz
Hi Amadeusz
Thank you for your research
it feels like it works around the fact that some DAIs didn't had set channels_min properly, which will of course not work with snd_soc_dai_stream_valid(). Perhaps if we want to remove this flag we should just set channels_min everywhere where we want to have playback or capture and just remove flag?
Interesting. It seems detail usage was updated from original to emphasize checking.
For me, I have no objection about this.
Similar story with playback_only and capture_only, which was introduced in two patch series to remove unsupported FEs from list of available ones:
(snip)
I feel like if driver author knows that one of directions should be unused they should just set min_channels to 0 to disable it on either BE or FE DAI (personally I would prefer FE DAI in most cases).
Unfortunately, this is not good idea, IMO. Because min_channels is set at snd_soc_dai_driver driver, and almost all drivers are using same settings for all device.
For example if you use same 4 x Codecs, but want to use playback_only on 1 Codec, min_channels = 0 will be used on all Codecs. Same things can be happen on CPU side, too. So we want to have xxx_only option, IMO.
Thank you for your help !!
Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Mark, Amadeusz, Jerome, Pierre-Louis
[Flag style] unsigned int flags; #define SND_SOC_FLAGS_ASSERTION_PLAYBACK BIT(x) #define SND_SOC_FLAGS_ASSERTION_CAPTURE BIT(x) ...
[BitField style] unsigned int playback_assertion:1; unsigned int capture_assertion:1; ...
(snip)
So, my suggestion is that next v3 patch uses [Flag style]. And after that, post new patch-set to switch [BitField style] to [Flag style] for other flags. But I wonder is this good approach ?
I would like to fixup my comment above. Actually, I have no objection about current [BitField style]. If there is no special opinion/objection about this, I will post [BitField style] in v3 patch-set with assertion flag (= dpcm_xxx, and xxx_only compatible). Switching to [Flag style] can be next topic/patch-set.
I will wait comment mail until end of this week. If there was no comment, I will post v3 patch then.
Thank you for your help !!
Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On Sun, Apr 14, 2024 at 11:40:14PM +0000, Kuninori Morimoto wrote:
I would like to fixup my comment above. Actually, I have no objection about current [BitField style]. If there is no special opinion/objection about this, I will post [BitField style] in v3 patch-set with assertion flag (= dpcm_xxx, and xxx_only compatible). Switching to [Flag style] can be next topic/patch-set.
I will wait comment mail until end of this week. If there was no comment, I will post v3 patch then.
I don't have super strong opinions either way on the style here.
participants (3)
-
Amadeusz Sławiński
-
Kuninori Morimoto
-
Mark Brown