RE: [PATCH] ALSA: compress: allow pause and resume during draining

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On 9/29/20 04:13 PM, Takashi Iwai wrote:
>On Tue, 29 Sep 2020 03:51:35 +0200,
>Gyeongtaek Lee wrote:
>> 
>> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>> >On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>> >> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>> >>> With a stream with low bitrate, user can't pause or resume the stream
>> >>> near the end of the stream because current ALSA doesn't allow it.
>> >>> If the stream has very low bitrate enough to store whole stream into
>> >>> the buffer, user can't do anything except stop the stream and then
>> >>> restart it from the first.
>> >>> If pause and resume is allowed during draining, user experience can be
>> >>> enhanced.
>> >> 
>> >> It seems that we need a new state to handle the pause + drain condition for
>> >> this case.
>> >> 
>> >> With this proposed change, the pause state in drain is invisible.
>> >
>> >Indeed it's be much nicer to have a new state, e..g 
>> >SNDRV_PCM_STATE_DRAINING_PAUSED.
>> Ok. I will add the new state.
>> >
>> >One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>> >this have impacts on userspace as well? We'd change the value of 
>> >SNDRV_PCM_STATE_LAST.
>> >
>> I also agree that adding new state and increase LAST value in the header of uapi
>> could be dangerous. So, I added it to comress_offload.h for now.
>> It could be merged into snd_pcm_state_t in someday with big changes.
>> Could you review the fixed patch below?
>
>Hrm, this resulted in rather more complex changes than the original
>patch.  It shows that introducing yet another state is no good idea
>for this particular case.
>
>Since the possible application's behavior after this pause is as same
>as the normal pause (i.e. either resuming pause or dropping), I find
>it OK to take the original approach.
Thank you for the review.
I think that I should resend the original patch.
Am I right?
>
>
>thanks,
>
>Takashi
>
>> With a stream with low bitrate, user can't pause or resume the stream
>> near the end of the stream because current ALSA doesn't allow it.
>> If the stream has very low bitrate enough to store whole stream into
>> the buffer, user can't do anything except stop the stream and then
>> restart it from first.
>> If pause, resume are allowed during draining, user experience can be
>> enhanced.
>> 
>> New state for pause during draining is defined in compress_offload.h for
>> now. If PCM_STATEs in uapi/sound/asound.h is changed, pcm libraries and
>> userspace application will be affected.
>> 
>> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  include/uapi/sound/compress_offload.h |  3 ++
>>  sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
>>  2 files changed, 41 insertions(+), 9 deletions(-)
>> 
>> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
>> index 7184265c0b0d..f30b9851d1d7 100644
>> --- a/include/uapi/sound/compress_offload.h
>> +++ b/include/uapi/sound/compress_offload.h
>> @@ -189,4 +189,7 @@ struct snd_compr_metadata {
>>  #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
>>  #define SND_COMPR_TRIGGER_NEXT_TRACK 8
>>  #define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
>> +
>> +/* FIXME move this to asound.h */
>> +#define	SNDRV_PCM_STATE_DRAINING_PAUSED	(SNDRV_PCM_STATE_LAST + 1)
>>  #endif
>> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
>> index 0e53f6f31916..58fbe0d99431 100644
>> --- a/sound/core/compress_offload.c
>> +++ b/sound/core/compress_offload.c
>> @@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>>  	case SNDRV_PCM_STATE_RUNNING:
>>  	case SNDRV_PCM_STATE_DRAINING:
>>  	case SNDRV_PCM_STATE_PAUSED:
>> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>>  		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
>>  		break;
>>  	default:
>> @@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
>>  	case SNDRV_PCM_STATE_RUNNING:
>>  	case SNDRV_PCM_STATE_PREPARED:
>>  	case SNDRV_PCM_STATE_PAUSED:
>> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>>  		if (avail >= stream->runtime->fragment_size)
>>  			retval = snd_compr_get_poll(stream);
>>  		break;
>> @@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>>  {
>>  	int retval;
>>  
>> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
>> +	switch (stream->runtime->state) {
>> +	case SNDRV_PCM_STATE_RUNNING:
>> +		retval = stream->ops->trigger(stream,
>> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> +		if (!retval)
>> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
>> +		break;
>> +	case SNDRV_PCM_STATE_DRAINING:
>> +		retval = stream->ops->trigger(stream,
>> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> +		if (!retval)
>> +			stream->runtime->state =
>> +				SNDRV_PCM_STATE_DRAINING_PAUSED;
>> +		break;
>> +	default:
>>  		return -EPERM;
>> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> -	if (!retval)
>> -		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
>> +	}
>>  	return retval;
>>  }
>>  
>> @@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
>>  {
>>  	int retval;
>>  
>> -	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
>> +	switch (stream->runtime->state) {
>> +	case SNDRV_PCM_STATE_PAUSED:
>> +		retval = stream->ops->trigger(stream,
>> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> +		if (!retval)
>> +			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>> +		break;
>> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> +		retval = stream->ops->trigger(stream,
>> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> +		if (!retval)
>> +			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
>> +		break;
>> +	default:
>>  		return -EPERM;
>> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> -	if (!retval)
>> -		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>> +	}
>>  	return retval;
>>  }
>>  
>> @@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
>>  	 */
>>  
>>  	ret = wait_event_interruptible(stream->runtime->sleep,
>> -			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
>> +			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
>> +			(stream->runtime->state !=
>> +				SNDRV_PCM_STATE_DRAINING_PAUSED));
>>  	if (ret == -ERESTARTSYS)
>>  		pr_debug("wait aborted by a signal\n");
>>  	else if (ret)
>> @@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>>  	case SNDRV_PCM_STATE_SETUP:
>>  	case SNDRV_PCM_STATE_PREPARED:
>>  	case SNDRV_PCM_STATE_PAUSED:
>> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>>  		return -EPERM;
>>  	case SNDRV_PCM_STATE_XRUN:
>>  		return -EPIPE;
>> @@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>>  	case SNDRV_PCM_STATE_SETUP:
>>  	case SNDRV_PCM_STATE_PREPARED:
>>  	case SNDRV_PCM_STATE_PAUSED:
>> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>>  		return -EPERM;
>>  	case SNDRV_PCM_STATE_XRUN:
>>  		return -EPIPE;
>> 
>> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
>> -- 
>> 2.21.0
>> 
>> 
>

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Tue, 29 Sep 2020 10:40:51 +0200,
Gyeongtaek Lee wrote:
> 
> On 9/29/20 04:13 PM, Takashi Iwai wrote:
> >On Tue, 29 Sep 2020 03:51:35 +0200,
> >Gyeongtaek Lee wrote:
> >> 
> >> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
> >> >On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> >> >> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> >> >>> With a stream with low bitrate, user can't pause or resume the stream
> >> >>> near the end of the stream because current ALSA doesn't allow it.
> >> >>> If the stream has very low bitrate enough to store whole stream into
> >> >>> the buffer, user can't do anything except stop the stream and then
> >> >>> restart it from the first.
> >> >>> If pause and resume is allowed during draining, user experience can be
> >> >>> enhanced.
> >> >> 
> >> >> It seems that we need a new state to handle the pause + drain condition for
> >> >> this case.
> >> >> 
> >> >> With this proposed change, the pause state in drain is invisible.
> >> >
> >> >Indeed it's be much nicer to have a new state, e..g 
> >> >SNDRV_PCM_STATE_DRAINING_PAUSED.
> >> Ok. I will add the new state.
> >> >
> >> >One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
> >> >this have impacts on userspace as well? We'd change the value of 
> >> >SNDRV_PCM_STATE_LAST.
> >> >
> >> I also agree that adding new state and increase LAST value in the header of uapi
> >> could be dangerous. So, I added it to comress_offload.h for now.
> >> It could be merged into snd_pcm_state_t in someday with big changes.
> >> Could you review the fixed patch below?
> >
> >Hrm, this resulted in rather more complex changes than the original
> >patch.  It shows that introducing yet another state is no good idea
> >for this particular case.
> >
> >Since the possible application's behavior after this pause is as same
> >as the normal pause (i.e. either resuming pause or dropping), I find
> >it OK to take the original approach.
> Thank you for the review.
> I think that I should resend the original patch.

Let's wait a bit for other opinions.  We may add rather a new flag
instead of introducing a new state, too, for example.

Also, I'm not sure about any side-effect to drivers that expect the
pause only during the running state.  We might need some check for a
capability flag?

In anyway, the timing is bad; it's too late for 5.10 to apply such a
core change.  Can we postpone this for 5.11?


thanks,

Takashi


> Am I right?
> >
> >
> >thanks,
> >
> >Takashi
> >
> >> With a stream with low bitrate, user can't pause or resume the stream
> >> near the end of the stream because current ALSA doesn't allow it.
> >> If the stream has very low bitrate enough to store whole stream into
> >> the buffer, user can't do anything except stop the stream and then
> >> restart it from first.
> >> If pause, resume are allowed during draining, user experience can be
> >> enhanced.
> >> 
> >> New state for pause during draining is defined in compress_offload.h for
> >> now. If PCM_STATEs in uapi/sound/asound.h is changed, pcm libraries and
> >> userspace application will be affected.
> >> 
> >> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  include/uapi/sound/compress_offload.h |  3 ++
> >>  sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
> >>  2 files changed, 41 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> >> index 7184265c0b0d..f30b9851d1d7 100644
> >> --- a/include/uapi/sound/compress_offload.h
> >> +++ b/include/uapi/sound/compress_offload.h
> >> @@ -189,4 +189,7 @@ struct snd_compr_metadata {
> >>  #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
> >>  #define SND_COMPR_TRIGGER_NEXT_TRACK 8
> >>  #define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
> >> +
> >> +/* FIXME move this to asound.h */
> >> +#define	SNDRV_PCM_STATE_DRAINING_PAUSED	(SNDRV_PCM_STATE_LAST + 1)
> >>  #endif
> >> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> >> index 0e53f6f31916..58fbe0d99431 100644
> >> --- a/sound/core/compress_offload.c
> >> +++ b/sound/core/compress_offload.c
> >> @@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
> >>  	case SNDRV_PCM_STATE_RUNNING:
> >>  	case SNDRV_PCM_STATE_DRAINING:
> >>  	case SNDRV_PCM_STATE_PAUSED:
> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> >>  		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
> >>  		break;
> >>  	default:
> >> @@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
> >>  	case SNDRV_PCM_STATE_RUNNING:
> >>  	case SNDRV_PCM_STATE_PREPARED:
> >>  	case SNDRV_PCM_STATE_PAUSED:
> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> >>  		if (avail >= stream->runtime->fragment_size)
> >>  			retval = snd_compr_get_poll(stream);
> >>  		break;
> >> @@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
> >>  {
> >>  	int retval;
> >>  
> >> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> >> +	switch (stream->runtime->state) {
> >> +	case SNDRV_PCM_STATE_RUNNING:
> >> +		retval = stream->ops->trigger(stream,
> >> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> >> +		if (!retval)
> >> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> >> +		break;
> >> +	case SNDRV_PCM_STATE_DRAINING:
> >> +		retval = stream->ops->trigger(stream,
> >> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> >> +		if (!retval)
> >> +			stream->runtime->state =
> >> +				SNDRV_PCM_STATE_DRAINING_PAUSED;
> >> +		break;
> >> +	default:
> >>  		return -EPERM;
> >> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> >> -	if (!retval)
> >> -		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> >> +	}
> >>  	return retval;
> >>  }
> >>  
> >> @@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
> >>  {
> >>  	int retval;
> >>  
> >> -	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
> >> +	switch (stream->runtime->state) {
> >> +	case SNDRV_PCM_STATE_PAUSED:
> >> +		retval = stream->ops->trigger(stream,
> >> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> >> +		if (!retval)
> >> +			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> >> +		break;
> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> >> +		retval = stream->ops->trigger(stream,
> >> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> >> +		if (!retval)
> >> +			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> >> +		break;
> >> +	default:
> >>  		return -EPERM;
> >> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> >> -	if (!retval)
> >> -		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> >> +	}
> >>  	return retval;
> >>  }
> >>  
> >> @@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> >>  	 */
> >>  
> >>  	ret = wait_event_interruptible(stream->runtime->sleep,
> >> -			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
> >> +			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
> >> +			(stream->runtime->state !=
> >> +				SNDRV_PCM_STATE_DRAINING_PAUSED));
> >>  	if (ret == -ERESTARTSYS)
> >>  		pr_debug("wait aborted by a signal\n");
> >>  	else if (ret)
> >> @@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> >>  	case SNDRV_PCM_STATE_SETUP:
> >>  	case SNDRV_PCM_STATE_PREPARED:
> >>  	case SNDRV_PCM_STATE_PAUSED:
> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> >>  		return -EPERM;
> >>  	case SNDRV_PCM_STATE_XRUN:
> >>  		return -EPIPE;
> >> @@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> >>  	case SNDRV_PCM_STATE_SETUP:
> >>  	case SNDRV_PCM_STATE_PREPARED:
> >>  	case SNDRV_PCM_STATE_PAUSED:
> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> >>  		return -EPERM;
> >>  	case SNDRV_PCM_STATE_XRUN:
> >>  		return -EPIPE;
> >> 
> >> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> >> -- 
> >> 2.21.0
> >> 
> >> 
> >
> 

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On 9/29/20 06:14 PM, Takashi Iwai wrote:
>On Tue, 29 Sep 2020 10:40:51 +0200,
>Gyeongtaek Lee wrote:
>> 
>> On 9/29/20 04:13 PM, Takashi Iwai wrote:
>> >On Tue, 29 Sep 2020 03:51:35 +0200,
>> >Gyeongtaek Lee wrote:
>> >> 
>> >> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>> >> >On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>> >> >> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>> >> >>> With a stream with low bitrate, user can't pause or resume the stream
>> >> >>> near the end of the stream because current ALSA doesn't allow it.
>> >> >>> If the stream has very low bitrate enough to store whole stream into
>> >> >>> the buffer, user can't do anything except stop the stream and then
>> >> >>> restart it from the first.
>> >> >>> If pause and resume is allowed during draining, user experience can be
>> >> >>> enhanced.
>> >> >> 
>> >> >> It seems that we need a new state to handle the pause + drain condition for
>> >> >> this case.
>> >> >> 
>> >> >> With this proposed change, the pause state in drain is invisible.
>> >> >
>> >> >Indeed it's be much nicer to have a new state, e..g 
>> >> >SNDRV_PCM_STATE_DRAINING_PAUSED.
>> >> Ok. I will add the new state.
>> >> >
>> >> >One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>> >> >this have impacts on userspace as well? We'd change the value of 
>> >> >SNDRV_PCM_STATE_LAST.
>> >> >
>> >> I also agree that adding new state and increase LAST value in the header of uapi
>> >> could be dangerous. So, I added it to comress_offload.h for now.
>> >> It could be merged into snd_pcm_state_t in someday with big changes.
>> >> Could you review the fixed patch below?
>> >
>> >Hrm, this resulted in rather more complex changes than the original
>> >patch.  It shows that introducing yet another state is no good idea
>> >for this particular case.
>> >
>> >Since the possible application's behavior after this pause is as same
>> >as the normal pause (i.e. either resuming pause or dropping), I find
>> >it OK to take the original approach.
>> Thank you for the review.
>> I think that I should resend the original patch.
>
>Let's wait a bit for other opinions.  We may add rather a new flag
>instead of introducing a new state, too, for example.
>
>Also, I'm not sure about any side-effect to drivers that expect the
>pause only during the running state.  We might need some check for a
>capability flag?
Ok. I will wait for more opinion and then resend fixed patch.
>
>In anyway, the timing is bad; it's too late for 5.10 to apply such a
>core change.  Can we postpone this for 5.11?
No problem. Actually I need this patch on 5.4 stable.
>
>
>thanks,
>
>Takashi
>
>
>> Am I right?
>> >
>> >
>> >thanks,
>> >
>> >Takashi
>> >
>> >> With a stream with low bitrate, user can't pause or resume the stream
>> >> near the end of the stream because current ALSA doesn't allow it.
>> >> If the stream has very low bitrate enough to store whole stream into
>> >> the buffer, user can't do anything except stop the stream and then
>> >> restart it from first.
>> >> If pause, resume are allowed during draining, user experience can be
>> >> enhanced.
>> >> 
>> >> New state for pause during draining is defined in compress_offload.h for
>> >> now. If PCM_STATEs in uapi/sound/asound.h is changed, pcm libraries and
>> >> userspace application will be affected.
>> >> 
>> >> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
>> >> Cc: stable@vger.kernel.org
>> >> ---
>> >>  include/uapi/sound/compress_offload.h |  3 ++
>> >>  sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
>> >>  2 files changed, 41 insertions(+), 9 deletions(-)
>> >> 
>> >> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
>> >> index 7184265c0b0d..f30b9851d1d7 100644
>> >> --- a/include/uapi/sound/compress_offload.h
>> >> +++ b/include/uapi/sound/compress_offload.h
>> >> @@ -189,4 +189,7 @@ struct snd_compr_metadata {
>> >>  #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
>> >>  #define SND_COMPR_TRIGGER_NEXT_TRACK 8
>> >>  #define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
>> >> +
>> >> +/* FIXME move this to asound.h */
>> >> +#define	SNDRV_PCM_STATE_DRAINING_PAUSED	(SNDRV_PCM_STATE_LAST + 1)
>> >>  #endif
>> >> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
>> >> index 0e53f6f31916..58fbe0d99431 100644
>> >> --- a/sound/core/compress_offload.c
>> >> +++ b/sound/core/compress_offload.c
>> >> @@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>> >>  	case SNDRV_PCM_STATE_RUNNING:
>> >>  	case SNDRV_PCM_STATE_DRAINING:
>> >>  	case SNDRV_PCM_STATE_PAUSED:
>> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> >>  		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
>> >>  		break;
>> >>  	default:
>> >> @@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
>> >>  	case SNDRV_PCM_STATE_RUNNING:
>> >>  	case SNDRV_PCM_STATE_PREPARED:
>> >>  	case SNDRV_PCM_STATE_PAUSED:
>> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> >>  		if (avail >= stream->runtime->fragment_size)
>> >>  			retval = snd_compr_get_poll(stream);
>> >>  		break;
>> >> @@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>> >>  {
>> >>  	int retval;
>> >>  
>> >> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
>> >> +	switch (stream->runtime->state) {
>> >> +	case SNDRV_PCM_STATE_RUNNING:
>> >> +		retval = stream->ops->trigger(stream,
>> >> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> >> +		if (!retval)
>> >> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
>> >> +		break;
>> >> +	case SNDRV_PCM_STATE_DRAINING:
>> >> +		retval = stream->ops->trigger(stream,
>> >> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> >> +		if (!retval)
>> >> +			stream->runtime->state =
>> >> +				SNDRV_PCM_STATE_DRAINING_PAUSED;
>> >> +		break;
>> >> +	default:
>> >>  		return -EPERM;
>> >> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
>> >> -	if (!retval)
>> >> -		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
>> >> +	}
>> >>  	return retval;
>> >>  }
>> >>  
>> >> @@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
>> >>  {
>> >>  	int retval;
>> >>  
>> >> -	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
>> >> +	switch (stream->runtime->state) {
>> >> +	case SNDRV_PCM_STATE_PAUSED:
>> >> +		retval = stream->ops->trigger(stream,
>> >> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> >> +		if (!retval)
>> >> +			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>> >> +		break;
>> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> >> +		retval = stream->ops->trigger(stream,
>> >> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> >> +		if (!retval)
>> >> +			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
>> >> +		break;
>> >> +	default:
>> >>  		return -EPERM;
>> >> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
>> >> -	if (!retval)
>> >> -		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>> >> +	}
>> >>  	return retval;
>> >>  }
>> >>  
>> >> @@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
>> >>  	 */
>> >>  
>> >>  	ret = wait_event_interruptible(stream->runtime->sleep,
>> >> -			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
>> >> +			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
>> >> +			(stream->runtime->state !=
>> >> +				SNDRV_PCM_STATE_DRAINING_PAUSED));
>> >>  	if (ret == -ERESTARTSYS)
>> >>  		pr_debug("wait aborted by a signal\n");
>> >>  	else if (ret)
>> >> @@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>> >>  	case SNDRV_PCM_STATE_SETUP:
>> >>  	case SNDRV_PCM_STATE_PREPARED:
>> >>  	case SNDRV_PCM_STATE_PAUSED:
>> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> >>  		return -EPERM;
>> >>  	case SNDRV_PCM_STATE_XRUN:
>> >>  		return -EPIPE;
>> >> @@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>> >>  	case SNDRV_PCM_STATE_SETUP:
>> >>  	case SNDRV_PCM_STATE_PREPARED:
>> >>  	case SNDRV_PCM_STATE_PAUSED:
>> >> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>> >>  		return -EPERM;
>> >>  	case SNDRV_PCM_STATE_XRUN:
>> >>  		return -EPIPE;
>> >> 
>> >> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
>> >> -- 
>> >> 2.21.0
>> >> 
>> >> 
>> >
>> 
>

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Pierre-Louis Bossart]

>>>> Since the possible application's behavior after this pause is as same
>>>> as the normal pause (i.e. either resuming pause or dropping), I find
>>>> it OK to take the original approach.
>>> Thank you for the review.
>>> I think that I should resend the original patch.
>>
>> Let's wait a bit for other opinions.  We may add rather a new flag
>> instead of introducing a new state, too, for example.
>>
>> Also, I'm not sure about any side-effect to drivers that expect the
>> pause only during the running state.  We might need some check for a
>> capability flag?
> Ok. I will wait for more opinion and then resend fixed patch.

Question: have you thought about the case where a 'partial drain' 
happens, typically when you are doing gapless playback?

I see in snd_compress_wait_for_drain() a wait on a state different from 
DRAINING, which is precisely what would be modified with your proposal. 
It's been a while since I looked at this code but it'd be worth checking 
that the pause is supported in 'normal' and 'partial' drain cases.

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

On 29-09-20, 10:54, Takashi Iwai wrote:
> On Tue, 29 Sep 2020 10:40:51 +0200,
> Gyeongtaek Lee wrote:
> > 
> > On 9/29/20 04:13 PM, Takashi Iwai wrote:
> > >On Tue, 29 Sep 2020 03:51:35 +0200,
> > >Gyeongtaek Lee wrote:
> > >> 
> > >> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
> > >> >On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> > >> >> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> > >> >>> With a stream with low bitrate, user can't pause or resume the stream
> > >> >>> near the end of the stream because current ALSA doesn't allow it.
> > >> >>> If the stream has very low bitrate enough to store whole stream into
> > >> >>> the buffer, user can't do anything except stop the stream and then
> > >> >>> restart it from the first.
> > >> >>> If pause and resume is allowed during draining, user experience can be
> > >> >>> enhanced.
> > >> >> 
> > >> >> It seems that we need a new state to handle the pause + drain condition for
> > >> >> this case.
> > >> >> 
> > >> >> With this proposed change, the pause state in drain is invisible.
> > >> >
> > >> >Indeed it's be much nicer to have a new state, e..g 
> > >> >SNDRV_PCM_STATE_DRAINING_PAUSED.
> > >> Ok. I will add the new state.
> > >> >
> > >> >One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
> > >> >this have impacts on userspace as well? We'd change the value of 
> > >> >SNDRV_PCM_STATE_LAST.
> > >> >
> > >> I also agree that adding new state and increase LAST value in the header of uapi
> > >> could be dangerous. So, I added it to comress_offload.h for now.
> > >> It could be merged into snd_pcm_state_t in someday with big changes.
> > >> Could you review the fixed patch below?
> > >
> > >Hrm, this resulted in rather more complex changes than the original
> > >patch.  It shows that introducing yet another state is no good idea
> > >for this particular case.
> > >
> > >Since the possible application's behavior after this pause is as same
> > >as the normal pause (i.e. either resuming pause or dropping), I find
> > >it OK to take the original approach.
> > Thank you for the review.
> > I think that I should resend the original patch.
> 
> Let's wait a bit for other opinions.  We may add rather a new flag
> instead of introducing a new state, too, for example.

I was out for a week, back now ;-)

So bigger question is if kernel should handle this change or we ask
userspace to handle this. Userland knows that bit rate is less so small
buffer can be for longer duration so instead of sending dumb X bytes,
should it not scale the buffer based on bit rate?

From that premise the partial_drain should be invoked only for last
write().

Also, I am bit skeptical for adding changes to states while we are
draining (that too partial one)... is this change driving complex
changes and should we push back to have this implemented better in
userland..?

> 
> Also, I'm not sure about any side-effect to drivers that expect the
> pause only during the running state.  We might need some check for a
> capability flag?
> 
> In anyway, the timing is bad; it's too late for 5.10 to apply such a
> core change.  Can we postpone this for 5.11?

Yes this needs more thinking, I am still not convinced kernel should
handle it!

Regards
-- 
~Vinod

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Pierre-Louis Bossart]

>>>> Hrm, this resulted in rather more complex changes than the original
>>>> patch.  It shows that introducing yet another state is no good idea
>>>> for this particular case.
>>>>
>>>> Since the possible application's behavior after this pause is as same
>>>> as the normal pause (i.e. either resuming pause or dropping), I find
>>>> it OK to take the original approach.
>>> Thank you for the review.
>>> I think that I should resend the original patch.
>>
>> Let's wait a bit for other opinions.  We may add rather a new flag
>> instead of introducing a new state, too, for example.
> 
> I was out for a week, back now ;-)
> 
> So bigger question is if kernel should handle this change or we ask
> userspace to handle this. Userland knows that bit rate is less so small
> buffer can be for longer duration so instead of sending dumb X bytes,
> should it not scale the buffer based on bit rate?

what about variable bit-rate? or cases where you have a 'bit reservoir' 
in previous frames?

You really cannot in general translate bytes to time with compressed 
data, which is the reason we introduced the API in the first place...

Userspace *may* know the duration for specific formats or based on 
metadata, but in some cases the only way to know is actually to decode 
the data.

I suggest we keep the compressed API based on bytes and non-periodic 
events when the bytes were consumed/generated.

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

On 01-10-20, 10:28, Pierre-Louis Bossart wrote:
> 
> 
> 
> > > > > Hrm, this resulted in rather more complex changes than the original
> > > > > patch.  It shows that introducing yet another state is no good idea
> > > > > for this particular case.
> > > > > 
> > > > > Since the possible application's behavior after this pause is as same
> > > > > as the normal pause (i.e. either resuming pause or dropping), I find
> > > > > it OK to take the original approach.
> > > > Thank you for the review.
> > > > I think that I should resend the original patch.
> > > 
> > > Let's wait a bit for other opinions.  We may add rather a new flag
> > > instead of introducing a new state, too, for example.
> > 
> > I was out for a week, back now ;-)
> > 
> > So bigger question is if kernel should handle this change or we ask
> > userspace to handle this. Userland knows that bit rate is less so small
> > buffer can be for longer duration so instead of sending dumb X bytes,
> > should it not scale the buffer based on bit rate?
> 
> what about variable bit-rate? or cases where you have a 'bit reservoir' in
> previous frames?
> 
> You really cannot in general translate bytes to time with compressed data,
> which is the reason we introduced the API in the first place...
> 
> Userspace *may* know the duration for specific formats or based on metadata,
> but in some cases the only way to know is actually to decode the data.
> 
> I suggest we keep the compressed API based on bytes and non-periodic events
> when the bytes were consumed/generated.

Changing the API away from bytes was not proposed so not sure...

The SM in kernel might be bit more convoluted so was wondering if we can
handle this in userland. The changelog for this patch says that for
test case was sending whole file, surely that is not an optimal approach.
Should we allow folks to send whole file to kernel and then issue
partial drain?

-- 
~Vinod

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Pierre-Louis Bossart]

> The SM in kernel might be bit more convoluted so was wondering if we can
> handle this in userland. The changelog for this patch says that for
> test case was sending whole file, surely that is not an optimal approach.

It's rather common to have to deal with very small files, even with PCM, 
e.g. for notifications. It's actually a classic test case that exposes 
design issues in drivers, where e.g. the last part of the notification 
is not played.

> Should we allow folks to send whole file to kernel and then issue
> partial drain?

I don't think there should be a conceptual limitation here. If the 
userspace knows that the last part of the file is smaller than a 
fragment it should be able to issue a drain (or partial drain if it's a 
gapless solution).

However now that I think of it, I am not sure what happens if the file 
is smaller than a fragment. That may very well be a limitation in the 
design.

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
>> The SM in kernel might be bit more convoluted so was wondering if we can
>> handle this in userland. The changelog for this patch says that for
>> test case was sending whole file, surely that is not an optimal approach.
>
>It's rather common to have to deal with very small files, even with PCM, 
>e.g. for notifications. It's actually a classic test case that exposes 
>design issues in drivers, where e.g. the last part of the notification 
>is not played.
>
>> Should we allow folks to send whole file to kernel and then issue
>> partial drain?
>
>I don't think there should be a conceptual limitation here. If the 
>userspace knows that the last part of the file is smaller than a 
>fragment it should be able to issue a drain (or partial drain if it's a 
>gapless solution).
>
>However now that I think of it, I am not sure what happens if the file 
>is smaller than a fragment. That may very well be a limitation in the 
>design.
>
Thanks for the comments.

Actually, problem can be occurred with big file also.
Application usually requests draining after sending last frame.
If user clicks pause button after draining, pause will be failed
and the file just be played until end.
If application stop and start playback for this case, 
start of last frame will be heard again because stop sets state to SETUP,
and write is needed to set the state to PREPARED for start.
If bitrate of the file is low, time stamp will be reversed and be heard weird.
I also hope this problem can be handled in userspace easily but I couldn't find a way for now.

I think that this is the time that I should share fixed patch following the comments to help the discussion.
Following opinions are added to the patch.
1. it's be much nicer to have a new state - Takashi
2. We can add this state to asound.h so the user space can be updated. - Jaroslav
3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav

I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
with a change in asound.h not in compress_offload.h.
Should I increase SNDRV_PCM_VERSION also?

And what happened if I request back-porting a patch which changes VERSION to stables?

Below is the patch just to help discussion.
I'll resend the fixed patch after this discussion is completed.

With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first.
If pause, resume are allowed during draining, user experience can be
enhanced.

The version is increased due to new behavior.

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/uapi/sound/asound.h           |  5 +--
 include/uapi/sound/compress_offload.h |  2 +-
 sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..499b364974ec 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -315,8 +315,9 @@ typedef int __bitwise snd_pcm_state_t;
 #define	SNDRV_PCM_STATE_XRUN		((__force snd_pcm_state_t) 4) /* stream reached an xrun */
 #define	SNDRV_PCM_STATE_DRAINING	((__force snd_pcm_state_t) 5) /* stream is draining */
 #define	SNDRV_PCM_STATE_PAUSED		((__force snd_pcm_state_t) 6) /* stream is paused */
-#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 7) /* hardware is suspended */
-#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 8) /* hardware is disconnected */
+#define	SNDRV_PCM_STATE_DRAINING_PAUSED	((__force snd_pcm_state_t) 7) /* stream is paused during draining*/
+#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 8) /* hardware is suspended */
+#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 9) /* hardware is disconnected */
 #define	SNDRV_PCM_STATE_LAST		SNDRV_PCM_STATE_DISCONNECTED
 
 enum {
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 7184265c0b0d..46fdf50d5c00 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -31,7 +31,7 @@
 #include <sound/compress_params.h>
 
 
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 1)
 /**
  * struct snd_compressed_buffer - compressed buffer
  * @fragment_size: size of buffer fragment in bytes
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..58fbe0d99431 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_DRAINING:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
 		break;
 	default:
@@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		if (avail >= stream->runtime->fragment_size)
 			retval = snd_compr_get_poll(stream);
 		break;
@@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state =
+				SNDRV_PCM_STATE_DRAINING_PAUSED;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 	 */
 
 	ret = wait_event_interruptible(stream->runtime->sleep,
-			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
+			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
+			(stream->runtime->state !=
+				SNDRV_PCM_STATE_DRAINING_PAUSED));
 	if (ret == -ERESTARTSYS)
 		pr_debug("wait aborted by a signal\n");
 	else if (ret)
@@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
@@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
-- 
2.21.0

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Thu, 08 Oct 2020 11:49:24 +0200,
Gyeongtaek Lee wrote:
> 
> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
> >> The SM in kernel might be bit more convoluted so was wondering if we can
> >> handle this in userland. The changelog for this patch says that for
> >> test case was sending whole file, surely that is not an optimal approach.
> >
> >It's rather common to have to deal with very small files, even with PCM, 
> >e.g. for notifications. It's actually a classic test case that exposes 
> >design issues in drivers, where e.g. the last part of the notification 
> >is not played.
> >
> >> Should we allow folks to send whole file to kernel and then issue
> >> partial drain?
> >
> >I don't think there should be a conceptual limitation here. If the 
> >userspace knows that the last part of the file is smaller than a 
> >fragment it should be able to issue a drain (or partial drain if it's a 
> >gapless solution).
> >
> >However now that I think of it, I am not sure what happens if the file 
> >is smaller than a fragment. That may very well be a limitation in the 
> >design.
> >
> Thanks for the comments.
> 
> Actually, problem can be occurred with big file also.
> Application usually requests draining after sending last frame.
> If user clicks pause button after draining, pause will be failed
> and the file just be played until end.
> If application stop and start playback for this case, 
> start of last frame will be heard again because stop sets state to SETUP,
> and write is needed to set the state to PREPARED for start.
> If bitrate of the file is low, time stamp will be reversed and be heard weird.
> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
> 
> I think that this is the time that I should share fixed patch following the comments to help the discussion.
> Following opinions are added to the patch.
> 1. it's be much nicer to have a new state - Takashi

Well, it wasn't me; I'm not against the new state *iff* it would end
up with cleaner code.  Admittedly, the new state can be more
"consistent" regarding the state transition.  If we allow the PAUSE
state during DRAINING, it'll lead to multiple states after resuming
the pause.

> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
> 
> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
> with a change in asound.h not in compress_offload.h.
> Should I increase SNDRV_PCM_VERSION also?

Yes, if we really add the PCM state, it's definitely needed.

> And what happened if I request back-porting a patch which changes VERSION to stables?

If we introduce the new change, it must be safe to the old kernels,
too.  The problem is only about the compatibility of the user-space
program, not about the kernel.


HOWEVER: I'm still concerned by the addition of a new PCM state.
Jaroslav suggested two steps approach, (1) first add the state only in
the uapi header, then use (2) the new state actually.  But, this
doesn't help much, simply because the step 1 won't catch real bugs.

Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
guess most of code can be compiled fine.  So, at the step 1, no one
notices it and bothered, either.  But, at the step 2, you'll hit a
problem.

Adding a new state is something like to add a new color to the traffic
signal.  In some countries, the car turning right at a crossing
doesn't have to stop at a red signal.  Suppose that we want to control
it, and change the international rule by introducing a new color (say
magenta) signal to stop the car turning right.  That'll be a big
confusion because most drivers are trained for only red, green and
yellow signals.

Similarly, if we add a new PCM state, every program code that deals
with the PCM state may be confused by the new state.  It has to be
reviewed and corrected manually, because it's no syntax problem the
compiler may catch.


thanks,

Takashi


> 
> Below is the patch just to help discussion.
> I'll resend the fixed patch after this discussion is completed.
> 
> With a stream with low bitrate, user can't pause or resume the stream
> near the end of the stream because current ALSA doesn't allow it.
> If the stream has very low bitrate enough to store whole stream into
> the buffer, user can't do anything except stop the stream and then
> restart it from the first.
> If pause, resume are allowed during draining, user experience can be
> enhanced.
> 
> The version is increased due to new behavior.
> 
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org
> ---
>  include/uapi/sound/asound.h           |  5 +--
>  include/uapi/sound/compress_offload.h |  2 +-
>  sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
>  3 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..499b364974ec 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -315,8 +315,9 @@ typedef int __bitwise snd_pcm_state_t;
>  #define	SNDRV_PCM_STATE_XRUN		((__force snd_pcm_state_t) 4) /* stream reached an xrun */
>  #define	SNDRV_PCM_STATE_DRAINING	((__force snd_pcm_state_t) 5) /* stream is draining */
>  #define	SNDRV_PCM_STATE_PAUSED		((__force snd_pcm_state_t) 6) /* stream is paused */
> -#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 7) /* hardware is suspended */
> -#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 8) /* hardware is disconnected */
> +#define	SNDRV_PCM_STATE_DRAINING_PAUSED	((__force snd_pcm_state_t) 7) /* stream is paused during draining*/
> +#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 8) /* hardware is suspended */
> +#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 9) /* hardware is disconnected */
>  #define	SNDRV_PCM_STATE_LAST		SNDRV_PCM_STATE_DISCONNECTED
>  
>  enum {
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 7184265c0b0d..46fdf50d5c00 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -31,7 +31,7 @@
>  #include <sound/compress_params.h>
>  
>  
> -#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 0)
> +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 1)
>  /**
>   * struct snd_compressed_buffer - compressed buffer
>   * @fragment_size: size of buffer fragment in bytes
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 0e53f6f31916..58fbe0d99431 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>  	case SNDRV_PCM_STATE_RUNNING:
>  	case SNDRV_PCM_STATE_DRAINING:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
>  		break;
>  	default:
> @@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
>  	case SNDRV_PCM_STATE_RUNNING:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		if (avail >= stream->runtime->fragment_size)
>  			retval = snd_compr_get_poll(stream);
>  		break;
> @@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>  {
>  	int retval;
>  
> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> +	switch (stream->runtime->state) {
> +	case SNDRV_PCM_STATE_RUNNING:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +		break;
> +	case SNDRV_PCM_STATE_DRAINING:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> +		if (!retval)
> +			stream->runtime->state =
> +				SNDRV_PCM_STATE_DRAINING_PAUSED;
> +		break;
> +	default:
>  		return -EPERM;
> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> -	if (!retval)
> -		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +	}
>  	return retval;
>  }
>  
> @@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
>  {
>  	int retval;
>  
> -	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
> +	switch (stream->runtime->state) {
> +	case SNDRV_PCM_STATE_PAUSED:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +		break;
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +		break;
> +	default:
>  		return -EPERM;
> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> -	if (!retval)
> -		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +	}
>  	return retval;
>  }
>  
> @@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
>  	 */
>  
>  	ret = wait_event_interruptible(stream->runtime->sleep,
> -			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
> +			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
> +			(stream->runtime->state !=
> +				SNDRV_PCM_STATE_DRAINING_PAUSED));
>  	if (ret == -ERESTARTSYS)
>  		pr_debug("wait aborted by a signal\n");
>  	else if (ret)
> @@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  	case SNDRV_PCM_STATE_SETUP:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		return -EPERM;
>  	case SNDRV_PCM_STATE_XRUN:
>  		return -EPIPE;
> @@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	case SNDRV_PCM_STATE_SETUP:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		return -EPERM;
>  	case SNDRV_PCM_STATE_XRUN:
>  		return -EPIPE;
> -- 
> 2.21.0
> 

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):
> On Thu, 08 Oct 2020 11:49:24 +0200,
> Gyeongtaek Lee wrote:
>>
>> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
>>>> The SM in kernel might be bit more convoluted so was wondering if we can
>>>> handle this in userland. The changelog for this patch says that for
>>>> test case was sending whole file, surely that is not an optimal approach.
>>>
>>> It's rather common to have to deal with very small files, even with PCM, 
>>> e.g. for notifications. It's actually a classic test case that exposes 
>>> design issues in drivers, where e.g. the last part of the notification 
>>> is not played.
>>>
>>>> Should we allow folks to send whole file to kernel and then issue
>>>> partial drain?
>>>
>>> I don't think there should be a conceptual limitation here. If the 
>>> userspace knows that the last part of the file is smaller than a 
>>> fragment it should be able to issue a drain (or partial drain if it's a 
>>> gapless solution).
>>>
>>> However now that I think of it, I am not sure what happens if the file 
>>> is smaller than a fragment. That may very well be a limitation in the 
>>> design.
>>>
>> Thanks for the comments.
>>
>> Actually, problem can be occurred with big file also.
>> Application usually requests draining after sending last frame.
>> If user clicks pause button after draining, pause will be failed
>> and the file just be played until end.
>> If application stop and start playback for this case, 
>> start of last frame will be heard again because stop sets state to SETUP,
>> and write is needed to set the state to PREPARED for start.
>> If bitrate of the file is low, time stamp will be reversed and be heard weird.
>> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
>>
>> I think that this is the time that I should share fixed patch following the comments to help the discussion.
>> Following opinions are added to the patch.
>> 1. it's be much nicer to have a new state - Takashi
> 
> Well, it wasn't me; I'm not against the new state *iff* it would end
> up with cleaner code.  Admittedly, the new state can be more
> "consistent" regarding the state transition.  If we allow the PAUSE
> state during DRAINING, it'll lead to multiple states after resuming
> the pause.
> 
>> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
>> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
>>
>> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
>> with a change in asound.h not in compress_offload.h.
>> Should I increase SNDRV_PCM_VERSION also?
> 
> Yes, if we really add the PCM state, it's definitely needed.
> 
>> And what happened if I request back-porting a patch which changes VERSION to stables?
> 
> If we introduce the new change, it must be safe to the old kernels,
> too.  The problem is only about the compatibility of the user-space
> program, not about the kernel.
> 
> 
> HOWEVER: I'm still concerned by the addition of a new PCM state.
> Jaroslav suggested two steps approach, (1) first add the state only in
> the uapi header, then use (2) the new state actually.  But, this
> doesn't help much, simply because the step 1 won't catch real bugs.
> 
> Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
> guess most of code can be compiled fine.  So, at the step 1, no one
> notices it and bothered, either.  But, at the step 2, you'll hit a
> problem.
> 
> Adding a new state is something like to add a new color to the traffic
> signal.  In some countries, the car turning right at a crossing
> doesn't have to stop at a red signal.  Suppose that we want to control
> it, and change the international rule by introducing a new color (say
> magenta) signal to stop the car turning right.  That'll be a big
> confusion because most drivers are trained for only red, green and
> yellow signals.
> 
> Similarly, if we add a new PCM state, every program code that deals
> with the PCM state may be confused by the new state.  It has to be
> reviewed and corrected manually, because it's no syntax problem the
> compiler may catch.

If there is a handshake between both side, this problem is gone. We can just
add another flag / ioctl / whatever to activate the new behaviour.

						Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Fri, 09 Oct 2020 19:43:40 +0200,
Jaroslav Kysela wrote:
> 
> Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):
> > On Thu, 08 Oct 2020 11:49:24 +0200,
> > Gyeongtaek Lee wrote:
> >>
> >> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
> >>>> The SM in kernel might be bit more convoluted so was wondering if we can
> >>>> handle this in userland. The changelog for this patch says that for
> >>>> test case was sending whole file, surely that is not an optimal approach.
> >>>
> >>> It's rather common to have to deal with very small files, even with PCM, 
> >>> e.g. for notifications. It's actually a classic test case that exposes 
> >>> design issues in drivers, where e.g. the last part of the notification 
> >>> is not played.
> >>>
> >>>> Should we allow folks to send whole file to kernel and then issue
> >>>> partial drain?
> >>>
> >>> I don't think there should be a conceptual limitation here. If the 
> >>> userspace knows that the last part of the file is smaller than a 
> >>> fragment it should be able to issue a drain (or partial drain if it's a 
> >>> gapless solution).
> >>>
> >>> However now that I think of it, I am not sure what happens if the file 
> >>> is smaller than a fragment. That may very well be a limitation in the 
> >>> design.
> >>>
> >> Thanks for the comments.
> >>
> >> Actually, problem can be occurred with big file also.
> >> Application usually requests draining after sending last frame.
> >> If user clicks pause button after draining, pause will be failed
> >> and the file just be played until end.
> >> If application stop and start playback for this case, 
> >> start of last frame will be heard again because stop sets state to SETUP,
> >> and write is needed to set the state to PREPARED for start.
> >> If bitrate of the file is low, time stamp will be reversed and be heard weird.
> >> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
> >>
> >> I think that this is the time that I should share fixed patch following the comments to help the discussion.
> >> Following opinions are added to the patch.
> >> 1. it's be much nicer to have a new state - Takashi
> > 
> > Well, it wasn't me; I'm not against the new state *iff* it would end
> > up with cleaner code.  Admittedly, the new state can be more
> > "consistent" regarding the state transition.  If we allow the PAUSE
> > state during DRAINING, it'll lead to multiple states after resuming
> > the pause.
> > 
> >> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
> >> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
> >>
> >> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
> >> with a change in asound.h not in compress_offload.h.
> >> Should I increase SNDRV_PCM_VERSION also?
> > 
> > Yes, if we really add the PCM state, it's definitely needed.
> > 
> >> And what happened if I request back-porting a patch which changes VERSION to stables?
> > 
> > If we introduce the new change, it must be safe to the old kernels,
> > too.  The problem is only about the compatibility of the user-space
> > program, not about the kernel.
> > 
> > 
> > HOWEVER: I'm still concerned by the addition of a new PCM state.
> > Jaroslav suggested two steps approach, (1) first add the state only in
> > the uapi header, then use (2) the new state actually.  But, this
> > doesn't help much, simply because the step 1 won't catch real bugs.
> > 
> > Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
> > guess most of code can be compiled fine.  So, at the step 1, no one
> > notices it and bothered, either.  But, at the step 2, you'll hit a
> > problem.
> > 
> > Adding a new state is something like to add a new color to the traffic
> > signal.  In some countries, the car turning right at a crossing
> > doesn't have to stop at a red signal.  Suppose that we want to control
> > it, and change the international rule by introducing a new color (say
> > magenta) signal to stop the car turning right.  That'll be a big
> > confusion because most drivers are trained for only red, green and
> > yellow signals.
> > 
> > Similarly, if we add a new PCM state, every program code that deals
> > with the PCM state may be confused by the new state.  It has to be
> > reviewed and corrected manually, because it's no syntax problem the
> > compiler may catch.
> 
> If there is a handshake between both side, this problem is gone. We can just
> add another flag / ioctl / whatever to activate the new behaviour.

That's another tricky part.  We do have already some handshake in
alsa-lib to determine the supported protocol.  However, if a code in
question is outside that influence, we can't ensure that all belonging
components understand the new one.  e.g. if a program uses an
intermediate library, it's free from alsa-lib changes.  Or, imagine
some plugin.

If this were a change of the API function, we may have a better
control.  We may provide different versioned symbols in the worst
case, too.  But, an enum is essentially hard-coded, so we have no
direct influence once after it's compiled.


thanks,

Takashi

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

Hi Takashi, Jaroslav,

On 10-10-20, 11:08, Takashi Iwai wrote:
> On Fri, 09 Oct 2020 19:43:40 +0200,
> Jaroslav Kysela wrote:
> > 
> > Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):
> > > On Thu, 08 Oct 2020 11:49:24 +0200,
> > > Gyeongtaek Lee wrote:
> > >>
> > >> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
> > >>>> The SM in kernel might be bit more convoluted so was wondering if we can
> > >>>> handle this in userland. The changelog for this patch says that for
> > >>>> test case was sending whole file, surely that is not an optimal approach.
> > >>>
> > >>> It's rather common to have to deal with very small files, even with PCM, 
> > >>> e.g. for notifications. It's actually a classic test case that exposes 
> > >>> design issues in drivers, where e.g. the last part of the notification 
> > >>> is not played.
> > >>>
> > >>>> Should we allow folks to send whole file to kernel and then issue
> > >>>> partial drain?
> > >>>
> > >>> I don't think there should be a conceptual limitation here. If the 
> > >>> userspace knows that the last part of the file is smaller than a 
> > >>> fragment it should be able to issue a drain (or partial drain if it's a 
> > >>> gapless solution).
> > >>>
> > >>> However now that I think of it, I am not sure what happens if the file 
> > >>> is smaller than a fragment. That may very well be a limitation in the 
> > >>> design.
> > >>>
> > >> Thanks for the comments.
> > >>
> > >> Actually, problem can be occurred with big file also.
> > >> Application usually requests draining after sending last frame.
> > >> If user clicks pause button after draining, pause will be failed
> > >> and the file just be played until end.
> > >> If application stop and start playback for this case, 
> > >> start of last frame will be heard again because stop sets state to SETUP,
> > >> and write is needed to set the state to PREPARED for start.
> > >> If bitrate of the file is low, time stamp will be reversed and be heard weird.
> > >> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
> > >>
> > >> I think that this is the time that I should share fixed patch following the comments to help the discussion.
> > >> Following opinions are added to the patch.
> > >> 1. it's be much nicer to have a new state - Takashi
> > > 
> > > Well, it wasn't me; I'm not against the new state *iff* it would end
> > > up with cleaner code.  Admittedly, the new state can be more
> > > "consistent" regarding the state transition.  If we allow the PAUSE
> > > state during DRAINING, it'll lead to multiple states after resuming
> > > the pause.
> > > 
> > >> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
> > >> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
> > >>
> > >> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
> > >> with a change in asound.h not in compress_offload.h.
> > >> Should I increase SNDRV_PCM_VERSION also?
> > > 
> > > Yes, if we really add the PCM state, it's definitely needed.
> > > 
> > >> And what happened if I request back-porting a patch which changes VERSION to stables?
> > > 
> > > If we introduce the new change, it must be safe to the old kernels,
> > > too.  The problem is only about the compatibility of the user-space
> > > program, not about the kernel.
> > > 
> > > 
> > > HOWEVER: I'm still concerned by the addition of a new PCM state.
> > > Jaroslav suggested two steps approach, (1) first add the state only in
> > > the uapi header, then use (2) the new state actually.  But, this
> > > doesn't help much, simply because the step 1 won't catch real bugs.
> > > 
> > > Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
> > > guess most of code can be compiled fine.  So, at the step 1, no one
> > > notices it and bothered, either.  But, at the step 2, you'll hit a
> > > problem.
> > > 
> > > Adding a new state is something like to add a new color to the traffic
> > > signal.  In some countries, the car turning right at a crossing
> > > doesn't have to stop at a red signal.  Suppose that we want to control
> > > it, and change the international rule by introducing a new color (say
> > > magenta) signal to stop the car turning right.  That'll be a big
> > > confusion because most drivers are trained for only red, green and
> > > yellow signals.
> > > 
> > > Similarly, if we add a new PCM state, every program code that deals
> > > with the PCM state may be confused by the new state.  It has to be
> > > reviewed and corrected manually, because it's no syntax problem the
> > > compiler may catch.
> > 
> > If there is a handshake between both side, this problem is gone. We can just
> > add another flag / ioctl / whatever to activate the new behaviour.
> 
> That's another tricky part.  We do have already some handshake in
> alsa-lib to determine the supported protocol.  However, if a code in
> question is outside that influence, we can't ensure that all belonging
> components understand the new one.  e.g. if a program uses an
> intermediate library, it's free from alsa-lib changes.  Or, imagine
> some plugin.
> 
> If this were a change of the API function, we may have a better
> control.  We may provide different versioned symbols in the worst
> case, too.  But, an enum is essentially hard-coded, so we have no
> direct influence once after it's compiled.

So what if we add another state but keep it in kernel (hidden from
userspace)...?

Right now tinycompress does not make use of PCM streams, kernel handles
these. I am not aware of any other implementation.

So if the scope if within compress then it might work...

Thanks
-- 
~Vinod

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Mon, 12 Oct 2020 07:25:25 +0200,
Vinod Koul wrote:
> 
> Hi Takashi, Jaroslav,
> 
> On 10-10-20, 11:08, Takashi Iwai wrote:
> > On Fri, 09 Oct 2020 19:43:40 +0200,
> > Jaroslav Kysela wrote:
> > > 
> > > Dne 09. 10. 20 v 17:13 Takashi Iwai napsal(a):
> > > > On Thu, 08 Oct 2020 11:49:24 +0200,
> > > > Gyeongtaek Lee wrote:
> > > >>
> > > >> On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
> > > >>>> The SM in kernel might be bit more convoluted so was wondering if we can
> > > >>>> handle this in userland. The changelog for this patch says that for
> > > >>>> test case was sending whole file, surely that is not an optimal approach.
> > > >>>
> > > >>> It's rather common to have to deal with very small files, even with PCM, 
> > > >>> e.g. for notifications. It's actually a classic test case that exposes 
> > > >>> design issues in drivers, where e.g. the last part of the notification 
> > > >>> is not played.
> > > >>>
> > > >>>> Should we allow folks to send whole file to kernel and then issue
> > > >>>> partial drain?
> > > >>>
> > > >>> I don't think there should be a conceptual limitation here. If the 
> > > >>> userspace knows that the last part of the file is smaller than a 
> > > >>> fragment it should be able to issue a drain (or partial drain if it's a 
> > > >>> gapless solution).
> > > >>>
> > > >>> However now that I think of it, I am not sure what happens if the file 
> > > >>> is smaller than a fragment. That may very well be a limitation in the 
> > > >>> design.
> > > >>>
> > > >> Thanks for the comments.
> > > >>
> > > >> Actually, problem can be occurred with big file also.
> > > >> Application usually requests draining after sending last frame.
> > > >> If user clicks pause button after draining, pause will be failed
> > > >> and the file just be played until end.
> > > >> If application stop and start playback for this case, 
> > > >> start of last frame will be heard again because stop sets state to SETUP,
> > > >> and write is needed to set the state to PREPARED for start.
> > > >> If bitrate of the file is low, time stamp will be reversed and be heard weird.
> > > >> I also hope this problem can be handled in userspace easily but I couldn't find a way for now.
> > > >>
> > > >> I think that this is the time that I should share fixed patch following the comments to help the discussion.
> > > >> Following opinions are added to the patch.
> > > >> 1. it's be much nicer to have a new state - Takashi
> > > > 
> > > > Well, it wasn't me; I'm not against the new state *iff* it would end
> > > > up with cleaner code.  Admittedly, the new state can be more
> > > > "consistent" regarding the state transition.  If we allow the PAUSE
> > > > state during DRAINING, it'll lead to multiple states after resuming
> > > > the pause.
> > > > 
> > > >> 2. We can add this state to asound.h so the user space can be updated. - Jaroslav
> > > >> 3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav
> > > >>
> > > >> I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
> > > >> with a change in asound.h not in compress_offload.h.
> > > >> Should I increase SNDRV_PCM_VERSION also?
> > > > 
> > > > Yes, if we really add the PCM state, it's definitely needed.
> > > > 
> > > >> And what happened if I request back-porting a patch which changes VERSION to stables?
> > > > 
> > > > If we introduce the new change, it must be safe to the old kernels,
> > > > too.  The problem is only about the compatibility of the user-space
> > > > program, not about the kernel.
> > > > 
> > > > 
> > > > HOWEVER: I'm still concerned by the addition of a new PCM state.
> > > > Jaroslav suggested two steps approach, (1) first add the state only in
> > > > the uapi header, then use (2) the new state actually.  But, this
> > > > doesn't help much, simply because the step 1 won't catch real bugs.
> > > > 
> > > > Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
> > > > guess most of code can be compiled fine.  So, at the step 1, no one
> > > > notices it and bothered, either.  But, at the step 2, you'll hit a
> > > > problem.
> > > > 
> > > > Adding a new state is something like to add a new color to the traffic
> > > > signal.  In some countries, the car turning right at a crossing
> > > > doesn't have to stop at a red signal.  Suppose that we want to control
> > > > it, and change the international rule by introducing a new color (say
> > > > magenta) signal to stop the car turning right.  That'll be a big
> > > > confusion because most drivers are trained for only red, green and
> > > > yellow signals.
> > > > 
> > > > Similarly, if we add a new PCM state, every program code that deals
> > > > with the PCM state may be confused by the new state.  It has to be
> > > > reviewed and corrected manually, because it's no syntax problem the
> > > > compiler may catch.
> > > 
> > > If there is a handshake between both side, this problem is gone. We can just
> > > add another flag / ioctl / whatever to activate the new behaviour.
> > 
> > That's another tricky part.  We do have already some handshake in
> > alsa-lib to determine the supported protocol.  However, if a code in
> > question is outside that influence, we can't ensure that all belonging
> > components understand the new one.  e.g. if a program uses an
> > intermediate library, it's free from alsa-lib changes.  Or, imagine
> > some plugin.
> > 
> > If this were a change of the API function, we may have a better
> > control.  We may provide different versioned symbols in the worst
> > case, too.  But, an enum is essentially hard-coded, so we have no
> > direct influence once after it's compiled.
> 
> So what if we add another state but keep it in kernel (hidden from
> userspace)...?

That's fine, then it's just a kernel's business, and it should be
determined which one makes the code better.

But, there are things to be considered, though:

- SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
  This indicates that the type has to be defined in that way
  explicitly.

- Having a value over SNDRV_PCM_STATE_LAST internally is hackish.

> Right now tinycompress does not make use of PCM streams, kernel handles
> these. I am not aware of any other implementation.
> 
> So if the scope if within compress then it might work...

Yes.  But currently the API uses SND_PCM_* even for the compress
stuff.  Changing this value means to have influence on PCM, even if
PCM stuff doesn't use it yet.  (At least you'd need to increase
SND_PCM_STATE_LAST, for example.)

That said, if we want to change only for compress API by assuming that
the impact must be negligible, the first step would be to move from
SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
should be compatible, but this has to be changed at first.  Then you
can introduce a new value there.


thanks,

Takashi

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

On 12-10-20, 09:01, Takashi Iwai wrote:
> On Mon, 12 Oct 2020 07:25:25 +0200,

> > So what if we add another state but keep it in kernel (hidden from
> > userspace)...?
> 
> That's fine, then it's just a kernel's business, and it should be
> determined which one makes the code better.
> 
> But, there are things to be considered, though:
> 
> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
>   This indicates that the type has to be defined in that way
>   explicitly.
> 
> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
> 
> > Right now tinycompress does not make use of PCM streams, kernel handles
> > these. I am not aware of any other implementation.
> > 
> > So if the scope if within compress then it might work...
> 
> Yes.  But currently the API uses SND_PCM_* even for the compress
> stuff.  Changing this value means to have influence on PCM, even if
> PCM stuff doesn't use it yet.  (At least you'd need to increase
> SND_PCM_STATE_LAST, for example.)
> 
> That said, if we want to change only for compress API by assuming that
> the impact must be negligible, the first step would be to move from
> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
> should be compatible, but this has to be changed at first.  Then you
> can introduce a new value there.

I think that sounds reasonable to me, we should not have used
SNDRV_PCM_STATE_* in the first place and long term fix for this should
be SNDRV_COMPRESS_STATE_

I will cook a patch for this

Thanks
-- 
~Vinod

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a):
> On 12-10-20, 09:01, Takashi Iwai wrote:
>> On Mon, 12 Oct 2020 07:25:25 +0200,
> 
>>> So what if we add another state but keep it in kernel (hidden from
>>> userspace)...?
>>
>> That's fine, then it's just a kernel's business, and it should be
>> determined which one makes the code better.
>>
>> But, there are things to be considered, though:
>>
>> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
>>   This indicates that the type has to be defined in that way
>>   explicitly.
>>
>> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
>>
>>> Right now tinycompress does not make use of PCM streams, kernel handles
>>> these. I am not aware of any other implementation.
>>>
>>> So if the scope if within compress then it might work...
>>
>> Yes.  But currently the API uses SND_PCM_* even for the compress
>> stuff.  Changing this value means to have influence on PCM, even if
>> PCM stuff doesn't use it yet.  (At least you'd need to increase
>> SND_PCM_STATE_LAST, for example.)
>>
>> That said, if we want to change only for compress API by assuming that
>> the impact must be negligible, the first step would be to move from
>> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
>> should be compatible, but this has to be changed at first.  Then you
>> can introduce a new value there.
> 
> I think that sounds reasonable to me, we should not have used
> SNDRV_PCM_STATE_* in the first place and long term fix for this should
> be SNDRV_COMPRESS_STATE_
> 
> I will cook a patch for this

Although the impact is not high, I do think that we should enable the new
behaviour conditionally (when the user space asks for it) even if the state
values are split. I think that the whole thread is about 'how to extend the
current APIs'. The hidden way is really not so nice.

Unfortunately, there are no reserved fields in the snd_compr_params structure
for this, but I see the similarity with the 'no_wake_mode' field which
controls the driver behaviour.

					Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

On 12-10-20, 15:29, Jaroslav Kysela wrote:
> Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a):
> > On 12-10-20, 09:01, Takashi Iwai wrote:
> >> On Mon, 12 Oct 2020 07:25:25 +0200,
> > 
> >>> So what if we add another state but keep it in kernel (hidden from
> >>> userspace)...?
> >>
> >> That's fine, then it's just a kernel's business, and it should be
> >> determined which one makes the code better.
> >>
> >> But, there are things to be considered, though:
> >>
> >> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
> >>   This indicates that the type has to be defined in that way
> >>   explicitly.
> >>
> >> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
> >>
> >>> Right now tinycompress does not make use of PCM streams, kernel handles
> >>> these. I am not aware of any other implementation.
> >>>
> >>> So if the scope if within compress then it might work...
> >>
> >> Yes.  But currently the API uses SND_PCM_* even for the compress
> >> stuff.  Changing this value means to have influence on PCM, even if
> >> PCM stuff doesn't use it yet.  (At least you'd need to increase
> >> SND_PCM_STATE_LAST, for example.)
> >>
> >> That said, if we want to change only for compress API by assuming that
> >> the impact must be negligible, the first step would be to move from
> >> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
> >> should be compatible, but this has to be changed at first.  Then you
> >> can introduce a new value there.
> > 
> > I think that sounds reasonable to me, we should not have used
> > SNDRV_PCM_STATE_* in the first place and long term fix for this should
> > be SNDRV_COMPRESS_STATE_
> > 
> > I will cook a patch for this
> 
> Although the impact is not high, I do think that we should enable the new
> behaviour conditionally (when the user space asks for it) even if the state
> values are split. I think that the whole thread is about 'how to extend the
> current APIs'. The hidden way is really not so nice.
> 
> Unfortunately, there are no reserved fields in the snd_compr_params structure
> for this, but I see the similarity with the 'no_wake_mode' field which
> controls the driver behaviour.

