DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[srv_kaichieh.chaung]

Hi,
      Let me illustrate the system first.
FE1 -----+----> BE
         |
FE2 -----+

      When there is multi-FE stream to a single BE, the BE will receive
SND_SOC_DAPM_STREAM_STOP if one of the FE shutdown.
However, the BE is stilled used by another, this will let BE cpu_dai and
codec_dai think they are inactive. The later power check will go wrong.

I have check the latest linux version, and didn't find any related
change.
The following is my change to address this, and will re-send a formal
patch if it's ok.


diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 70e8088..9c2d159 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -163,6 +163,13 @@ int dpcm_dapm_stream_event(struct
snd_soc_pcm_runtime *fe, int dir,
 		dev_dbg(be->dev, "ASoC: BE %s event %d dir %d\n",
 				be->dai_link->name, event, dir);

+		/* don't sent stop event if this BE is used by other FE */
+		if (event == SND_SOC_DAPM_STREAM_STOP &&
+		    be->dpcm[dir].users >= 1) {
+			pr_warn("kc, %s(), be->dai_link->name %s skip stop event\n",
__func__, be->dai_link->name);
+			continue;
+		}
+
 		snd_soc_dapm_stream_event(be, dir, event);
 	}
 

Sincerely,
Kai Chieh

DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Chuang Eric]

Hi,

      Let me
illustrate the system first.

FE1 -----+----> BE

         |

FE2 -----+

 

      When there is
multi-FE stream to a single BE, the BE will receive SND_SOC_DAPM_STREAM_STOP if
one of the FE shutdown.

However, the BE is stilled used by another, this will let
BE cpu_dai and codec_dai think they are inactive. The later power check will go
wrong.

 

I have check the latest linux version, and didn't find
any related change.

The following is my change to address this, and will
re-send a formal patch if it's ok.

 

 

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 70e8088..9c2d159 100644

--- a/sound/soc/soc-pcm.c

+++ b/sound/soc/soc-pcm.c

@@ -163,6 +163,13 @@ int dpcm_dapm_stream_event(struct
snd_soc_pcm_runtime *fe, int dir,

            dev_dbg(be->dev, "ASoC: BE %s
event %d dir %d\n",

                        be->dai_link->name,
event, dir);

 

+           /* don't
sent stop event if this BE is used by other FE */

+           if
(event == SND_SOC_DAPM_STREAM_STOP &&

+               be->dpcm[dir].users >= 1) {

+                 pr_warn("kc,
%s(), be->dai_link->name %s skip stop event\n",

__func__, be->dai_link->name);

+                 continue;

+           }

+

            snd_soc_dapm_stream_event(be, dir,
event);

      }

 

 

Sincerely,

Kai Chieh 		 	   		  

DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Kai Chieh Chuang]

Hi,
      Let me illustrate the system first.
FE1 -----+----> BE
         |
FE2 -----+

      When there is multi-FE stream to a single BE, the BE will receive
SND_SOC_DAPM_STREAM_STOP if one of the FE shutdown.
However, the BE is stilled used by another, this will let BE cpu_dai and
codec_dai think they are inactive. The later power check will go wrong.

I have check the latest linux version, and didn't find any related
change.
The following is my change to address this, and will re-send a formal
patch if it's ok.


diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 70e8088..9c2d159 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -163,6 +163,13 @@ int dpcm_dapm_stream_event(struct
snd_soc_pcm_runtime *fe, int dir,
 		dev_dbg(be->dev, "ASoC: BE %s event %d dir %d\n",
 				be->dai_link->name, event, dir);

+		/* don't sent stop event if this BE is used by other FE */
+		if (event == SND_SOC_DAPM_STREAM_STOP &&
+		    be->dpcm[dir].users >= 1) {
+			pr_warn("kc, %s(), be->dai_link->name %s skip stop event\n",
__func__, be->dai_link->name);
+			continue;
+		}
+
 		snd_soc_dapm_stream_event(be, dir, event);
 	}
 

Sincerely,
Kai Chieh

Re: DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]

On Tue, May 31, 2016 at 01:45:32PM +0800, Kai Chieh Chuang wrote:

