On Tue, Mar 14, 2023 at 6:52 PM Pierre-Louis Bossart
<pierre-louis.bossart@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 3/14/23 11:37, Daniel Baluta wrote:
> > On Tue, Mar 14, 2023 at 6:14 PM Pierre-Louis Bossart
> > <pierre-louis.bossart@xxxxxxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 3/14/23 10:34, Daniel Baluta wrote:
> >>> From: Daniel Baluta <daniel.baluta@xxxxxxx>
> >>>
> >>> After commit bbf7d3b1c4f40 ("ASoC: soc-pcm: align BE 'atomicity' with
> >>> that of the FE") BE and FE atomicity must match.
> >>>
> >>> In the case of Compress PCM there is a mismatch in atomicity between FE
> >>> and BE and we get errors like this:
> >>>
> >>> [ 36.434566] sai1-wm8960-hifi: dpcm_be_connect: FE is atomic but BE
> >>> is nonatomic, invalid configuration
> >>
> >> Not clear on the 'FE is atomic' in the case of a compressed stream,
> >> which has to be handled with some sort of IPC, i.e. be nonatomic.
> >>
> >
> > 'FE is atomic' in this message is printed because this is the default value
> > of nonatomic field when PCM struct associated for a Compress PCM
> > struct is allocated.
> >
> > No one changes 'nonatomic' field for Compress FE until my current patch.
> >
> >> Also not sure why the FE is not set as nonatomic by the SOF parts?
> >> If it's needed for PCM, why wouldn't it be needed for compressed data?
> >
> > FE is not touched for SOF parts. Only BE is set to nonatomic by SOF.
>
> Where do you see the BE being changed by SOF?
>
> >
> > See: sound/soc/topology.c
> >
> > » /* Set nonatomic property for FE dai links as their trigger
> > action involves IPC's */
> > » if (!link->no_pcm) {
> > » » link->nonatomic = true;
> > » » return 0;
> > » }
>
> that's a FE property, not BE.
You are right.
>
> > FE for PCM is modified by sound/soc/soc-pcm.c
> >
> > int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> > » pcm->nonatomic = rtd->dai_link->nonatomic;
> >
> > So, I guess people assumed that is enough to use RTD dai link to set
> > pcm->noatomic field
> > and didn't look at it in SOF.
>
> Ah yes, now I see your point now. You still had a logical inversion
> above but you're correct here.
>
> > When FE for Compress PCM is created, we don't use soc_new_pcm but instead
> > we use snd_pcm_new_internal which doesn't inherit the nonatomic field
> > of the rtd->dai_link
> > as Normal PCM does inside soc_pcm_new.
> >
> > So, my patch makes sure we inherit the nonatomic field from
> > rtd->dai_link also for Compress PCM
> > similar with what already happens for Normal PCM.
> >
> > tl;dr: when creating a Normal PCM pcm->nonatomic is inherited from RTD
> > DAI link. when creating a
> > Compress PCM pcm->nonatomic field is not set. This patch makes sure
> > that for Compres PCM
> > we also inherit nonatomic from RTD DAI link.
>
> That makes sense. It's quite likely that the compress PCM should be
> nonatomic by default, not sure how it can work otherwise.
To sum up:
- we need to merge current patch because Compress PCM needs to
inherit the atomicity from FE DAI
Because SOF FE DAI links are made to be nonatomic:
sound/soc/sof/topology.c
» /* Set nonatomic property for FE dai links as their trigger
action involves IPC's */
» if (!link->no_pcm) {
» » link->nonatomic = true;
» » return 0;
» }
and with my patch:
sound/soc/soc-compress.c
+ /* inherit atomicity from DAI link */
+ be_pcm->nonatomic = rtd->dai_link->nonatomic;
+
rtd->pcm = be_pcm;
... then Compres PCM will be nonatomic.
Side note: I think be_pcm from the patch above should be called fe_pcm
instead. But that's a story for another patch.
thanks,
Daniel.