I was not really thinking of exporting the states to userspace.
Tinycompress does not use it, I do not see any uses of it to enable
userspace with it.. Do you think it should be exposed? If so why..?

Worst case we add an ioctl to query the state.. the state transitions
are anyway result of control ops on the stream

Btw what was the motivation for pcm to expose the stream states..?

-- 
~Vinod

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 12. 10. 20 v 15:55 Vinod Koul napsal(a):
> On 12-10-20, 15:29, Jaroslav Kysela wrote:
>> Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a):
>>> On 12-10-20, 09:01, Takashi Iwai wrote:
>>>> On Mon, 12 Oct 2020 07:25:25 +0200,
>>>
>>>>> So what if we add another state but keep it in kernel (hidden from
>>>>> userspace)...?
>>>>
>>>> That's fine, then it's just a kernel's business, and it should be
>>>> determined which one makes the code better.
>>>>
>>>> But, there are things to be considered, though:
>>>>
>>>> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
>>>>   This indicates that the type has to be defined in that way
>>>>   explicitly.
>>>>
>>>> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
>>>>
>>>>> Right now tinycompress does not make use of PCM streams, kernel handles
>>>>> these. I am not aware of any other implementation.
>>>>>
>>>>> So if the scope if within compress then it might work...
>>>>
>>>> Yes.  But currently the API uses SND_PCM_* even for the compress
>>>> stuff.  Changing this value means to have influence on PCM, even if
>>>> PCM stuff doesn't use it yet.  (At least you'd need to increase
>>>> SND_PCM_STATE_LAST, for example.)
>>>>
>>>> That said, if we want to change only for compress API by assuming that
>>>> the impact must be negligible, the first step would be to move from
>>>> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
>>>> should be compatible, but this has to be changed at first.  Then you
>>>> can introduce a new value there.
>>>
>>> I think that sounds reasonable to me, we should not have used
>>> SNDRV_PCM_STATE_* in the first place and long term fix for this should
>>> be SNDRV_COMPRESS_STATE_
>>>
>>> I will cook a patch for this
>>
>> Although the impact is not high, I do think that we should enable the new
>> behaviour conditionally (when the user space asks for it) even if the state
>> values are split. I think that the whole thread is about 'how to extend the
>> current APIs'. The hidden way is really not so nice.
>>
>> Unfortunately, there are no reserved fields in the snd_compr_params structure
>> for this, but I see the similarity with the 'no_wake_mode' field which
>> controls the driver behaviour.
> 
> I was not really thinking of exporting the states to userspace.
> Tinycompress does not use it, I do not see any uses of it to enable
> userspace with it.. Do you think it should be exposed? If so why..?

