[Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Robert Lee]

When compress offload pauses in draining state, not all platforms
need to keep in draining state. Some platforms may call drain or
partial drain again when resume from pause in draining, so it needs
to wake up from snd_compress_wait_for_drain() in this case.

Call API snd_compr_leave_draining_in_pause(), if the platform
doesn't need to keep in draining state when pause in draining
state.

Signed-off-by: Robert Lee <lerobert@google.com>
---
 include/sound/compress_driver.h | 14 ++++++++++++++
 sound/core/compress_offload.c   |  8 +++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 277087f635f3..e16524a93a14 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -145,6 +145,7 @@ struct snd_compr_ops {
  * @lock: device lock
  * @device: device id
  * @use_pause_in_draining: allow pause in draining, true when set
+ * @leave_draining_in_pause: leave draining state when pausing in draining
  */
 struct snd_compr {
 	const char *name;
@@ -156,6 +157,7 @@ struct snd_compr {
 	struct mutex lock;
 	int device;
 	bool use_pause_in_draining;
+	bool leave_draining_in_pause;
 #ifdef CONFIG_SND_VERBOSE_PROCFS
 	/* private: */
 	char id[64];
@@ -182,6 +184,18 @@ static inline void snd_compr_use_pause_in_draining(struct snd_compr_stream *subs
 	substream->device->use_pause_in_draining = true;
 }
 
+/**
+ * snd_compr_leave_draining_in_pause - Leave draining state when pause in draining
+ * @substream: compress substream to set
+ *
+ * In some platform, we need to leave draining state when we use pause in draining.
+ * Add API to allow leave draining state.
+ */
+static inline void snd_compr_leave_draining_in_pause(struct snd_compr_stream *substream)
+{
+	substream->device->leave_draining_in_pause = true;
+}
+
 /* dsp driver callback apis
  * For playback: driver should call snd_compress_fragment_elapsed() to let the
  * framework know that a fragment has been consumed from the ring buffer
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 21ce4c056a92..c6e5c8f072d7 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -719,8 +719,14 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 		if (!stream->device->use_pause_in_draining)
 			return -EPERM;
 		retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-		if (!retval)
+		if (!retval) {
+			if (stream->device->leave_draining_in_pause) {
+				stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+				wake_up(&stream->runtime->sleep);
+				break;
+			}
 			stream->pause_in_draining = true;
+		}
 		break;
 	default:
 		return -EPERM;
-- 
2.32.0.93.g670b81a890-goog

Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Takashi Iwai]

On Thu, 08 Jul 2021 04:08:15 +0200,
Robert Lee wrote:
> 
> When compress offload pauses in draining state, not all platforms
> need to keep in draining state. Some platforms may call drain or
> partial drain again when resume from pause in draining, so it needs
> to wake up from snd_compress_wait_for_drain() in this case.
> 
> Call API snd_compr_leave_draining_in_pause(), if the platform
> doesn't need to keep in draining state when pause in draining
> state.
> 
> Signed-off-by: Robert Lee <lerobert@google.com>
> ---
>  include/sound/compress_driver.h | 14 ++++++++++++++
>  sound/core/compress_offload.c   |  8 +++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 277087f635f3..e16524a93a14 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -145,6 +145,7 @@ struct snd_compr_ops {
>   * @lock: device lock
>   * @device: device id
>   * @use_pause_in_draining: allow pause in draining, true when set
> + * @leave_draining_in_pause: leave draining state when pausing in draining
>   */
>  struct snd_compr {
>  	const char *name;
> @@ -156,6 +157,7 @@ struct snd_compr {
>  	struct mutex lock;
>  	int device;
>  	bool use_pause_in_draining;
> +	bool leave_draining_in_pause;
>  #ifdef CONFIG_SND_VERBOSE_PROCFS
>  	/* private: */
>  	char id[64];
> @@ -182,6 +184,18 @@ static inline void snd_compr_use_pause_in_draining(struct snd_compr_stream *subs
>  	substream->device->use_pause_in_draining = true;
>  }
>  
> +/**
> + * snd_compr_leave_draining_in_pause - Leave draining state when pause in draining
> + * @substream: compress substream to set
> + *
> + * In some platform, we need to leave draining state when we use pause in draining.
> + * Add API to allow leave draining state.
> + */
> +static inline void snd_compr_leave_draining_in_pause(struct snd_compr_stream *substream)
> +{
> +	substream->device->leave_draining_in_pause = true;
> +}
> +
>  /* dsp driver callback apis
>   * For playback: driver should call snd_compress_fragment_elapsed() to let the
>   * framework know that a fragment has been consumed from the ring buffer
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 21ce4c056a92..c6e5c8f072d7 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -719,8 +719,14 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>  		if (!stream->device->use_pause_in_draining)
>  			return -EPERM;
>  		retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> -		if (!retval)
> +		if (!retval) {
> +			if (stream->device->leave_draining_in_pause) {
> +				stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +				wake_up(&stream->runtime->sleep);
> +				break;
> +			}
>  			stream->pause_in_draining = true;
> +		}

Hrm, what actually happens with this new flag?  It changes the state
to PAUSED even if it's done during the draining.  Then user resumes
the pause via snd_compr_resume(), and now the state changes to
RUNNING.  OTOH, if the draining runs normally, it'll end up with
SETUP.

Even if the above is even designed behavior, it must be described
properly somewhere.  The state change is described in snd_compr_open()
comment, and the new behavior should be mentioned there as well.
(Admittedly, the previous hack for the pause-during-drain is also
missing and should have been mentioned there; but an excuse is that
the pause-during-drain doesn't change the state itself :)


thanks,

Takashi

Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Robert Lee]

Hi Takashi,

It is a little complex to describe the design in detail, but try to
explain simply
what issue we meet.

If w/o the change,  after user resumes from the pause, our system would call
snd_compr_drain() or snd_compr_partial_drain() again after it returns from
previous drain (when EOF reaches). Then it will block in this drain and no one
wake it up because EOF has already reached. I add this change to return from
the previous drain.

And yes, after user resumes it, it will change state to RUNNING. Then it will
call snd_compr_drain() or snd_compr_partial_drain() very soon and change
state to DRAINING again.

Actually, I am wondering how the pause-during-drain can keep the state in
DRAINING. It should have a different design. :)

I also checked the snd_compr_open() comment, and it doesn't mention that
we cannot pause in DRAINING state. Looks like it needs to be updated according
to these changes. Maybe it can be updated in another commit?

* SNDRV_PCM_STATE_DRAINING: When stream is draining current data. This is done
 *      by calling SNDRV_COMPRESS_DRAIN.
 * SNDRV_PCM_STATE_PAUSED: When stream is paused. This is done by calling
 *      SNDRV_COMPRESS_PAUSE. It can be stopped or resumed by calling
 *      SNDRV_COMPRESS_STOP or SNDRV_COMPRESS_RESUME respectively.

thanks,
Robert.

Takashi Iwai <tiwai@suse.de> 於 2021年7月8日 週四 下午7:24寫道：


Takashi Iwai <tiwai@suse.de> 於 2021年7月8日 週四 下午7:24寫道：
>
> On Thu, 08 Jul 2021 04:08:15 +0200,
> Robert Lee wrote:
> >
> > When compress offload pauses in draining state, not all platforms
> > need to keep in draining state. Some platforms may call drain or
> > partial drain again when resume from pause in draining, so it needs
> > to wake up from snd_compress_wait_for_drain() in this case.
> >
> > Call API snd_compr_leave_draining_in_pause(), if the platform
> > doesn't need to keep in draining state when pause in draining
> > state.
> >
> > Signed-off-by: Robert Lee <lerobert@google.com>
> > ---
> >  include/sound/compress_driver.h | 14 ++++++++++++++
> >  sound/core/compress_offload.c   |  8 +++++++-
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > index 277087f635f3..e16524a93a14 100644
> > --- a/include/sound/compress_driver.h
> > +++ b/include/sound/compress_driver.h
> > @@ -145,6 +145,7 @@ struct snd_compr_ops {
> >   * @lock: device lock
> >   * @device: device id
> >   * @use_pause_in_draining: allow pause in draining, true when set
> > + * @leave_draining_in_pause: leave draining state when pausing in draining
> >   */
> >  struct snd_compr {
> >       const char *name;
> > @@ -156,6 +157,7 @@ struct snd_compr {
> >       struct mutex lock;
> >       int device;
> >       bool use_pause_in_draining;
> > +     bool leave_draining_in_pause;
> >  #ifdef CONFIG_SND_VERBOSE_PROCFS
> >       /* private: */
> >       char id[64];
> > @@ -182,6 +184,18 @@ static inline void snd_compr_use_pause_in_draining(struct snd_compr_stream *subs
> >       substream->device->use_pause_in_draining = true;
> >  }
> >
> > +/**
> > + * snd_compr_leave_draining_in_pause - Leave draining state when pause in draining
> > + * @substream: compress substream to set
> > + *
> > + * In some platform, we need to leave draining state when we use pause in draining.
> > + * Add API to allow leave draining state.
> > + */
> > +static inline void snd_compr_leave_draining_in_pause(struct snd_compr_stream *substream)
> > +{
> > +     substream->device->leave_draining_in_pause = true;
> > +}
> > +
> >  /* dsp driver callback apis
> >   * For playback: driver should call snd_compress_fragment_elapsed() to let the
> >   * framework know that a fragment has been consumed from the ring buffer
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index 21ce4c056a92..c6e5c8f072d7 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -719,8 +719,14 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
> >               if (!stream->device->use_pause_in_draining)
> >                       return -EPERM;
> >               retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> > -             if (!retval)
> > +             if (!retval) {
> > +                     if (stream->device->leave_draining_in_pause) {
> > +                             stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> > +                             wake_up(&stream->runtime->sleep);
> > +                             break;
> > +                     }
> >                       stream->pause_in_draining = true;
> > +             }
>
> Hrm, what actually happens with this new flag?  It changes the state
> to PAUSED even if it's done during the draining.  Then user resumes
> the pause via snd_compr_resume(), and now the state changes to
> RUNNING.  OTOH, if the draining runs normally, it'll end up with
> SETUP.
>
> Even if the above is even designed behavior, it must be described
> properly somewhere.  The state change is described in snd_compr_open()
> comment, and the new behavior should be mentioned there as well.
> (Admittedly, the previous hack for the pause-during-drain is also
> missing and should have been mentioned there; but an excuse is that
> the pause-during-drain doesn't change the state itself :)
>
>
> thanks,
>
> Takashi

Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Jaroslav Kysela]

On 08. 07. 21 15:47, Robert Lee wrote:
> Hi Takashi,
> 
> It is a little complex to describe the design in detail, but try to
> explain simply
> what issue we meet.
> 
> If w/o the change,  after user resumes from the pause, our system would call
> snd_compr_drain() or snd_compr_partial_drain() again after it returns from
> previous drain (when EOF reaches). Then it will block in this drain and no one
> wake it up because EOF has already reached. I add this change to return from
> the previous drain.

It looks like that the driver does not call snd_compr_drain_notify() so the
state is not updated to SETUP on EOF.

> Actually, I am wondering how the pause-during-drain can keep the state in
> DRAINING. It should have a different design. :)

I already proposed to add a new state (because it's a new state), but the
conservative way was elected to avoid user space changes.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Robert Lee]

Jaroslav Kysela <perex@perex.cz> 於 2021年7月8日 週四 下午10:53寫道：
>
> On 08. 07. 21 15:47, Robert Lee wrote:
> > Hi Takashi,
> >
> > It is a little complex to describe the design in detail, but try to
> > explain simply
> > what issue we meet.
> >
> > If w/o the change,  after user resumes from the pause, our system would call
> > snd_compr_drain() or snd_compr_partial_drain() again after it returns from
> > previous drain (when EOF reaches). Then it will block in this drain and no one
> > wake it up because EOF has already reached. I add this change to return from
> > the previous drain.
>
> It looks like that the driver does not call snd_compr_drain_notify() so the
> state is not updated to SETUP on EOF.
>
We indeed call snd_compr_drain_notify() on EOF, but after return from
wait_for _drain there is another drain again immediately.
Looks like the system queue some states change on user space and need
to drain again after resume from pause.
I suppose there is different design on user space so I add the hook to
handle diffent usage.

> > Actually, I am wondering how the pause-during-drain can keep the state in
> > DRAINING. It should have a different design. :)
>
> I already proposed to add a new state (because it's a new state), but the
> conservative way was elected to avoid user space changes.
>
>                                 Jaroslav
>
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining

[Takashi Iwai]

On Fri, 09 Jul 2021 04:08:29 +0200,
Robert Lee wrote:
> 
> Jaroslav Kysela <perex@perex.cz> 於 2021年7月8日 週四 下午10:53寫道：
> >
> > On 08. 07. 21 15:47, Robert Lee wrote:
> > > Hi Takashi,
> > >
> > > It is a little complex to describe the design in detail, but try to
> > > explain simply
> > > what issue we meet.
> > >
> > > If w/o the change,  after user resumes from the pause, our system would call
> > > snd_compr_drain() or snd_compr_partial_drain() again after it returns from
> > > previous drain (when EOF reaches). Then it will block in this drain and no one
> > > wake it up because EOF has already reached. I add this change to return from
> > > the previous drain.
> >
> > It looks like that the driver does not call snd_compr_drain_notify() so the
> > state is not updated to SETUP on EOF.
> >
> We indeed call snd_compr_drain_notify() on EOF, but after return from
> wait_for _drain there is another drain again immediately.
> Looks like the system queue some states change on user space and need
> to drain again after resume from pause.
> I suppose there is different design on user space so I add the hook to
> handle diffent usage.

Right, the previous drain-in-pause implementation was purely in the
kernel side, and user-space didn't change much; after resuming from
the pause, the driver resumes exactly to the same state before the
pause (i.e. start draining again).

The difference sounds similar like the suspend/resume scheme; some
does resume by itself to the previous state while some requires the
explicit action.

> > > Actually, I am wondering how the pause-during-drain can keep the state in
> > > DRAINING. It should have a different design. :)
> >
> > I already proposed to add a new state (because it's a new state), but the
> > conservative way was elected to avoid user space changes.

Yes, the primary concern is that the compress API uses the very same
state like PCM, and if we extend PCM state, it'll be a much larger
problem.  And, even if we change the state to compress-only, it's
still an ABI incompatibility, and it has to be carefully handled not
to break the existing application (e.g. expose the new state only when
the application is really ready to handle -- introducing a new ioctl
for state or introduce a new ioctl like SNDRV_PCM_IOCTL_USER_PVERSION
that informs the ABI version the user-space understands).


Takashi