ASoC: SOF: Intel: hda-dai: cpu/codec/dai/dailink confusion #3128
Conversation
ujfalusi
left a comment
ujfalusi
left a comment
There was a problem hiding this comment.
I'm not un-confused even after this series... I have used to systems where the DAI / platform are mostly independent, just swap the platform driver servicing the DAI driver and it juts works. The card (dai link) driver for sure has to have knowledge on how to link components together.
It all boils down to HDA treating all components as interdependent part of the same pipe, link, stream.
Making the code less confusing is not easy for sure.
| struct snd_soc_dapm_widget *w; | ||
| int ret; | ||
|
|
||
| ret = hda_dai_link_hw_free(substream); |
There was a problem hiding this comment.
The order how the free was done also changes, before the hda_dai_widget_update(dai->dev, w, DMA_CHAN_INVALID, false); was done before what it is now under hda_dai_link_hw_free()
Is there any dependency between the two?
For me the operation done under hda_dai_link_hw_free() is more like platform driver like (DMA free) which might come from the way HDA is architectured..
There was a problem hiding this comment.
I don't think there's any relationship between dealing with the widget and dealing with the stream tag, but that's precisely what I wanted to poke at.
We have a helper, use it to simplify widget lookup Suggested-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
TRIGGER_RESUME is not supported on Intel platforms, let's remove untested/dead code. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
… DMA The Intel documentation refers to the concepts of 'HDAudio host DMA' (system memory <--> DSP) and 'HDaudio link DMA' (DSP <--> peripherals). We currently use the prefix 'hda_link' to describe DAI operations, which can be confused for dailink operations. Since the topology tokens refer unambiguously to the 'HDA' dai, let's drop the link prefix for dai-related ops/callbacks. Conversely let's use 'hda_link_dma' for routines related to the DMA management. In a follow-up patch we will introduce the 'hda_dai_link' prefix for dailink ops/callbacks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
hda/cpu-codec-dai-dailink-confusion
branch
from
b0daf48 to
6778327
Compare
We don't need to pass a hda_stream to find the sdev to find a device for dev_dbg. Just pass the device. While we're at it, also update error log Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
First split the dailink and dai operations in separate functions, that will still be called from the dai callbacks. In a follow-up step these dailink routines will be moved and invoked from the dailink callbacks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
hda/cpu-codec-dai-dailink-confusion
branch
from
6778327 to
17377ac
Compare
| snd_soc_dai_set_dma_data(dai, substream, NULL); | ||
| snd_soc_dai_set_dma_data(cpu_dai, substream, NULL); | ||
| snd_hdac_ext_stream_release(link_dev, HDAC_EXT_STREAM_TYPE_LINK); | ||
| link_dev->link_prepared = 0; |
There was a problem hiding this comment.
@plbossart the change looks good but to be truly balanced in terms of splitting DAI ops, DAI link pos and DAI link DMA ops, shouldnt the above 3 lines be called from link_dma_free() ?
There was a problem hiding this comment.
@ranj063 did you mean introducing a new function called hda_link_dma_free()?
If yes, I find it quite hard to identify what is related to the dailink and what is related to DMA proper.
There was a problem hiding this comment.
hmm yes, it is a hard split but the converse of the above are in dma_params() so maybe these can bein the new function
Traces captured on UpExtreme: root@plb-UP-WHL01:~# speaker-test -Dhw:0,0 -c2 -r48000 -t sine -l1 The BE fixup is called three times, one in the BE hw_params, once for the codec dai and once for the cpu dai. WHY? [ 23.948549] Analog Playback and Capture: dpcm_be_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948567] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948574] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948578] Analog Playback and Capture: dpcm_be_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948587] snd_hda_codec_realtek ehdaudio0D0: snd_soc_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948593] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948598] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948602] snd_hda_codec_realtek ehdaudio0D0: snd_soc_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948607] snd_hda_codec_realtek ehdaudio0D0: plb: hdac_hda_dai_hw_params: start dai Analog Codec DAI [ 23.948614] snd_hda_codec_realtek ehdaudio0D0: plb: hdac_hda_dai_hw_params: done Analog Codec DAI [ 23.948622] sof-audio-pci-intel-cnl 0000:00:1f.3: snd_soc_dai_hw_params: plb before snd_soc_link_be_hw_params_fixup [ 23.948627] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: start [ 23.948631] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: sof_pcm_dai_link_fixup: done [ 23.948636] sof-audio-pci-intel-cnl 0000:00:1f.3: snd_soc_dai_hw_params: plb after snd_soc_link_be_hw_params_fixup [ 23.948640] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: hda_dai_hw_params: start dai Analog CPU DAI [ 23.949507] sof-audio-pci-intel-cnl 0000:00:1f.3: plb: hda_dai_hw_params: done dai Analog CPU DAI Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code handles cpu and codec dais inconsistently, sometimes codec-first and sometimes cpu-first. The most puzzling case is hw_params, where the codec dais are handled first, in a reversal of the DAPM graph order. This order is particularly problematic for SoundWire-based solutions, where the master runtime is allocated by the Slave driver, a clear layering violation to work-around the order imposed by soc-pcm.c. This patch enforces a strict cpu-dai first behavior. There is no change besides moving code blocks around. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
For HDaudio we store the stream_tag as the tx_mask for the codec_dai. When the cpu_dai hw_params is done, we set the tdm mask as the stream tag. But since the codec_dai hw_params is done first, this does not interfere with codec_mask_fixup. When we revert and do the cpu_dai hw_params first, then we end-up using the stream tag in soc_pcm_codec_params_fixup(). This makes of course no sense, we end-up using garbage channel masks and there's no audio. The following trages were captured on TGL, one can clearly see that we use a linear set of tags, but we treat them as masks. That's just wrong. We need to stop overloading definitions and use a real stream_tag, as done for SoundWire. [ 17.792152] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 1, ignored [ 17.793842] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 2, ignored [ 17.795153] snd_hda_codec_hdmi ehdaudio0D2: soc_pcm_hw_params: plb: stream tag is 3, ignored [ 17.819472] snd_hda_codec_realtek ehdaudio0D0: soc_pcm_hw_params: plb: stream tag is 4, ignored [ 39.037775] snd_hda_codec_realtek ehdaudio0D0: soc_pcm_hw_params: plb: stream tag is 1, ignored Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
| if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) | ||
| continue; | ||
|
|
||
| ret = snd_soc_dai_hw_params(cpu_dai, substream, params); |
There was a problem hiding this comment.
snd_soc_dai_hw_params will call snd_soc_link_be_hw_params_fixup to change the params.
In original sequence, codec_params will use default params. With this commit, it will use the params fixed up by cpu_dai. This may be a risk.
/* copy params for each codec */
codec_params = *params;
Or we can copy this method and do it like " cpu_params = *params", then use cpu_params
There was a problem hiding this comment.
I removed the order change in my last version, not worth the effort.
|
closing, replaced mostly by #3146 |
4 participants
After SSP and SoundWire, my bad karma pushed me to revisit the HDaudio dai support, which is just an awful bag of confused legacy.
The main goal of this PR is to split dailink and dai operations, so that we can avoid dependencies on the ASoC-DPCM order in which cpu and codec dais are handled.
Compiled tested and sine-tone smoke test only for now. Let's see what I broke.