I don't think that it's required to expose the state for the compressed API to
add this new feature. I just talk about to activate the new feature
conditionally. The question is how to extend the API now.

> Worst case we add an ioctl to query the state.. the state transitions
> are anyway result of control ops on the stream
> 
> Btw what was the motivation for pcm to expose the stream states..?

The driver may change the state when underrun / overrun or an I/O error occurs
and there's also mmap write/read mode, so the traditional read/write with an
error code handling does not work here. Also, the user space should know the
state anyway, so it's better to have all parts synced.

						Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Mon, 12 Oct 2020 16:10:10 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 10. 20 v 15:55 Vinod Koul napsal(a):
> > On 12-10-20, 15:29, Jaroslav Kysela wrote:
> >> Dne 12. 10. 20 v 14:24 Vinod Koul napsal(a):
> >>> On 12-10-20, 09:01, Takashi Iwai wrote:
> >>>> On Mon, 12 Oct 2020 07:25:25 +0200,
> >>>
> >>>>> So what if we add another state but keep it in kernel (hidden from
> >>>>> userspace)...?
> >>>>
> >>>> That's fine, then it's just a kernel's business, and it should be
> >>>> determined which one makes the code better.
> >>>>
> >>>> But, there are things to be considered, though:
> >>>>
> >>>> - SNDRV_PCM_STATE_* is defined as snd_pcm_state_t with __bitwise.
> >>>>   This indicates that the type has to be defined in that way
> >>>>   explicitly.
> >>>>
> >>>> - Having a value over SNDRV_PCM_STATE_LAST internally is hackish.
> >>>>
> >>>>> Right now tinycompress does not make use of PCM streams, kernel handles
> >>>>> these. I am not aware of any other implementation.
> >>>>>
> >>>>> So if the scope if within compress then it might work...
> >>>>
> >>>> Yes.  But currently the API uses SND_PCM_* even for the compress
> >>>> stuff.  Changing this value means to have influence on PCM, even if
> >>>> PCM stuff doesn't use it yet.  (At least you'd need to increase
> >>>> SND_PCM_STATE_LAST, for example.)
> >>>>
> >>>> That said, if we want to change only for compress API by assuming that
> >>>> the impact must be negligible, the first step would be to move from
> >>>> SND_PCM_STATE_* to the own state, SND_COMPRESS_STATE_*.  The values
> >>>> should be compatible, but this has to be changed at first.  Then you
> >>>> can introduce a new value there.
> >>>
> >>> I think that sounds reasonable to me, we should not have used
> >>> SNDRV_PCM_STATE_* in the first place and long term fix for this should
> >>> be SNDRV_COMPRESS_STATE_
> >>>
> >>> I will cook a patch for this
> >>
> >> Although the impact is not high, I do think that we should enable the new
> >> behaviour conditionally (when the user space asks for it) even if the state
> >> values are split. I think that the whole thread is about 'how to extend the
> >> current APIs'. The hidden way is really not so nice.
> >>
> >> Unfortunately, there are no reserved fields in the snd_compr_params structure
> >> for this, but I see the similarity with the 'no_wake_mode' field which
> >> controls the driver behaviour.
> > 
> > I was not really thinking of exporting the states to userspace.
> > Tinycompress does not use it, I do not see any uses of it to enable
> > userspace with it.. Do you think it should be exposed? If so why..?
> 
> I don't think that it's required to expose the state for the compressed API to
> add this new feature. I just talk about to activate the new feature
> conditionally. The question is how to extend the API now.

