On Mon, Feb 17, 2020 at 06:12:53PM +0100, Stephan Gerhold wrote:
> On Mon, Feb 17, 2020 at 03:43:01PM +0000, Mark Brown wrote:
> > On Mon, Feb 17, 2020 at 03:41:20PM +0100, Stephan Gerhold wrote:
> >
> > > I'm a bit confused about this patch, isn't SNDRV_PCM_STREAM_PLAYBACK
> > > used for both cpu_dai and codec_dai in the playback case?
> >
> > It is in the normal case, but with a CODEC<->CODEC link (which was what
> > this was targeting) we need to bodge things by swapping playback and
> > capture on one end of the link.
>
> I see. Looking at the code again I'm guessing the cause of the crash
> "fixed" by this patch is commit a342031cdd08 ("ASoC: create pcm for
> codec2codec links as well") where the codec2codec case was sort of
> patched in. This is what we had before this patch:
>
> /* Adapt stream for codec2codec links */
> struct snd_soc_pcm_stream *cpu_capture = rtd->dai_link->params ?
> &cpu_dai->driver->playback : &cpu_dai->driver->capture;
> struct snd_soc_pcm_stream *cpu_playback = rtd->dai_link->params ?
> &cpu_dai->driver->capture : &cpu_dai->driver->playback;
>
> This does the swapping you mentioned, so I guess rtd->dai_link->params
> is only set for the codec2codec case?
>
> for_each_rtd_codec_dai(rtd, i, codec_dai) {
> if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK))
> playback = 1;
> if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE))
> capture = 1;
> }
>
> capture = capture && cpu_capture->channels_min;
> playback = playback && cpu_playback->channels_min;
>
> And this does a part of the check in snd_soc_dai_stream_valid(),
> but without the NULL check of cpu_capture/cpu_playback.
> (Maybe that is the cause of the crash.)
Uh, no, I am completely wrong here. :)
cpu_capture/cpu_playback cannot actually be NULL...
I should have looked more carefully at snd_soc_dai_stream_valid()...
But I still wonder if the approach below would be easier?
>
> From my limited understanding, I would say that a much simpler way to
> implement this would be:
>
> /* Adapt stream for codec2codec links */
> int cpu_capture = rtd->dai_link->params ?
> SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
> int cpu_playback = rtd->dai_link->params ?
> SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
>
> for_each_rtd_codec_dai(rtd, i, codec_dai) {
> if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
> snd_soc_dai_stream_valid(cpu_dai, cpu_playback))
> playback = 1;
> if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
> snd_soc_dai_stream_valid(cpu_dai, cpu_capture))
> capture = 1;
> }
>
> since snd_soc_dai_stream_valid() does both the NULL-check and the
> "channels_min" check.
>
> But I'm really not familar with the codec2codec case and am unable to
> test it :) What do you think?
>
> Thanks,
> Stephan
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
