| Message ID | 87r0rep4we.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: replace dpcm_playback/capture to playback/capture_only | expand |
On 5/18/2023 7:47 AM, 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_codec_dais(rtd, i, codec_dai) { > |(*) ... > 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; > > This patch enables both flags case, but dpcm_playback/capture flags > will be removed if all driver were switched to new playback/capture_only > flags. > > Here, CPU <-> Codec relationship is like this > > DPCM > [CPU/dummy]-[dummy/Codec] > ^^^^^^^^^^^ > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > (X) part is checking only CPU DAI, and > (Y) part is checking both CPU/Codec DAI > > This means (X)/(Y) are checking same position. > Because dammy DAI is always available, > we can share same code for all cases (= X/Y/Z). > > This patch merge these. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
On 5/18/2023 7:47 AM, 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_codec_dais(rtd, i, codec_dai) { > |(*) ... > 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; > > This patch enables both flags case, but dpcm_playback/capture flags > will be removed if all driver were switched to new playback/capture_only > flags. > > Here, CPU <-> Codec relationship is like this > > DPCM > [CPU/dummy]-[dummy/Codec] > ^^^^^^^^^^^ > Normal > [CPU/Codec] > ^^^^^^^^^^^ > > (X) part is checking only CPU DAI, and > (Y) part is checking both CPU/Codec DAI > > This means (X)/(Y) are checking same position. > Because dammy DAI is always available, > we can share same code for all cases (= X/Y/Z). > > This patch merge these. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-pcm.c | 75 +++++++++++++-------------------------------- > 1 file changed, 22 insertions(+), 53 deletions(-) > > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index af5d4e1effdf..f47ddf660c57 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -2732,7 +2732,10 @@ 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 *codec_dai; > struct snd_soc_dai *cpu_dai; > + int cpu_capture = SNDRV_PCM_STREAM_CAPTURE; > + int cpu_playback = SNDRV_PCM_STREAM_PLAYBACK; > int tmp_playback = 0; > int tmp_capture = 0; > int i; > @@ -2748,61 +2751,27 @@ 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; > + /* Adapt stream for codec2codec links */ > + if (dai_link->c2c_params) { > + cpu_capture = SNDRV_PCM_STREAM_PLAYBACK; > + cpu_playback = SNDRV_PCM_STREAM_CAPTURE; > + } > > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - tmp_playback = 1; > - break; > - } > - } > - if (!tmp_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 */ > + 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; > > - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > - tmp_capture = 1; > - break; > - } > - } > - > - if (!tmp_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 *codec_dai; > - > - /* Adapt stream for codec2codec links */ > - int cpu_capture = dai_link->c2c_params ? > - SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE; > - int cpu_playback = dai_link->c2c_params ? > - SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; > - > - for_each_rtd_codec_dais(rtd, i, codec_dai) { > - cpu_dai = asoc_rtd_to_cpu(rtd, i); > - > - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) > - tmp_playback = 1; > - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && > - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) > - tmp_capture = 1; > - } > + for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > + codec_dai = asoc_rtd_to_codec(rtd, i); /* get paired codec */ > + > + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && > + snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) > + tmp_playback = 1; > + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && > + snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) > + tmp_capture = 1; > } > > if (dai_link->playback_only) I put the patchset to test and it fails to enumerate devices on our platforms. Bisect leads me to this patch, here is dmesg fragment: [ 34.609909] snd_soc_avs:avs_component_probe: avs_hdaudio avs_hdaudio.2: probing hdaudioB0D2-platform card hdaudioB0D2 [ 34.612274] snd_soc_core:soc_tplg_load_header: avs_hdaudio avs_hdaudio.2: ASoC: Got 0x490 bytes of type 8 version 0 vendor 0 at pass 0 [ 34.612456] snd_soc_core:soc_tplg_load_header: avs_hdaudio avs_hdaudio.2: ASoC: Got 0x7ec bytes of type 5 version 0 vendor 0 at pass 3 [ 34.612477] snd_soc_core:soc_tplg_dapm_widget_elems_load: avs_hdaudio avs_hdaudio.2: ASoC: adding 6 DAPM widgets [ 34.612493] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi1_fe id 17 [ 34.612774] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi1_be id 17 [ 34.613025] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi2_fe id 17 [ 34.613297] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi2_be id 17 [ 34.613552] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi3_fe id 17 [ 34.613823] snd_soc_core:soc_tplg_dapm_widget_create: avs_hdaudio avs_hdaudio.2: ASoC: creating DAPM widget hdmi3_be id 17 [ 34.614077] snd_soc_core:soc_tplg_load_header: avs_hdaudio avs_hdaudio.2: ASoC: Got 0xab0 bytes of type 7 version 0 vendor 0 at pass 4 [ 34.614272] snd_soc_core:snd_soc_register_dai: snd_soc_avs 0000:00:0e.0: ASoC: Registered DAI 'HDMI1-dai' [ 34.614290] snd_soc_core:snd_soc_dapm_new_dai_widgets: snd_soc_avs 0000:00:0e.0: ASoC: adding HDMI1-playback widget [ 34.614453] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding HDMI1 [ 34.615192] snd_soc_core:snd_soc_register_dai: snd_soc_avs 0000:00:0e.0: ASoC: Registered DAI 'HDMI2-dai' [ 34.615210] snd_soc_core:snd_soc_dapm_new_dai_widgets: snd_soc_avs 0000:00:0e.0: ASoC: adding HDMI2-playback widget [ 34.615371] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding HDMI2 [ 34.616060] snd_soc_core:snd_soc_register_dai: snd_soc_avs 0000:00:0e.0: ASoC: Registered DAI 'HDMI3-dai' [ 34.616077] snd_soc_core:snd_soc_dapm_new_dai_widgets: snd_soc_avs 0000:00:0e.0: ASoC: adding HDMI3-playback widget [ 34.616235] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding HDMI3 [ 34.616858] snd_soc_core:soc_tplg_pcm_elems_load: avs_hdaudio avs_hdaudio.2: ASoC: adding 3 PCM DAIs [ 34.616876] snd_soc_core:soc_tplg_load_header: avs_hdaudio avs_hdaudio.2: ASoC: Got 0x4a4 bytes of type 4 version 0 vendor 0 at pass 5 [ 34.616895] snd_soc_core:soc_tplg_dapm_graph_elems_load: avs_hdaudio avs_hdaudio.2: ASoC: adding 9 DAPM routes for index 0 [ 34.617601] avs_hdaudio avs_hdaudio.2: ASoC: Parent card not yet available, widget card binding deferred [ 34.618153] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding hdaudioB0D2 link0 [ 34.618724] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding hdaudioB0D2 link1 [ 34.619221] snd_soc_core:snd_soc_add_pcm_runtime: avs_hdaudio avs_hdaudio.2: ASoC: binding hdaudioB0D2 link2 [ 34.619973] probing-LINK: substream (null) has no playback, no capture [ 34.620016] avs_hdaudio avs_hdaudio.2: ASoC: can't create pcm (null) :-22 [ 34.620196] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'hdaudioB0D2-cpu0' [ 34.620309] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'hdaudioB0D2-cpu1' [ 34.620419] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'hdaudioB0D2-cpu2' [ 34.621254] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'HDMI3-dai' [ 34.621837] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'HDMI2-dai' [ 34.622704] snd_soc_core:snd_soc_unregister_dai: snd_soc_avs 0000:00:0e.0: ASoC: Unregistered DAI 'HDMI1-dai' [ 34.623620] snd_soc_core:snd_soc_unregister_dai: snd_hda_codec_hdmi hdaudioB0D2: ASoC: Unregistered DAI 'HDMI 0' [ 34.623695] snd_soc_core:snd_soc_unregister_dai: snd_hda_codec_hdmi hdaudioB0D2: ASoC: Unregistered DAI 'HDMI 1' [ 34.623769] snd_soc_core:snd_soc_unregister_dai: snd_hda_codec_hdmi hdaudioB0D2: ASoC: Unregistered DAI 'HDMI 2' [ 34.624779] snd_hda_core:snd_hdac_display_power: snd_soc_avs 0000:00:0e.0: display power disable [ 34.628057] avs_hdaudio: probe of avs_hdaudio.2 failed with error -22
Hi Amadeusz Thank you for testing > I put the patchset to test and it fails to enumerate devices on our > platforms. > > Bisect leads me to this patch, here is dmesg fragment: (snip) > [ 34.617601] avs_hdaudio avs_hdaudio.2: ASoC: Parent card not yet > available, widget card binding deferred (snip) > [ 34.619973] probing-LINK: substream (null) has no playback, no capture Hmm.. I tested it on my many type of connections, but couldn't reproduce the error... It seems you got [01/20] patch error = no playback, no capture. This means has_playback/capture both flags are 0. It seems you are using soc-topology. Is it topology specific something ? But hmm.. ? static int soc_get_playback_capture(...) { ... (A) if (dai_link->dynamic || dai_link->no_pcm) { ... if (dai_link->dpcm_playback) { ... (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { ... } ... } if (dai_link->dpcm_capture) { ... (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { ... } } ... } } It checks CPU (B) when no_pcm (A) on original. But I think "no_pcm - CPU" is dummy DAI -> above check is no meaning. After the patch, it checks both CPU/Codec. I wonder is your Codec which is connected to no_pcm DAI valid ? Thank you for your help !! Best regards --- Kuninori Morimoto
On 5/22/2023 6:35 AM, Kuninori Morimoto wrote: > > Hi Amadeusz > > Thank you for testing > >> I put the patchset to test and it fails to enumerate devices on our >> platforms. >> >> Bisect leads me to this patch, here is dmesg fragment: > (snip) >> [ 34.617601] avs_hdaudio avs_hdaudio.2: ASoC: Parent card not yet >> available, widget card binding deferred > (snip) >> [ 34.619973] probing-LINK: substream (null) has no playback, no capture > > Hmm.. I tested it on my many type of connections, > but couldn't reproduce the error... > > It seems you got [01/20] patch error = no playback, no capture. > This means has_playback/capture both flags are 0. > > It seems you are using soc-topology. > Is it topology specific something ? > > But hmm.. ? > > static int soc_get_playback_capture(...) > { > ... > (A) if (dai_link->dynamic || dai_link->no_pcm) { > ... > if (dai_link->dpcm_playback) { > ... > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > ... > } > ... > } > if (dai_link->dpcm_capture) { > ... > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > ... > } > } > ... > } > } > > It checks CPU (B) when no_pcm (A) on original. > But I think "no_pcm - CPU" is dummy DAI -> above check is no meaning. > After the patch, it checks both CPU/Codec. no_pcm means that we are describing Back End, if you check any file in sound/soc/intel/avs/boards, they set link->no_pcm to 1 - those files describe card and BE (codec) configuration. Topology in case of our driver describe Front Ends (what is visible when you do "aplay -l"), as well as DSP path. Topology sets link->dynamic to 1 in soc_tplg_fe_link_create(). Do note that card and topology are loaded separately hence the "card binding deferred" message in dmesg above. > I wonder is your Codec which is connected to no_pcm DAI valid ? > I'm not sure what do you mean, if it is valid? It works fine before this patch ;)
Hi Amadeusz > > static int soc_get_playback_capture(...) > > { > > ... > > (A) if (dai_link->dynamic || dai_link->no_pcm) { > > ... > > if (dai_link->dpcm_playback) { > > ... > > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > > ... > > } > > ... > > } > > if (dai_link->dpcm_capture) { > > ... > > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > > ... > > } > > } > > ... > > } > > } > > > > It checks CPU (B) when no_pcm (A) on original. > > But I think "no_pcm - CPU" is dummy DAI -> above check is no meaning. > > After the patch, it checks both CPU/Codec. (snip) > > I wonder is your Codec which is connected to no_pcm DAI valid ? > > I'm not sure what do you mean, if it is valid? It works fine before this > patch ;) Ah, sorry, let me explain detail. static int soc_get_playback_capture(...) { ... (A) if (dai_link->dynamic || dai_link->no_pcm) { int stream; if (dai_link->dpcm_playback) { stream = SNDRV_PCM_STREAM_PLAYBACK; (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { (C) if (snd_soc_dai_stream_valid(cpu_dai, stream)) { has_playback = 1; break; } } ... } Before appling the patch, in DPCM case (A), it checks CPU valid only (B)/(C). In many case, DPCM is like this dynamic no_pcm [CPU/dummy-Codec] - [dummy-CPU/Codec] I noticed that before the patch, it checks dummy DAI only for no_pcm case. But after appling the patch, it will check both CPU and Codec valid (X)/(Y)/(Z). I think this was changed after patch. So, my question was what happen for snd_soc_dai_stream_valid() on your Codec. # I notcied that avs_create_dai_links() is not using dummy CPU on no_pcm case... static int soc_get_playback_capture(...) { ... (X) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { (Y) codec_dai = asoc_rtd_to_codec(rtd, i); /* get paired codec */ (Z) if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && (Z) snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) has_playback = 1; (Z) if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && (Z) snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) has_capture = 1; } ... } Thank you for your help !! Best regards --- Kuninori Morimoto
On 5/23/2023 1:49 AM, Kuninori Morimoto wrote: > > Hi Amadeusz > >>> static int soc_get_playback_capture(...) >>> { >>> ... >>> (A) if (dai_link->dynamic || dai_link->no_pcm) { >>> ... >>> if (dai_link->dpcm_playback) { >>> ... >>> (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { >>> ... >>> } >>> ... >>> } >>> if (dai_link->dpcm_capture) { >>> ... >>> (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { >>> ... >>> } >>> } >>> ... >>> } >>> } >>> >>> It checks CPU (B) when no_pcm (A) on original. >>> But I think "no_pcm - CPU" is dummy DAI -> above check is no meaning. >>> After the patch, it checks both CPU/Codec. > (snip) >>> I wonder is your Codec which is connected to no_pcm DAI valid ? >> >> I'm not sure what do you mean, if it is valid? It works fine before this >> patch ;) > > Ah, sorry, let me explain detail. > > static int soc_get_playback_capture(...) > { > ... > (A) if (dai_link->dynamic || dai_link->no_pcm) { > int stream; > > if (dai_link->dpcm_playback) { > stream = SNDRV_PCM_STREAM_PLAYBACK; > > (B) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > (C) if (snd_soc_dai_stream_valid(cpu_dai, stream)) { > has_playback = 1; > break; > } > } > ... > } > > Before appling the patch, in DPCM case (A), it checks CPU valid only (B)/(C). > In many case, DPCM is like this > > dynamic no_pcm > [CPU/dummy-Codec] - [dummy-CPU/Codec] > > I noticed that before the patch, it checks dummy DAI only for no_pcm case. > But after appling the patch, it will check both CPU and Codec > valid (X)/(Y)/(Z). > > I think this was changed after patch. > So, my question was what happen for snd_soc_dai_stream_valid() on your Codec. > > # I notcied that avs_create_dai_links() is not using dummy CPU on no_pcm case... > > > static int soc_get_playback_capture(...) > { > ... > (X) for_each_rtd_cpu_dais(rtd, i, cpu_dai) { > (Y) codec_dai = asoc_rtd_to_codec(rtd, i); /* get paired codec */ > > (Z) if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && > (Z) snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) > has_playback = 1; > (Z) if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && > (Z) snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) > has_capture = 1; > } > ... > } > > Thank you for your help !! > Hi, sorry for small delay, I've debugged the issue. Seems like one less critical problem is in ASoC HDA codec, which blocks HDA and HDMI probing altogether in our driver, something like this should fix this: commit ed93e4fbc045b8da7906484a9c88e6b53864949b (HEAD) Author: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> Date: Wed May 24 20:54:44 2023 +0200 ASoC: codecs: hda: Fix probe codec definition In order to properly bind snd_soc_dai_driver its snd_soc_pcm_stream need to provide channels_min value, otherwise ASoC framework may think that stream is improper. Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c index d57b043d6bfe..62fd99f530bf 100644 --- a/sound/soc/codecs/hda.c +++ b/sound/soc/codecs/hda.c @@ -341,6 +341,8 @@ static const struct snd_soc_dapm_widget hda_dapm_widgets[] = { static struct snd_soc_dai_driver card_binder_dai = { .id = -1, .name = "codec-probing-DAI", + .capture.channels_min = 1, + .playback.channels_min = 1, }; static int hda_hdev_attach(struct hdac_device *hdev) With the above fixed, there is issue with how HDMI is being initialized and I consider it a blocker. So it needs to be fixed first before this patchset can be merged. I did simple patch like: diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 64a944016c78..e84c3e46557e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2317,6 +2317,7 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec) if (spec->pcm_used >= ARRAY_SIZE(spec->pcm_rec)) break; /* other pstr fields are set in open */ + pstr->channels_min = 1; } return 0; and it works for testing purposes, but as commented in code, those fields are initialized later (in hdmi_pcm_open()), which apparently in case of ASoC binding is too late. Likely those assignments need to be moved earlier. Another thing that should probably be done is some kind of look through ASoC codec files to make sure that they all define channels_min properly, otherwise there may be more unexpected failures. Thanks, Amadeusz
Hi Amadeusz Cc Mark Thank you for debuging/checking. > > I noticed that before the patch, it checks dummy DAI only for no_pcm case. > > But after appling the patch, it will check both CPU and Codec > > valid (X)/(Y)/(Z). (snip) > sorry for small delay, I've debugged the issue. Seems like one less > critical problem is in ASoC HDA codec, which blocks HDA and HDMI probing > altogether in our driver, something like this should fix this: (snip) > @@ -341,6 +341,8 @@ static const struct snd_soc_dapm_widget > hda_dapm_widgets[] = { > static struct snd_soc_dai_driver card_binder_dai = { > .id = -1, > .name = "codec-probing-DAI", > + .capture.channels_min = 1, > + .playback.channels_min = 1, > }; (snip) > and it works for testing purposes, but as commented in code, those > fields are initialized later (in hdmi_pcm_open()), which apparently in > case of ASoC binding is too late. Likely those assignments need to be > moved earlier. > > Another thing that should probably be done is some kind of look through > ASoC codec files to make sure that they all define channels_min > properly, otherwise there may be more unexpected failures. Hmm... >From logic point of view, I think we need to check all validation. But from compatible point of view, we want to skip validation check for BE Codec... Especially, quick hack solution (= adding channels_min in many place) might case other bug I guess. OK, let's skip BE Codec in v2 for now. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index af5d4e1effdf..f47ddf660c57 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2732,7 +2732,10 @@ 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 *codec_dai; struct snd_soc_dai *cpu_dai; + int cpu_capture = SNDRV_PCM_STREAM_CAPTURE; + int cpu_playback = SNDRV_PCM_STREAM_PLAYBACK; int tmp_playback = 0; int tmp_capture = 0; int i; @@ -2748,61 +2751,27 @@ 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; + /* Adapt stream for codec2codec links */ + if (dai_link->c2c_params) { + cpu_capture = SNDRV_PCM_STREAM_PLAYBACK; + cpu_playback = SNDRV_PCM_STREAM_CAPTURE; + } - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - tmp_playback = 1; - break; - } - } - if (!tmp_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 */ + 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; - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - tmp_capture = 1; - break; - } - } - - if (!tmp_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 *codec_dai; - - /* Adapt stream for codec2codec links */ - int cpu_capture = dai_link->c2c_params ? - SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE; - int cpu_playback = dai_link->c2c_params ? - SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; - - for_each_rtd_codec_dais(rtd, i, codec_dai) { - cpu_dai = asoc_rtd_to_cpu(rtd, i); - - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && - snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - tmp_playback = 1; - if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && - snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - tmp_capture = 1; - } + for_each_rtd_cpu_dais(rtd, i, cpu_dai) { + codec_dai = asoc_rtd_to_codec(rtd, i); /* get paired codec */ + + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && + snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) + tmp_playback = 1; + if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && + snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) + tmp_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_codec_dais(rtd, i, codec_dai) { |(*) ... 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; This patch enables both flags case, but dpcm_playback/capture flags will be removed if all driver were switched to new playback/capture_only flags. Here, CPU <-> Codec relationship is like this DPCM [CPU/dummy]-[dummy/Codec] ^^^^^^^^^^^ Normal [CPU/Codec] ^^^^^^^^^^^ (X) part is checking only CPU DAI, and (Y) part is checking both CPU/Codec DAI This means (X)/(Y) are checking same position. Because dammy DAI is always available, we can share same code for all cases (= X/Y/Z). This patch merge these. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- sound/soc/soc-pcm.c | 75 +++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 53 deletions(-)