The PCM API has an ioctl (SNDRV_PCM_IOCTL_USER_PVERSION) to tell
kernel which protocol version the user-space can talk with.  It's a
reverse direction from SNDRV_PCM_IOCTL_PVERSION, and with this
mechanism, the kernel can determine whether a specific feature can be
enabled to user-space or not.

I guess the compress API can introduce the same mechanism, if any
conditional behavior is really mandatory.

But, I doubt whether we really need to care about that; as mentioned
earlier, there is little to change from the user-space side.  It just
pause or resume.  The only difference is the resume target, and
honestly speaking, there is no interest in it from user-space side.
And, the rest is about the kernel internal, and this can be really
done in the way of the original patch.  The flow is quite simple and
understandable...


> > Worst case we add an ioctl to query the state.. the state transitions
> > are anyway result of control ops on the stream
> > 
> > Btw what was the motivation for pcm to expose the stream states..?
> 
> The driver may change the state when underrun / overrun or an I/O error occurs
> and there's also mmap write/read mode, so the traditional read/write with an
> error code handling does not work here. Also, the user space should know the
> state anyway, so it's better to have all parts synced.

For PCM, yes, the state query is a must from user-space applications,
and that's the reason I've been arguing.


thanks,

Takashi

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 12. 10. 20 v 16:21 Takashi Iwai napsal(a):

