ASoC: soc-compress: Inherit atomicity from DAI link for Compress FE

Commit Message

Daniel Baluta (OSS) March 14, 2023, 3:34 p.m. UTC

From: Daniel Baluta <[e-mail]>

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
[   36.444278]  PCM Deep Buffer: ASoC: can't connect SAI1.OUT

In order to fix this we must inherit the atomicity from DAI link
associated with current PCM Compress FE.

Fixes: bbf7d3b1c4f4 ("ASoC: soc-pcm: align BE 'atomicity' with that of the FE")
Signed-off-by: Daniel Baluta <[e-mail]>
---
 sound/soc/soc-compress.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Pierre-Louis Bossart March 14, 2023, 4:14 p.m. UTC | #1

On 3/14/23 10:34, Daniel Baluta wrote:
> From: Daniel Baluta <[e-mail]>
> 
> 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.

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?

> [   36.444278]  PCM Deep Buffer: ASoC: can't connect SAI1.OUT
> 
> In order to fix this we must inherit the atomicity from DAI link
> associated with current PCM Compress FE.
> 
> Fixes: bbf7d3b1c4f4 ("ASoC: soc-pcm: align BE 'atomicity' with that of the FE")
> Signed-off-by: Daniel Baluta <[e-mail]>
> ---
>  sound/soc/soc-compress.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index e7aa6f360cab..d649b0cf4744 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -622,6 +622,9 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  			return ret;
>  		}
>  
> +		/* inherit atomicity from DAI link */
> +		be_pcm->nonatomic = rtd->dai_link->nonatomic;
> +
>  		rtd->pcm = be_pcm;
>  		rtd->fe_compr = 1;
>  		if (rtd->dai_link->dpcm_playback)

Daniel Baluta March 14, 2023, 4:37 p.m. UTC | #2

On Tue, Mar 14, 2023 at 6:14 PM Pierre-Louis Bossart
<[e-mail]> wrote:
>
>
>
> On 3/14/23 10:34, Daniel Baluta wrote:
> > From: Daniel Baluta <[e-mail]>
> >
> > 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.

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;
»       }

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.

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.


thanks,
Daniel.

Pierre-Louis Bossart March 14, 2023, 4:52 p.m. UTC | #3

On 3/14/23 11:37, Daniel Baluta wrote:
> On Tue, Mar 14, 2023 at 6:14 PM Pierre-Louis Bossart
> <[e-mail]> wrote:
>>
>>
>>
>> On 3/14/23 10:34, Daniel Baluta wrote:
>>> From: Daniel Baluta <[e-mail]>
>>>
>>> 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.

> 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.

Daniel Baluta March 16, 2023, 6:58 a.m. UTC | #4

On Tue, Mar 14, 2023 at 6:52 PM Pierre-Louis Bossart
<[e-mail]> wrote:
>
>
>
> On 3/14/23 11:37, Daniel Baluta wrote:
> > On Tue, Mar 14, 2023 at 6:14 PM Pierre-Louis Bossart
> > <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 3/14/23 10:34, Daniel Baluta wrote:
> >>> From: Daniel Baluta <[e-mail]>
> >>>
> >>> 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.

Patch

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index e7aa6f360cab..d649b0cf4744 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -622,6 +622,9 @@  int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 			return ret;
 		}
 
+		/* inherit atomicity from DAI link */
+		be_pcm->nonatomic = rtd->dai_link->nonatomic;
+
 		rtd->pcm = be_pcm;
 		rtd->fe_compr = 1;
 		if (rtd->dai_link->dpcm_playback)