Re: [PATCH 08/20] ASoC: soc-pcm.c: cleanup soc_get_playback_capture()

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

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