> But, I doubt whether we really need to care about that; as mentioned
> earlier, there is little to change from the user-space side.  It just
> pause or resume.  The only difference is the resume target, and
> honestly speaking, there is no interest in it from user-space side.
> And, the rest is about the kernel internal, and this can be really
> done in the way of the original patch.  The flow is quite simple and
> understandable...

The core compress code already uses the state mechanism (although internally).

Also, it's really unclear if all drivers were checked, if the pause triggers
can be called from the drain state (I know it's another point, but the drivers
should probably offer a flag that they support this). And why to call the
pause release callback when there's no pause (drain + release ioctl instead
drain + pause + release ioctl)? It's a clear midlevel code fault. This
protection should be there not in the hw drivers.

I refer the original patch:
  https://lore.kernel.org/alsa-devel/000c01d69585$228db6b0$67a92410$@samsung.com/

						Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Mon, 12 Oct 2020 16:46:56 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 10. 20 v 16:21 Takashi Iwai napsal(a):
> 
> > But, I doubt whether we really need to care about that; as mentioned
> > earlier, there is little to change from the user-space side.  It just
> > pause or resume.  The only difference is the resume target, and
> > honestly speaking, there is no interest in it from user-space side.
> > And, the rest is about the kernel internal, and this can be really
> > done in the way of the original patch.  The flow is quite simple and
> > understandable...
> 
> The core compress code already uses the state mechanism (although internally).
> 
> Also, it's really unclear if all drivers were checked, if the pause triggers
> can be called from the drain state (I know it's another point, but the drivers
> should probably offer a flag that they support this). And why to call the
> pause release callback when there's no pause (drain + release ioctl instead
> drain + pause + release ioctl)? It's a clear midlevel code fault. This
> protection should be there not in the hw drivers.
> 
> I refer the original patch:
>   https://lore.kernel.org/alsa-devel/000c01d69585$228db6b0$67a92410$@samsung.com/

