[PATCH 1/4] ALSA: compress: Fix regression on compressed capture streams

[PATCH 1/4] ALSA: compress: Fix regression on compressed capture streams

[Charles Keepax]

A previous fix to the stop handling on compressed capture streams causes
some knock on issues. The previous fix updated snd_compr_drain_notify to
set the state back to PREPARED for capture streams. This causes some
issues however as the handling for snd_compr_poll differs between the
two states and some user-space applications were relying on the poll
failing after the stream had been stopped.

To correct this regression whilst still fixing the original problem the
patch was addressing, update the capture handling to skip the PREPARED
state rather than skipping the SETUP state as it has done until now.

Fixes: 4f2ab5e1d13d ("ALSA: compress: Fix stop handling on compressed capture streams")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/compress_driver.h |  5 +----
 sound/core/compress_offload.c   | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index c5188ff724d12..bc88d6f964da9 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -173,10 +173,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 	if (snd_BUG_ON(!stream))
 		return;
 
-	if (stream->direction == SND_COMPRESS_PLAYBACK)
-		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
-	else
-		stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
+	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 
 	wake_up(&stream->runtime->sleep);
 }
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a1a6fd75cfe50..f031495311ee4 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -587,10 +587,7 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 		stream->metadata_set = false;
 		stream->next_track = false;
 
-		if (stream->direction == SND_COMPRESS_PLAYBACK)
-			stream->runtime->state = SNDRV_PCM_STATE_SETUP;
-		else
-			stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
+		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 	} else {
 		return -EPERM;
 	}
@@ -706,8 +703,17 @@ static int snd_compr_start(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_SETUP:
+		if (stream->direction != SND_COMPRESS_CAPTURE)
+			return -EPERM;
+		break;
+	case SNDRV_PCM_STATE_PREPARED:
+		break;
+	default:
 		return -EPERM;
+	}
+
 	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_START);
 	if (!retval)
 		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
-- 
2.11.0

[PATCH 2/4] ALSA: compress: Prevent bypasses of set_params

[Charles Keepax]

Currently, whilst in SNDRV_PCM_STATE_OPEN it is possible to call
snd_compr_stop, snd_compr_drain and snd_compr_partial_drain, which
allow a transition to SNDRV_PCM_STATE_SETUP. The stream should
only be able to move to the setup state once it has received a
SNDRV_COMPRESS_SET_PARAMS ioctl. Fix this issue by not allowing
those ioctls whilst in the open state.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/core/compress_offload.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index f031495311ee4..e1a216fd832f9 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -724,9 +724,15 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_OPEN:
+	case SNDRV_PCM_STATE_SETUP:
+	case SNDRV_PCM_STATE_PREPARED:
 		return -EPERM;
+	default:
+		break;
+	}
+
 	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
 	if (!retval) {
 		snd_compr_drain_notify(stream);
@@ -814,9 +820,14 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_OPEN:
+	case SNDRV_PCM_STATE_SETUP:
+	case SNDRV_PCM_STATE_PREPARED:
 		return -EPERM;
+	default:
+		break;
+	}
 
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
 	if (retval) {
@@ -853,9 +864,16 @@ static int snd_compr_next_track(struct snd_compr_stream *stream)
 static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 {
 	int retval;
-	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
+
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_OPEN:
+	case SNDRV_PCM_STATE_SETUP:
+	case SNDRV_PCM_STATE_PREPARED:
 		return -EPERM;
+	default:
+		break;
+	}
+
 	/* stream can be drained only when next track has been signalled */
 	if (stream->next_track == false)
 		return -EPERM;
-- 
2.11.0

[PATCH 3/4] ALSA: compress: Don't allow drain operations on capture streams

[Charles Keepax]

Partial drain and next track are intended for gapless playback and
don't really have an obvious interpretation for a capture stream, so
makes sense to not allow those operations on capture streams. Drain
would make sense on a capture stream but currently the implementation
of drain involves the kernel waiting for the DSP to consume its
available data, whereas a capture drain would involve waiting for
user-space to consume the data available on the DSP. Disallow drain
on capture streams until that is implemented.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/core/compress_offload.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index e1a216fd832f9..c7d56cee0d510 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -829,6 +829,10 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 		break;
 	}
 