> +		/* don't sent stop event if this BE is used by other FE */
> +		if (event == SND_SOC_DAPM_STREAM_STOP &&
> +		    be->dpcm[dir].users >= 1) {
> +			pr_warn("kc, %s(), be->dai_link->name %s skip stop event\n",
> __func__, be->dai_link->name);
> +			continue;
> +		}
> +
>  		snd_soc_dapm_stream_event(be, dir, event);
>  	}

If this isn't happening that seems like a bug, I'm a bit surprised
nobody else ran into it?  Shouldn't the counts of stream events that
happen be symmetric (ie, we get as many stops as starts), or are we
possibly missing some from things being switched in and out or something?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Kai Chieh Chuang]

> If this isn't happening that seems like a bug, I'm a bit surprised
> nobody else ran into it?  Shouldn't the counts of stream events that
> happen be symmetric (ie, we get as many stops as starts), or are we
> possibly missing some from things being switched in and out or
something?

This idea is same as the DPCM BE state control, in

int dpcm_be_dai_hw_free(..)
{
......
                /* do not free hw if this BE is used by other FE */
                if (be->dpcm[stream].users > 1)
                        continue;

and

int dpcm_be_dai_shutdown(...)
{
....
                if (--be->dpcm[stream].users != 0)
                        continue;

The BE receiving SND_SOC_DAPM_STREAM_STOP event, while still used by a
FE who is in STREAM_START state. The DAPM will just inactive BE related
widget/dai without checking. This just doesn't seems right.

And the last FE using the BE calls in to dpcm_fe_dai_shutdown(), will
run the process normally.


Actually, i trace the log, when one of the FE sent stop event to "still
in use BE", the BE power state will not be changed. Until the system
goes into
SNDRV_CTL_POWER_D3cold, the BE will be shutdown (i did set the
ignore_suspend=1). I didn't check why the BE is not turned off at the
point when receiving the stop event.

I'm also curious why no one seems to have the same issue. Probably there
is not much DPCM architecture system yet on the Linux?

Re: DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 535 bytes --]

On Fri, Jun 24, 2016 at 02:21:34PM +0800, KaiChieh Chuang wrote:

> Is there any comment on this?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  Sending content
free pings just adds to the mail volume (if they are seen at all) and if 
something has gone wrong you'll have to resend the patches anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 645 bytes --]

On Sun, Jun 05, 2016 at 02:58:59PM +0800, Kai Chieh Chuang wrote:

> The BE receiving SND_SOC_DAPM_STREAM_STOP event, while still used by a
> FE who is in STREAM_START state. The DAPM will just inactive BE related
> widget/dai without checking. This just doesn't seems right.

Why not?  The backend is still active and running, why would it expect
to be stopped unless it's supposed to actually stop?

> Actually, i trace the log, when one of the FE sent stop event to "still
> in use BE", the BE power state will not be changed. Until the system

Sure, until all the users stop using it it should stay running.  What
would you expect it to do?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: DPCM: skip DAPM_STREAM_STOP event to BE, if still used by other FE

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 909 bytes --]

On Mon, Jul 04, 2016 at 10:26:54AM +0800, KaiChieh Chuang wrote:
> On Thu, 2016-06-30 at 03:08 +0800, Mark Brown wrote:
> > On Sun, Jun 05, 2016 at 02:58:59PM +0800, Kai Chieh Chuang wrote:

> > > The BE receiving SND_SOC_DAPM_STREAM_STOP event, while still used by a
> > > FE who is in STREAM_START state. The DAPM will just inactive BE related
> > > widget/dai without checking. This just doesn't seems right.

> > Why not?  The backend is still active and running, why would it expect
> > to be stopped unless it's supposed to actually stop?

> Yes, I don't expect it to stop.
> But at this situation, the BE change to state STREAM_STOP while
> still used by a FE in STREAM_START state.

> Shouldn't the BE stay in the STREAM_START state?

Yes, but that sounded like the situation you were describing?  It's
really hard to follow what you're trying to say here, perhaps just send
the fix you're proposing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]