Point taken, and yes, this should be handled conditionally only for
the drivers that do support such a mode.


Takashi

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On 10/08/20 06:49 PM, Takashi Iwai wrote:
>On Mon, 12 Oct 2020 16:46:56 +0200,
>Jaroslav Kysela wrote:
>> 
>> Dne 12. 10. 20 v 16:21 Takashi Iwai napsal(a):
>> 
>> > But, I doubt whether we really need to care about that; as mentioned
>> > earlier, there is little to change from the user-space side.  It just
>> > pause or resume.  The only difference is the resume target, and
>> > honestly speaking, there is no interest in it from user-space side.
>> > And, the rest is about the kernel internal, and this can be really
>> > done in the way of the original patch.  The flow is quite simple and
>> > understandable...
>> 
>> The core compress code already uses the state mechanism (although internally).
>> 
>> Also, it's really unclear if all drivers were checked, if the pause triggers
>> can be called from the drain state (I know it's another point, but the drivers
>> should probably offer a flag that they support this). And why to call the
>> pause release callback when there's no pause (drain + release ioctl instead
>> drain + pause + release ioctl)? It's a clear midlevel code fault. This
>> protection should be there not in the hw drivers.
>> 
>> I refer the original patch:
>>   https://lore.kernel.org/alsa-devel/000c01d69585$228db6b0$67a92410$@samsung.com/
>
>Point taken, and yes, this should be handled conditionally only for
>the drivers that do support such a mode.
>
Thanks for all your advices.
I updated the patch from original as all your comments.

1. API 'snd_compr_use_pause_in_draining' is added to allow pause during draining
   only when the hw driver allow it explicitly.
2. Flag 'pause_in_draining' is added to the struct snd_compr_stream to check whether
   the stream has been paused during draining when snd_compr_resume is called.
   I need this change in the older stable kernels. 
   However, changing states is too complicated and even dangerous for not only kernel
   but also userspace as this discussion.
   So, I'd like to suggest to use this simple flag first, and then apply more patch
   which fixes this patch to use states for compress after SNDRV_COMPRESS_STATE_*
   is applied to the ALSA mainline by Vinod.

Following is the fixed patch. 
If there is more points should be fixed or discussed, just let me know.