+	/* drain not implemented for capture streams yet */
+	if (stream->direction == SND_COMPRESS_CAPTURE)
+		return -EPERM;
+
 	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
 	if (retval) {
 		pr_debug("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
@@ -847,6 +851,10 @@ static int snd_compr_next_track(struct snd_compr_stream *stream)
 	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
 		return -EPERM;
 
+	/* next track doesn't have any meaning for capture streams */
+	if (stream->direction == SND_COMPRESS_CAPTURE)
+		return -EPERM;
+
 	/* you can signal next track if this is intended to be a gapless stream
 	 * and current track metadata is set
 	 */
@@ -874,6 +882,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 		break;
 	}
 
+	/* partial drain doesn't have any meaning for capture streams */
+	if (stream->direction == SND_COMPRESS_CAPTURE)
+		return -EPERM;
+
 	/* stream can be drained only when next track has been signalled */
 	if (stream->next_track == false)
 		return -EPERM;
-- 
2.11.0

Re: [PATCH 3/4] ALSA: compress: Don't allow drain operations on capture streams

[Vinod Koul]

On 09-07-19, 11:52, Charles Keepax wrote:
> Partial drain and next track are intended for gapless playback and
> don't really have an obvious interpretation for a capture stream, so
> makes sense to not allow those operations on capture streams. Drain

Sounds right

> would make sense on a capture stream but currently the implementation
> of drain involves the kernel waiting for the DSP to consume its
> available data, whereas a capture drain would involve waiting for
> user-space to consume the data available on the DSP. Disallow drain
> on capture streams until that is implemented.

Well it in unclear to me about the support required in kernel! Kernel
issues drain request to DSP and waits for that to be done. When DSP has
encoded and copied the data the notification should make drain complete.
So I dont see anything in kernel required for that

> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  sound/core/compress_offload.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index e1a216fd832f9..c7d56cee0d510 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -829,6 +829,10 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  		break;
>  	}
>  
> +	/* drain not implemented for capture streams yet */
> +	if (stream->direction == SND_COMPRESS_CAPTURE)
> +		return -EPERM;
> +
>  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
>  	if (retval) {
>  		pr_debug("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> @@ -847,6 +851,10 @@ static int snd_compr_next_track(struct snd_compr_stream *stream)
>  	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
>  		return -EPERM;
>  
> +	/* next track doesn't have any meaning for capture streams */
> +	if (stream->direction == SND_COMPRESS_CAPTURE)
> +		return -EPERM;
> +
>  	/* you can signal next track if this is intended to be a gapless stream
>  	 * and current track metadata is set
>  	 */
> @@ -874,6 +882,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  		break;
>  	}
>  
> +	/* partial drain doesn't have any meaning for capture streams */
> +	if (stream->direction == SND_COMPRESS_CAPTURE)
> +		return -EPERM;
> +
>  	/* stream can be drained only when next track has been signalled */
>  	if (stream->next_track == false)
>  		return -EPERM;
> -- 
> 2.11.0

-- 
~Vinod

Re: [PATCH 3/4] ALSA: compress: Don't allow drain operations on capture streams

[Charles Keepax]

On Tue, Jul 09, 2019 at 05:06:33PM +0530, Vinod Koul wrote:
> On 09-07-19, 11:52, Charles Keepax wrote:
> > would make sense on a capture stream but currently the implementation
> > of drain involves the kernel waiting for the DSP to consume its
> > available data, whereas a capture drain would involve waiting for
> > user-space to consume the data available on the DSP. Disallow drain
> > on capture streams until that is implemented.
> 
> Well it in unclear to me about the support required in kernel! Kernel
> issues drain request to DSP and waits for that to be done. When DSP has
> encoded and copied the data the notification should make drain complete.
> So I dont see anything in kernel required for that
> 

Ok that makes sense I think what I missed there is we are looking
at the capture drain being complete when the data is in the DSP
buffer, not when it has been pulled by user-space as I had.

I will update the patch to remove this one.

Thanks,
Charles

[PATCH 4/4] ALSA: compress: Be more restrictive about when a drain is allowed

[Charles Keepax]

Draining makes little sense in the situation of hardware overrun, as the
hardware will have consumed all its available samples. Additionally,
draining whilst the stream is paused would presumably get stuck as no
data is being consumed on the DSP side.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/core/compress_offload.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index c7d56cee0d510..9fcd06395a046 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -824,7 +824,10 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_OPEN:
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
+	case SNDRV_PCM_STATE_PAUSED:
 		return -EPERM;
+	case SNDRV_PCM_STATE_XRUN:
+		return -EPIPE;
 	default:
 		break;
 	}
@@ -877,7 +880,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_OPEN:
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
+	case SNDRV_PCM_STATE_PAUSED:
 		return -EPERM;
+	case SNDRV_PCM_STATE_XRUN:
+		return -EPIPE;
 	default:
 		break;
 	}
-- 
2.11.0