With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first.
If pause, resume are allowed during draining, user experience can be
enhanced.

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/sound/compress_driver.h | 17 +++++++++++++
 sound/core/compress_offload.c   | 44 +++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 70cbc5095e72..5a0d6737de5e 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -67,6 +67,7 @@ struct snd_compr_runtime {
  * @metadata_set: metadata set flag, true when set
  * @next_track: has userspace signal next track transition, true when set
  * @partial_drain: undergoing partial_drain for stream, true when set
+ * @pause_in_draining: paused during draining state, true when set
  * @private_data: pointer to DSP private data
  * @dma_buffer: allocated buffer if any
  */
@@ -80,6 +81,7 @@ struct snd_compr_stream {
 	bool metadata_set;
 	bool next_track;
 	bool partial_drain;
+	bool pause_in_draining;
 	void *private_data;
 	struct snd_dma_buffer dma_buffer;
 };
@@ -142,6 +144,7 @@ struct snd_compr_ops {
  * @direction: Playback or capture direction
  * @lock: device lock
  * @device: device id
+ * @use_pause_in_draining: allow pause in draining, true when set
  */
 struct snd_compr {
 	const char *name;
@@ -152,6 +155,7 @@ struct snd_compr {
 	unsigned int direction;
 	struct mutex lock;
 	int device;
+	bool use_pause_in_draining;
 #ifdef CONFIG_SND_VERBOSE_PROCFS
 	/* private: */
 	char id[64];
@@ -166,6 +170,19 @@ int snd_compress_deregister(struct snd_compr *device);
 int snd_compress_new(struct snd_card *card, int device,
 			int type, const char *id, struct snd_compr *compr);
 
+/**
+ * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
+ * @substream: compress substream to set
+ *
+ * Allow pause and resume in draining state.
+ * Only HW driver supports this transition can call this API.
+ */
+static inline void snd_compr_use_pause_in_draining(
+					struct snd_compr_stream *substream)
+{
+	substream->device->use_pause_in_draining = 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 0e53f6f31916..a071485383ed 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -708,11 +708,24 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->pause_in_draining = true;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +733,25 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining ||
+		    !stream->pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->pause_in_draining = false;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -767,6 +794,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 		/* clear flags and stop any drain wait */
 		stream->partial_drain = false;
 		stream->metadata_set = false;
+		stream->pause_in_draining = false;
 		snd_compr_drain_notify(stream);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;

base-commit: 865c50e1d279671728c2936cb7680eb89355eeea
-- 
2.21.0
>
>Takashi
>

[PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first because most of applications call draining
after sending last frame to the kernel.
If pause, resume are allowed during draining, user experience can be
enhanced.
To prevent malfunction in HW drivers which don't support pause
during draining, pause during draining will only work if HW driver
enable this feature explicitly by calling
snd_compr_use_pause_in_draining().

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/sound/compress_driver.h | 17 +++++++++++++
 sound/core/compress_offload.c   | 44 +++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 70cbc5095e72..5a0d6737de5e 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -67,6 +67,7 @@ struct snd_compr_runtime {
  * @metadata_set: metadata set flag, true when set
  * @next_track: has userspace signal next track transition, true when set
  * @partial_drain: undergoing partial_drain for stream, true when set
+ * @pause_in_draining: paused during draining state, true when set
  * @private_data: pointer to DSP private data
  * @dma_buffer: allocated buffer if any
  */
@@ -80,6 +81,7 @@ struct snd_compr_stream {
 	bool metadata_set;
 	bool next_track;
 	bool partial_drain;
+	bool pause_in_draining;
 	void *private_data;
 	struct snd_dma_buffer dma_buffer;
 };
@@ -142,6 +144,7 @@ struct snd_compr_ops {
  * @direction: Playback or capture direction
  * @lock: device lock
  * @device: device id
+ * @use_pause_in_draining: allow pause in draining, true when set
  */
 struct snd_compr {
 	const char *name;
@@ -152,6 +155,7 @@ struct snd_compr {
 	unsigned int direction;
 	struct mutex lock;
 	int device;
+	bool use_pause_in_draining;
 #ifdef CONFIG_SND_VERBOSE_PROCFS
 	/* private: */
 	char id[64];
@@ -166,6 +170,19 @@ int snd_compress_deregister(struct snd_compr *device);
 int snd_compress_new(struct snd_card *card, int device,
 			int type, const char *id, struct snd_compr *compr);
 
+/**
+ * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
+ * @substream: compress substream to set
+ *
+ * Allow pause and resume in draining state.
+ * Only HW driver supports this transition can call this API.
+ */
+static inline void snd_compr_use_pause_in_draining(
+					struct snd_compr_stream *substream)
+{
+	substream->device->use_pause_in_draining = 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 0e53f6f31916..a071485383ed 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -708,11 +708,24 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->pause_in_draining = true;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +733,25 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining ||
+		    !stream->pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->pause_in_draining = false;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -767,6 +794,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 		/* clear flags and stop any drain wait */
 		stream->partial_drain = false;
 		stream->metadata_set = false;
+		stream->pause_in_draining = false;
 		snd_compr_drain_notify(stream);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
-- 
2.21.0

[PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first because most of applications call draining
after sending last frame to the kernel.
If pause, resume are allowed during draining, user experience can be
enhanced.
To prevent malfunction in HW drivers which don't support pause
during draining, pause during draining will only work if HW driver
enable this feature explicitly by calling
snd_compr_use_pause_in_draining().

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/sound/compress_driver.h | 17 +++++++++++++
 sound/core/compress_offload.c   | 44 +++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 70cbc5095e72..5a0d6737de5e 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -67,6 +67,7 @@ struct snd_compr_runtime {
  * @metadata_set: metadata set flag, true when set
  * @next_track: has userspace signal next track transition, true when set
  * @partial_drain: undergoing partial_drain for stream, true when set
+ * @pause_in_draining: paused during draining state, true when set
  * @private_data: pointer to DSP private data
  * @dma_buffer: allocated buffer if any
  */
@@ -80,6 +81,7 @@ struct snd_compr_stream {
 	bool metadata_set;
 	bool next_track;
 	bool partial_drain;
+	bool pause_in_draining;
 	void *private_data;
 	struct snd_dma_buffer dma_buffer;
 };
@@ -142,6 +144,7 @@ struct snd_compr_ops {
  * @direction: Playback or capture direction
  * @lock: device lock
  * @device: device id
+ * @use_pause_in_draining: allow pause in draining, true when set
  */
 struct snd_compr {
 	const char *name;
@@ -152,6 +155,7 @@ struct snd_compr {
 	unsigned int direction;
 	struct mutex lock;
 	int device;
+	bool use_pause_in_draining;
 #ifdef CONFIG_SND_VERBOSE_PROCFS
 	/* private: */
 	char id[64];
@@ -166,6 +170,19 @@ int snd_compress_deregister(struct snd_compr *device);
 int snd_compress_new(struct snd_card *card, int device,
 			int type, const char *id, struct snd_compr *compr);
 
+/**
+ * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
+ * @substream: compress substream to set
+ *
+ * Allow pause and resume in draining state.
+ * Only HW driver supports this transition can call this API.
+ */
+static inline void snd_compr_use_pause_in_draining(
+					struct snd_compr_stream *substream)
+{
+	substream->device->use_pause_in_draining = 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 0e53f6f31916..a071485383ed 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -708,11 +708,24 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->pause_in_draining = true;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +733,25 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		if (!stream->device->use_pause_in_draining ||
+		    !stream->pause_in_draining)
+			return -EPERM;
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->pause_in_draining = false;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -767,6 +794,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 		/* clear flags and stop any drain wait */
 		stream->partial_drain = false;
 		stream->metadata_set = false;
+		stream->pause_in_draining = false;
 		snd_compr_drain_notify(stream);
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
-- 
2.21.0

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Mon, 26 Oct 2020 10:18:38 +0100,
Gyeongtaek Lee wrote:
> 
> With a stream with low bitrate, user can't pause or resume the stream
> near the end of the stream because current ALSA doesn't allow it.
> If the stream has very low bitrate enough to store whole stream into
> the buffer, user can't do anything except stop the stream and then
> restart it from the first because most of applications call draining
> after sending last frame to the kernel.
> If pause, resume are allowed during draining, user experience can be
> enhanced.
> To prevent malfunction in HW drivers which don't support pause
> during draining, pause during draining will only work if HW driver
> enable this feature explicitly by calling
> snd_compr_use_pause_in_draining().
> 
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org

Could you restart the new thread?  It's been hanging too deeply and
hard to read through.

Since it's the revised patch, please give the revision number (v2 or
such) and show what's different from the previous patches.

About the changes:

> +/**
> + * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
> + * @substream: compress substream to set
> + *
> + * Allow pause and resume in draining state.
> + * Only HW driver supports this transition can call this API.
> + */
> +static inline void snd_compr_use_pause_in_draining(
> +					struct snd_compr_stream *substream)
> +{
> +	substream->device->use_pause_in_draining = true;
> +}

How to set the flag is an open question.  A natural way would be to
set it somehow at creating the component object, but currently there
seems no way to pass any flags.


thanks,

Takashi

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On Mon, 26 Oct 2020 18:01:33 +0100, Takashi Iwai wrote:
>On Mon, 26 Oct 2020 10:18:38 +0100,
>Gyeongtaek Lee wrote:
>> 
>> With a stream with low bitrate, user can't pause or resume the stream
>> near the end of the stream because current ALSA doesn't allow it.
>> If the stream has very low bitrate enough to store whole stream into
>> the buffer, user can't do anything except stop the stream and then
>> restart it from the first because most of applications call draining
>> after sending last frame to the kernel.
>> If pause, resume are allowed during draining, user experience can be
>> enhanced.
>> To prevent malfunction in HW drivers which don't support pause
>> during draining, pause during draining will only work if HW driver
>> enable this feature explicitly by calling
>> snd_compr_use_pause_in_draining().
>> 
>> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
>> Cc: stable@vger.kernel.org
>
>Could you restart the new thread?  It's been hanging too deeply and
>hard to read through.
>
>Since it's the revised patch, please give the revision number (v2 or
>such) and show what's different from the previous patches.
>
Ok. I'll send the patch again with [PATCH v2] prefix.
>About the changes:
>
>> +/**
>> + * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
>> + * @substream: compress substream to set
>> + *
>> + * Allow pause and resume in draining state.
>> + * Only HW driver supports this transition can call this API.
>> + */
>> +static inline void snd_compr_use_pause_in_draining(
>> +					struct snd_compr_stream *substream)
>> +{
>> +	substream->device->use_pause_in_draining = true;
>> +}
>
>How to set the flag is an open question.  A natural way would be to
>set it somehow at creating the component object, but currently there
>seems no way to pass any flags.
Could you explain more about what is your concerning?
For me, calling snd_compr_use_pause_in_draining() in open() callback of 
snd_compr_ops was good enough.
I've tested it and it worked well on linux 5.4.
>
>
>thanks,
>
>Takashi
>

[PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first.
If pause and resume is allowed during draining, user experience can be
enhanced.

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
---
 sound/core/compress_offload.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..90b437f302a0 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -708,11 +708,20 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +729,20 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 

base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
-- 
2.21.0

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> With a stream with low bitrate, user can't pause or resume the stream
> near the end of the stream because current ALSA doesn't allow it.
> If the stream has very low bitrate enough to store whole stream into
> the buffer, user can't do anything except stop the stream and then
> restart it from the first.
> If pause and resume is allowed during draining, user experience can be
> enhanced.

It seems that we need a new state to handle the pause + drain condition for
this case.

With this proposed change, the pause state in drain is invisible.

					Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Pierre-Louis Bossart]

On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>> With a stream with low bitrate, user can't pause or resume the stream
>> near the end of the stream because current ALSA doesn't allow it.
>> If the stream has very low bitrate enough to store whole stream into
>> the buffer, user can't do anything except stop the stream and then
>> restart it from the first.
>> If pause and resume is allowed during draining, user experience can be
>> enhanced.
> 
> It seems that we need a new state to handle the pause + drain condition for
> this case.
> 
> With this proposed change, the pause state in drain is invisible.

Indeed it's be much nicer to have a new state, e..g 
SNDRV_PCM_STATE_DRAINING_PAUSED.

One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
this have impacts on userspace as well? We'd change the value of 
SNDRV_PCM_STATE_LAST.

RE: [PATCH] ALSA: compress: allow pause and resume during draining

[Gyeongtaek Lee]

On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>>> With a stream with low bitrate, user can't pause or resume the stream
>>> near the end of the stream because current ALSA doesn't allow it.
>>> If the stream has very low bitrate enough to store whole stream into
>>> the buffer, user can't do anything except stop the stream and then
>>> restart it from the first.
>>> If pause and resume is allowed during draining, user experience can be
>>> enhanced.
>> 
>> It seems that we need a new state to handle the pause + drain condition for
>> this case.
>> 
>> With this proposed change, the pause state in drain is invisible.
>
>Indeed it's be much nicer to have a new state, e..g 
>SNDRV_PCM_STATE_DRAINING_PAUSED.
Ok. I will add the new state.
>
>One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>this have impacts on userspace as well? We'd change the value of 
>SNDRV_PCM_STATE_LAST.
>
I also agree that adding new state and increase LAST value in the header of uapi
could be dangerous. So, I added it to comress_offload.h for now.
It could be merged into snd_pcm_state_t in someday with big changes.
Could you review the fixed patch below?


With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from first.
If pause, resume are allowed during draining, user experience can be
enhanced.

New state for pause during draining is defined in compress_offload.h for
now. If PCM_STATEs in uapi/sound/asound.h is changed, pcm libraries and
userspace application will be affected.

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/uapi/sound/compress_offload.h |  3 ++
 sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 7184265c0b0d..f30b9851d1d7 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -189,4 +189,7 @@ struct snd_compr_metadata {
 #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
 #define SND_COMPR_TRIGGER_NEXT_TRACK 8
 #define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
+
+/* FIXME move this to asound.h */
+#define	SNDRV_PCM_STATE_DRAINING_PAUSED	(SNDRV_PCM_STATE_LAST + 1)
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..58fbe0d99431 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_DRAINING:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
 		break;
 	default:
@@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		if (avail >= stream->runtime->fragment_size)
 			retval = snd_compr_get_poll(stream);
 		break;
@@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state =
+				SNDRV_PCM_STATE_DRAINING_PAUSED;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 	 */
 
 	ret = wait_event_interruptible(stream->runtime->sleep,
-			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
+			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
+			(stream->runtime->state !=
+				SNDRV_PCM_STATE_DRAINING_PAUSED));
 	if (ret == -ERESTARTSYS)
 		pr_debug("wait aborted by a signal\n");
 	else if (ret)
@@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
@@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;

base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
-- 
2.21.0

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Tue, 29 Sep 2020 03:51:35 +0200,
Gyeongtaek Lee wrote:
> 
> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
> >On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> >> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> >>> With a stream with low bitrate, user can't pause or resume the stream
> >>> near the end of the stream because current ALSA doesn't allow it.
> >>> If the stream has very low bitrate enough to store whole stream into
> >>> the buffer, user can't do anything except stop the stream and then
> >>> restart it from the first.
> >>> If pause and resume is allowed during draining, user experience can be
> >>> enhanced.
> >> 
> >> It seems that we need a new state to handle the pause + drain condition for
> >> this case.
> >> 
> >> With this proposed change, the pause state in drain is invisible.
> >
> >Indeed it's be much nicer to have a new state, e..g 
> >SNDRV_PCM_STATE_DRAINING_PAUSED.
> Ok. I will add the new state.
> >
> >One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
> >this have impacts on userspace as well? We'd change the value of 
> >SNDRV_PCM_STATE_LAST.
> >
> I also agree that adding new state and increase LAST value in the header of uapi
> could be dangerous. So, I added it to comress_offload.h for now.
> It could be merged into snd_pcm_state_t in someday with big changes.
> Could you review the fixed patch below?

Hrm, this resulted in rather more complex changes than the original
patch.  It shows that introducing yet another state is no good idea
for this particular case.

Since the possible application's behavior after this pause is as same
as the normal pause (i.e. either resuming pause or dropping), I find
it OK to take the original approach.


thanks,

Takashi

> With a stream with low bitrate, user can't pause or resume the stream
> near the end of the stream because current ALSA doesn't allow it.
> If the stream has very low bitrate enough to store whole stream into
> the buffer, user can't do anything except stop the stream and then
> restart it from first.
> If pause, resume are allowed during draining, user experience can be
> enhanced.
> 
> New state for pause during draining is defined in compress_offload.h for
> now. If PCM_STATEs in uapi/sound/asound.h is changed, pcm libraries and
> userspace application will be affected.
> 
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org
> ---
>  include/uapi/sound/compress_offload.h |  3 ++
>  sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 7184265c0b0d..f30b9851d1d7 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -189,4 +189,7 @@ struct snd_compr_metadata {
>  #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */
>  #define SND_COMPR_TRIGGER_NEXT_TRACK 8
>  #define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9
> +
> +/* FIXME move this to asound.h */
> +#define	SNDRV_PCM_STATE_DRAINING_PAUSED	(SNDRV_PCM_STATE_LAST + 1)
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 0e53f6f31916..58fbe0d99431 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>  	case SNDRV_PCM_STATE_RUNNING:
>  	case SNDRV_PCM_STATE_DRAINING:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
>  		break;
>  	default:
> @@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
>  	case SNDRV_PCM_STATE_RUNNING:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		if (avail >= stream->runtime->fragment_size)
>  			retval = snd_compr_get_poll(stream);
>  		break;
> @@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>  {
>  	int retval;
>  
> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> +	switch (stream->runtime->state) {
> +	case SNDRV_PCM_STATE_RUNNING:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +		break;
> +	case SNDRV_PCM_STATE_DRAINING:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> +		if (!retval)
> +			stream->runtime->state =
> +				SNDRV_PCM_STATE_DRAINING_PAUSED;
> +		break;
> +	default:
>  		return -EPERM;
> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> -	if (!retval)
> -		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +	}
>  	return retval;
>  }
>  
> @@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
>  {
>  	int retval;
>  
> -	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
> +	switch (stream->runtime->state) {
> +	case SNDRV_PCM_STATE_PAUSED:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +		break;
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> +		break;
> +	default:
>  		return -EPERM;
> -	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> -	if (!retval)
> -		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +	}
>  	return retval;
>  }
>  
> @@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
>  	 */
>  
>  	ret = wait_event_interruptible(stream->runtime->sleep,
> -			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
> +			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
> +			(stream->runtime->state !=
> +				SNDRV_PCM_STATE_DRAINING_PAUSED));
>  	if (ret == -ERESTARTSYS)
>  		pr_debug("wait aborted by a signal\n");
>  	else if (ret)
> @@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
>  	case SNDRV_PCM_STATE_SETUP:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		return -EPERM;
>  	case SNDRV_PCM_STATE_XRUN:
>  		return -EPIPE;
> @@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>  	case SNDRV_PCM_STATE_SETUP:
>  	case SNDRV_PCM_STATE_PREPARED:
>  	case SNDRV_PCM_STATE_PAUSED:
> +	case SNDRV_PCM_STATE_DRAINING_PAUSED:
>  		return -EPERM;
>  	case SNDRV_PCM_STATE_XRUN:
>  		return -EPIPE;
> 
> base-commit: a1b8638ba1320e6684aa98233c15255eb803fac7
> -- 
> 2.21.0
> 
> 

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 29. 09. 20 v 9:12 Takashi Iwai napsal(a):
> On Tue, 29 Sep 2020 03:51:35 +0200,
> Gyeongtaek Lee wrote:
>>
>> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>>> On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>>>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>>>>> With a stream with low bitrate, user can't pause or resume the stream
>>>>> near the end of the stream because current ALSA doesn't allow it.
>>>>> If the stream has very low bitrate enough to store whole stream into
>>>>> the buffer, user can't do anything except stop the stream and then
>>>>> restart it from the first.
>>>>> If pause and resume is allowed during draining, user experience can be
>>>>> enhanced.
>>>>
>>>> It seems that we need a new state to handle the pause + drain condition for
>>>> this case.
>>>>
>>>> With this proposed change, the pause state in drain is invisible.
>>>
>>> Indeed it's be much nicer to have a new state, e..g 
>>> SNDRV_PCM_STATE_DRAINING_PAUSED.
>> Ok. I will add the new state.
>>>
>>> One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>>> this have impacts on userspace as well? We'd change the value of 
>>> SNDRV_PCM_STATE_LAST.
>>>
>> I also agree that adding new state and increase LAST value in the header of uapi
>> could be dangerous. So, I added it to comress_offload.h for now.
>> It could be merged into snd_pcm_state_t in someday with big changes.
>> Could you review the fixed patch below?

I don't see a big problem to improve the API, but don't forget to increase the
SNDRV_COMPRESS_VERSION, so the user space apps can check for this new behaviour.

> Hrm, this resulted in rather more complex changes than the original
> patch.  It shows that introducing yet another state is no good idea
> for this particular case.

I don't think so. The states should be isolated and it's clearly a new state
and the resulted code at least gives a commented idea, what's going on. It
seems that the compress driver state is not exported to the user space at the
moment, so I would consider this extension as harmless. We can add this state
to asound.h so the user space can be updated. We may use this state for the
standard PCM devices one day, too. It makes sense to reserve it sooner than later.

BTW: Offtopic - Why compress code returns EPERM if the state is not correct?
It's not about the permissions. The EBADFD is much better code in this case.

						Jaroslav

> 
> Since the possible application's behavior after this pause is as same
> as the normal pause (i.e. either resuming pause or dropping), I find
> it OK to take the original approach.
> 
> 
> thanks,
> 
> Takashi
-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Tue, 29 Sep 2020 19:27:17 +0200,
Jaroslav Kysela wrote:
> 
> Dne 29. 09. 20 v 9:12 Takashi Iwai napsal(a):
> > On Tue, 29 Sep 2020 03:51:35 +0200,
> > Gyeongtaek Lee wrote:
> >>
> >> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
> >>> On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> >>>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> >>>>> With a stream with low bitrate, user can't pause or resume the stream
> >>>>> near the end of the stream because current ALSA doesn't allow it.
> >>>>> If the stream has very low bitrate enough to store whole stream into
> >>>>> the buffer, user can't do anything except stop the stream and then
> >>>>> restart it from the first.
> >>>>> If pause and resume is allowed during draining, user experience can be
> >>>>> enhanced.
> >>>>
> >>>> It seems that we need a new state to handle the pause + drain condition for
> >>>> this case.
> >>>>
> >>>> With this proposed change, the pause state in drain is invisible.
> >>>
> >>> Indeed it's be much nicer to have a new state, e..g 
> >>> SNDRV_PCM_STATE_DRAINING_PAUSED.
> >> Ok. I will add the new state.
> >>>
> >>> One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
> >>> this have impacts on userspace as well? We'd change the value of 
> >>> SNDRV_PCM_STATE_LAST.
> >>>
> >> I also agree that adding new state and increase LAST value in the header of uapi
> >> could be dangerous. So, I added it to comress_offload.h for now.
> >> It could be merged into snd_pcm_state_t in someday with big changes.
> >> Could you review the fixed patch below?
> 
> I don't see a big problem to improve the API, but don't forget to increase the
> SNDRV_COMPRESS_VERSION, so the user space apps can check for this new behaviour.
> 
> > Hrm, this resulted in rather more complex changes than the original
> > patch.  It shows that introducing yet another state is no good idea
> > for this particular case.
> 
> I don't think so. The states should be isolated and it's clearly a new state
> and the resulted code at least gives a commented idea, what's going on. It
> seems that the compress driver state is not exported to the user space at the
> moment, so I would consider this extension as harmless. We can add this state
> to asound.h so the user space can be updated. We may use this state for the
> standard PCM devices one day, too. It makes sense to reserve it sooner than later.

Well, adding a new state can be cumbersome sometimes. For example, the
code like below may hit a segfault out of sudden after the upgrade:

	const char *states[SNDRV_PCM_STATE_LAST + 1] = {
		[SNDRV_PCM_STATE_RUNNING] = "running",
		....
	};

	printf("current state = %s\n", states[s]);

It's not much frequent breakage, but this can give certainly some
incompatibilities even in the source code level.

That's the reason I'm reluctant to add a new state unless it's a must.
As mentioned, the expected application's behavior is just like the
normal pause state, either resuming pause or dropping.  The only case
where a new state would help for application is at most that they may
foresee beforehand which state it'll go after the resume, to drain or
to running.  If this is a must-to-have feature, we can reconsider.

> BTW: Offtopic - Why compress code returns EPERM if the state is not correct?
> It's not about the permissions. The EBADFD is much better code in this case.

Indeed that sounds inconsistent, but I'm afraid it too late to change?
Suppose some code already depending on the error code.  Who knows...


thanks,

Takashi

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 30. 09. 20 v 11:35 Takashi Iwai napsal(a):
> On Tue, 29 Sep 2020 19:27:17 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 29. 09. 20 v 9:12 Takashi Iwai napsal(a):
>>> On Tue, 29 Sep 2020 03:51:35 +0200,
>>> Gyeongtaek Lee wrote:
>>>>
>>>> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>>>>> On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>>>>>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>>>>>>> With a stream with low bitrate, user can't pause or resume the stream
>>>>>>> near the end of the stream because current ALSA doesn't allow it.
>>>>>>> If the stream has very low bitrate enough to store whole stream into
>>>>>>> the buffer, user can't do anything except stop the stream and then
>>>>>>> restart it from the first.
>>>>>>> If pause and resume is allowed during draining, user experience can be
>>>>>>> enhanced.
>>>>>>
>>>>>> It seems that we need a new state to handle the pause + drain condition for
>>>>>> this case.
>>>>>>
>>>>>> With this proposed change, the pause state in drain is invisible.
>>>>>
>>>>> Indeed it's be much nicer to have a new state, e..g 
>>>>> SNDRV_PCM_STATE_DRAINING_PAUSED.
>>>> Ok. I will add the new state.
>>>>>
>>>>> One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>>>>> this have impacts on userspace as well? We'd change the value of 
>>>>> SNDRV_PCM_STATE_LAST.
>>>>>
>>>> I also agree that adding new state and increase LAST value in the header of uapi
>>>> could be dangerous. So, I added it to comress_offload.h for now.
>>>> It could be merged into snd_pcm_state_t in someday with big changes.
>>>> Could you review the fixed patch below?
>>
>> I don't see a big problem to improve the API, but don't forget to increase the
>> SNDRV_COMPRESS_VERSION, so the user space apps can check for this new behaviour.
>>
>>> Hrm, this resulted in rather more complex changes than the original
>>> patch.  It shows that introducing yet another state is no good idea
>>> for this particular case.
>>
>> I don't think so. The states should be isolated and it's clearly a new state
>> and the resulted code at least gives a commented idea, what's going on. It
>> seems that the compress driver state is not exported to the user space at the
>> moment, so I would consider this extension as harmless. We can add this state
>> to asound.h so the user space can be updated. We may use this state for the
>> standard PCM devices one day, too. It makes sense to reserve it sooner than later.
> 
> Well, adding a new state can be cumbersome sometimes. For example, the
> code like below may hit a segfault out of sudden after the upgrade:
> 
> 	const char *states[SNDRV_PCM_STATE_LAST + 1] = {
> 		[SNDRV_PCM_STATE_RUNNING] = "running",
> 		....
> 	};
> 
> 	printf("current state = %s\n", states[s]);
> 
> It's not much frequent breakage, but this can give certainly some
> incompatibilities even in the source code level.

alsa-lib has already the correct protection for this case:

const char *snd_pcm_state_name(const snd_pcm_state_t state)
{
        if (state > SND_PCM_STATE_LAST)
                return NULL;
        return snd_pcm_state_names[state];
}

If there's no check, it's a clear bug.

> That's the reason I'm reluctant to add a new state unless it's a must.
> As mentioned, the expected application's behavior is just like the
> normal pause state, either resuming pause or dropping.  The only case
> where a new state would help for application is at most that they may
> foresee beforehand which state it'll go after the resume, to drain or
> to running.  If this is a must-to-have feature, we can reconsider.

I don't agree here. It's much better to not hide the state related transitions
even in the kernel in my eyes. For example drivers may behave differently when
they resume from running+pause or drain+pause states.

The correct SNDRV_PCM_STATE_LAST is just an implementation issue, which can be
easily solved.

					Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Takashi Iwai]

On Wed, 30 Sep 2020 11:57:45 +0200,
Jaroslav Kysela wrote:
> 
> Dne 30. 09. 20 v 11:35 Takashi Iwai napsal(a):
> > On Tue, 29 Sep 2020 19:27:17 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 29. 09. 20 v 9:12 Takashi Iwai napsal(a):
> >>> On Tue, 29 Sep 2020 03:51:35 +0200,
> >>> Gyeongtaek Lee wrote:
> >>>>
> >>>> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
> >>>>> On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
> >>>>>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
> >>>>>>> With a stream with low bitrate, user can't pause or resume the stream
> >>>>>>> near the end of the stream because current ALSA doesn't allow it.
> >>>>>>> If the stream has very low bitrate enough to store whole stream into
> >>>>>>> the buffer, user can't do anything except stop the stream and then
> >>>>>>> restart it from the first.
> >>>>>>> If pause and resume is allowed during draining, user experience can be
> >>>>>>> enhanced.
> >>>>>>
> >>>>>> It seems that we need a new state to handle the pause + drain condition for
> >>>>>> this case.
> >>>>>>
> >>>>>> With this proposed change, the pause state in drain is invisible.
> >>>>>
> >>>>> Indeed it's be much nicer to have a new state, e..g 
> >>>>> SNDRV_PCM_STATE_DRAINING_PAUSED.
> >>>> Ok. I will add the new state.
> >>>>>
> >>>>> One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
> >>>>> this have impacts on userspace as well? We'd change the value of 
> >>>>> SNDRV_PCM_STATE_LAST.
> >>>>>
> >>>> I also agree that adding new state and increase LAST value in the header of uapi
> >>>> could be dangerous. So, I added it to comress_offload.h for now.
> >>>> It could be merged into snd_pcm_state_t in someday with big changes.
> >>>> Could you review the fixed patch below?
> >>
> >> I don't see a big problem to improve the API, but don't forget to increase the
> >> SNDRV_COMPRESS_VERSION, so the user space apps can check for this new behaviour.
> >>
> >>> Hrm, this resulted in rather more complex changes than the original
> >>> patch.  It shows that introducing yet another state is no good idea
> >>> for this particular case.
> >>
> >> I don't think so. The states should be isolated and it's clearly a new state
> >> and the resulted code at least gives a commented idea, what's going on. It
> >> seems that the compress driver state is not exported to the user space at the
> >> moment, so I would consider this extension as harmless. We can add this state
> >> to asound.h so the user space can be updated. We may use this state for the
> >> standard PCM devices one day, too. It makes sense to reserve it sooner than later.
> > 
> > Well, adding a new state can be cumbersome sometimes. For example, the
> > code like below may hit a segfault out of sudden after the upgrade:
> > 
> > 	const char *states[SNDRV_PCM_STATE_LAST + 1] = {
> > 		[SNDRV_PCM_STATE_RUNNING] = "running",
> > 		....
> > 	};
> > 
> > 	printf("current state = %s\n", states[s]);
> > 
> > It's not much frequent breakage, but this can give certainly some
> > incompatibilities even in the source code level.
> 
> alsa-lib has already the correct protection for this case:
> 
> const char *snd_pcm_state_name(const snd_pcm_state_t state)
> {
>         if (state > SND_PCM_STATE_LAST)
>                 return NULL;
>         return snd_pcm_state_names[state];
> }
> 
> If there's no check, it's a clear bug.

That's not what I meant; the code I showed was just an example
implying that the addition of a new state may require the deep code
change that can't be caught by a compiler.  It may be silently
broken.

And imagine the user-space library code that contains handling of the
PCM state.  All those has to be updated as well to deal with a new
state, not only alsa-lib.

IOW, by adding a new item to an exposed attribute like PCM state, the
possibly needed change would be spread over all lib / application
code, and its influence shouldn't be underestimated.  If it were only
some internal change in alsa-lib, I won't be concerned at all.

> > That's the reason I'm reluctant to add a new state unless it's a must.
> > As mentioned, the expected application's behavior is just like the
> > normal pause state, either resuming pause or dropping.  The only case
> > where a new state would help for application is at most that they may
> > foresee beforehand which state it'll go after the resume, to drain or
> > to running.  If this is a must-to-have feature, we can reconsider.
> 
> I don't agree here. It's much better to not hide the state related transitions
> even in the kernel in my eyes. For example drivers may behave differently when
> they resume from running+pause or drain+pause states.

Yes, but that's basically the driver's business.  As mentioned, "if
this is a must-to-have feature" for applications, we'll need to
reconsider.  But it's not clear from the scenario yet.
(FWIW, if any, we may add another function to tell the after-resume
state, too; this might be even safer from the compatibility POV,
although it can be more complicated.)

> The correct SNDRV_PCM_STATE_LAST is just an implementation issue, which can be
> easily solved.

How easily solvable -- that's the question :)


thanks,

Takashi

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Jaroslav Kysela]

Dne 30. 09. 20 v 12:33 Takashi Iwai napsal(a):
> On Wed, 30 Sep 2020 11:57:45 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 30. 09. 20 v 11:35 Takashi Iwai napsal(a):
>>> On Tue, 29 Sep 2020 19:27:17 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 29. 09. 20 v 9:12 Takashi Iwai napsal(a):
>>>>> On Tue, 29 Sep 2020 03:51:35 +0200,
>>>>> Gyeongtaek Lee wrote:
>>>>>>
>>>>>> On 9/28/20 11:35 PM, Pierre-Louis Bossart wrote:
>>>>>>> On 9/28/20 6:13 AM, Jaroslav Kysela wrote:
>>>>>>>> Dne 28. 09. 20 v 12:50 Gyeongtaek Lee napsal(a):
>>>>>>>>> With a stream with low bitrate, user can't pause or resume the stream
>>>>>>>>> near the end of the stream because current ALSA doesn't allow it.
>>>>>>>>> If the stream has very low bitrate enough to store whole stream into
>>>>>>>>> the buffer, user can't do anything except stop the stream and then
>>>>>>>>> restart it from the first.
>>>>>>>>> If pause and resume is allowed during draining, user experience can be
>>>>>>>>> enhanced.
>>>>>>>>
>>>>>>>> It seems that we need a new state to handle the pause + drain condition for
>>>>>>>> this case.
>>>>>>>>
>>>>>>>> With this proposed change, the pause state in drain is invisible.
>>>>>>>
>>>>>>> Indeed it's be much nicer to have a new state, e..g 
>>>>>>> SNDRV_PCM_STATE_DRAINING_PAUSED.
>>>>>> Ok. I will add the new state.
>>>>>>>
>>>>>>> One concern is that states are defined in uapi/sound/asoc.h, so wouldn't 
>>>>>>> this have impacts on userspace as well? We'd change the value of 
>>>>>>> SNDRV_PCM_STATE_LAST.
>>>>>>>
>>>>>> I also agree that adding new state and increase LAST value in the header of uapi
>>>>>> could be dangerous. So, I added it to comress_offload.h for now.
>>>>>> It could be merged into snd_pcm_state_t in someday with big changes.
>>>>>> Could you review the fixed patch below?
>>>>
>>>> I don't see a big problem to improve the API, but don't forget to increase the
>>>> SNDRV_COMPRESS_VERSION, so the user space apps can check for this new behaviour.
>>>>
>>>>> Hrm, this resulted in rather more complex changes than the original
>>>>> patch.  It shows that introducing yet another state is no good idea
>>>>> for this particular case.
>>>>
>>>> I don't think so. The states should be isolated and it's clearly a new state
>>>> and the resulted code at least gives a commented idea, what's going on. It
>>>> seems that the compress driver state is not exported to the user space at the
>>>> moment, so I would consider this extension as harmless. We can add this state
>>>> to asound.h so the user space can be updated. We may use this state for the
>>>> standard PCM devices one day, too. It makes sense to reserve it sooner than later.
>>>
>>> Well, adding a new state can be cumbersome sometimes. For example, the
>>> code like below may hit a segfault out of sudden after the upgrade:
>>>
>>> 	const char *states[SNDRV_PCM_STATE_LAST + 1] = {
>>> 		[SNDRV_PCM_STATE_RUNNING] = "running",
>>> 		....
>>> 	};
>>>
>>> 	printf("current state = %s\n", states[s]);
>>>
>>> It's not much frequent breakage, but this can give certainly some
>>> incompatibilities even in the source code level.
>>
>> alsa-lib has already the correct protection for this case:
>>
>> const char *snd_pcm_state_name(const snd_pcm_state_t state)
>> {
>>         if (state > SND_PCM_STATE_LAST)
>>                 return NULL;
>>         return snd_pcm_state_names[state];
>> }
>>
>> If there's no check, it's a clear bug.
> 
> That's not what I meant; the code I showed was just an example
> implying that the addition of a new state may require the deep code
> change that can't be caught by a compiler.  It may be silently
> broken.
> 
> And imagine the user-space library code that contains handling of the
> PCM state.  All those has to be updated as well to deal with a new
> state, not only alsa-lib.
> 
> IOW, by adding a new item to an exposed attribute like PCM state, the
> possibly needed change would be spread over all lib / application
> code, and its influence shouldn't be underestimated.  If it were only
> some internal change in alsa-lib, I won't be concerned at all.
> 
>>> That's the reason I'm reluctant to add a new state unless it's a must.
>>> As mentioned, the expected application's behavior is just like the
>>> normal pause state, either resuming pause or dropping.  The only case
>>> where a new state would help for application is at most that they may
>>> foresee beforehand which state it'll go after the resume, to drain or
>>> to running.  If this is a must-to-have feature, we can reconsider.
>>
>> I don't agree here. It's much better to not hide the state related transitions
>> even in the kernel in my eyes. For example drivers may behave differently when
>> they resume from running+pause or drain+pause states.
> 
> Yes, but that's basically the driver's business.  As mentioned, "if
> this is a must-to-have feature" for applications, we'll need to
> reconsider.  But it's not clear from the scenario yet.
> (FWIW, if any, we may add another function to tell the after-resume
> state, too; this might be even safer from the compatibility POV,
> although it can be more complicated.)
> 
>> The correct SNDRV_PCM_STATE_LAST is just an implementation issue, which can be
>> easily solved.
> 
> How easily solvable -- that's the question :)

My proposal is reasonable - use the new state only internally in the kernel
for the moment, but update the headers and SNDRV_PCM_STATE_LAST now so the
depending code can be updated until the new state is exposed to the user
space, too. Something like future reservation. I believe that we need this
state also for the standard PCM API.

The SNDRV_PCM_STATE_LAST was added because it was supposed to be changed. It
great to keep the 100% driver compatibility but not if it forces us to do
hidden changes.

Another way is to activate the new state (and behaviour) conditionally using a
new parameter / flag or so from the user space. In this case, both sides know
what to do.

					Jaroslav

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

Re: [PATCH] ALSA: compress: allow pause and resume during draining

[Vinod Koul]

On 30-09-20, 11:35, Takashi Iwai wrote:
> On Tue, 29 Sep 2020 19:27:17 +0200,
> Jaroslav Kysela wrote:
> > BTW: Offtopic - Why compress code returns EPERM if the state is not correct?
> > It's not about the permissions. The EBADFD is much better code in this case.

That would be me ;-)

> Indeed that sounds inconsistent, but I'm afraid it too late to change?
> Suppose some code already depending on the error code.  Who knows...

The probability of that seems lesser ;D ... There are no public upstream
users who would care about this. Only public use is QC dragon board and
we use tinyplay, which does not care.

Downstream Android HAL would care but chances of them looking at this
error code are less (i will check with Qualcomm HAL to see)...

Should we try fixing it?

-- 
~Vinod