[RFC PATCH] ALSA: compress: Fix gapless playback state machine

[RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Srinivas Kandagatla]

For gapless playback call to snd_compr_drain_notify() after
partial drain should put the state to SNDRV_PCM_STATE_RUNNING
rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
process the buffers for new track.

With existing code, if we are playing 3 tracks in gapless, after
partial drain finished on previous track 1 the state is set to
SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
after data write. With this state calls to snd_compr_next_track() and
few other calls will fail as they expect the state to be in
SNDRV_PCM_STATE_RUNNING.

Here is the sequence of events and state transitions:

1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING

Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress functions (v6)")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---

I wonder who did gapless work on upstream so far?

 include/sound/compress_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 6ce8effa0b12..eabac33864c2 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -182,7 +182,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 	if (snd_BUG_ON(!stream))
 		return;
 
-	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+	stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
 
 	wake_up(&stream->runtime->sleep);
 }
-- 
2.21.0

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Jaroslav Kysela]

Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> For gapless playback call to snd_compr_drain_notify() after
> partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> process the buffers for new track.
> 
> With existing code, if we are playing 3 tracks in gapless, after
> partial drain finished on previous track 1 the state is set to
> SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> after data write. With this state calls to snd_compr_next_track() and
> few other calls will fail as they expect the state to be in
> SNDRV_PCM_STATE_RUNNING.
> 
> Here is the sequence of events and state transitions:
> 
> 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING


The snd_compr_drain_notify() is called only from snd_compr_stop(). Something 
is missing in this sequence?

					Jaroslav



> 
> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress functions (v6)")
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> 
> I wonder who did gapless work on upstream so far?
> 
>   include/sound/compress_driver.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 6ce8effa0b12..eabac33864c2 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -182,7 +182,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
>   	if (snd_BUG_ON(!stream))
>   		return;
>   
> -	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> +	stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>   
>   	wake_up(&stream->runtime->sleep);
>   }
> 


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

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Srinivas Kandagatla]

On 10/06/2020 11:40, Jaroslav Kysela wrote:
> Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
>> For gapless playback call to snd_compr_drain_notify() after
>> partial drain should put the state to SNDRV_PCM_STATE_RUNNING
>> rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
>> process the buffers for new track.
>>
>> With existing code, if we are playing 3 tracks in gapless, after
>> partial drain finished on previous track 1 the state is set to
>> SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
>> after data write. With this state calls to snd_compr_next_track() and
>> few other calls will fail as they expect the state to be in
>> SNDRV_PCM_STATE_RUNNING.
>>
>> Here is the sequence of events and state transitions:
>>
>> 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
>> 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
>> 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
>> 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
>> 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
>> 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
>> 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
>> 8. set_metadata (Track 3), no state change, state = 
>> SNDRV_PCM_STATE_PREPARED
>> 9. set_next_track (Track 3), !! FAILURE as state != 
>> SNDRV_PCM_STATE_RUNNING
> 
> 
> The snd_compr_drain_notify() is called only from snd_compr_stop(). 
> Something is missing in this sequence?

snd_compr_drain_notify() can also be called by drivers to notify when 
partial drain is finished. In this case its called from qcom compress 
driver.

--srini

> 
>                      Jaroslav
> 
> 
> 
>>
>> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other 
>> compress functions (v6)")
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>
>> I wonder who did gapless work on upstream so far?
>>
>>   include/sound/compress_driver.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/sound/compress_driver.h 
>> b/include/sound/compress_driver.h
>> index 6ce8effa0b12..eabac33864c2 100644
>> --- a/include/sound/compress_driver.h
>> +++ b/include/sound/compress_driver.h
>> @@ -182,7 +182,7 @@ static inline void snd_compr_drain_notify(struct 
>> snd_compr_stream *stream)
>>       if (snd_BUG_ON(!stream))
>>           return;
>> -    stream->runtime->state = SNDRV_PCM_STATE_SETUP;
>> +    stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>>       wake_up(&stream->runtime->sleep);
>>   }
>>
> 
> 

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Vinod Koul]

On 10-06-20, 12:40, Jaroslav Kysela wrote:
> Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > For gapless playback call to snd_compr_drain_notify() after
> > partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> > rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> > process the buffers for new track.
> > 
> > With existing code, if we are playing 3 tracks in gapless, after
> > partial drain finished on previous track 1 the state is set to
> > SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> > after data write. With this state calls to snd_compr_next_track() and
> > few other calls will fail as they expect the state to be in
> > SNDRV_PCM_STATE_RUNNING.
> > 
> > Here is the sequence of events and state transitions:
> > 
> > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> 
> 
> The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
> is missing in this sequence?

It is supposed to be invoked by driver when partial drain is complete..
both intel and sprd driver are calling this. snd_compr_stop is stop
while draining case so legit

Somehow not sure how it got missed earlier, but this seem a decent fix
but we still need to check all the states here..

-- 
~Vinod

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Charles Keepax]

On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
> On 10-06-20, 12:40, Jaroslav Kysela wrote:
> > Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > > For gapless playback call to snd_compr_drain_notify() after
> > > partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> > > rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> > > process the buffers for new track.
> > > 
> > > With existing code, if we are playing 3 tracks in gapless, after
> > > partial drain finished on previous track 1 the state is set to
> > > SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> > > after data write. With this state calls to snd_compr_next_track() and
> > > few other calls will fail as they expect the state to be in
> > > SNDRV_PCM_STATE_RUNNING.
> > > 
> > > Here is the sequence of events and state transitions:
> > > 
> > > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> > 
> > 
> > The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
> > is missing in this sequence?
> 
> It is supposed to be invoked by driver when partial drain is complete..
> both intel and sprd driver are calling this. snd_compr_stop is stop
> while draining case so legit
> 

Not sure I follow this statement, could you elaborate a bit?
snd_compr_stop putting the state to RUNNING seems fundamentally
broken to me, the whole point of snd_compr_stop is to take the
state out of RUNNING.

Thanks,
Charles

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Jaroslav Kysela]

Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
> On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
>> On 10-06-20, 12:40, Jaroslav Kysela wrote:
>>> Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
>>>> For gapless playback call to snd_compr_drain_notify() after
>>>> partial drain should put the state to SNDRV_PCM_STATE_RUNNING
>>>> rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
>>>> process the buffers for new track.
>>>>
>>>> With existing code, if we are playing 3 tracks in gapless, after
>>>> partial drain finished on previous track 1 the state is set to
>>>> SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
>>>> after data write. With this state calls to snd_compr_next_track() and
>>>> few other calls will fail as they expect the state to be in
>>>> SNDRV_PCM_STATE_RUNNING.
>>>>
>>>> Here is the sequence of events and state transitions:
>>>>
>>>> 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
>>>> 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
>>>> 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
>>>> 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
>>>> 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
>>>> 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
>>>> 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
>>>> 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
>>>> 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
>>>
>>>
>>> The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
>>> is missing in this sequence?
>>
>> It is supposed to be invoked by driver when partial drain is complete..
>> both intel and sprd driver are calling this. snd_compr_stop is stop
>> while draining case so legit
>>
> 
> Not sure I follow this statement, could you elaborate a bit?
> snd_compr_stop putting the state to RUNNING seems fundamentally
> broken to me, the whole point of snd_compr_stop is to take the
> state out of RUNNING.

Yes. I agree. It seems that the acknowledge for the partial drain should be 
handled differently.

				Jaroslav

> 
> Thanks,
> Charles
> 


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

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Vinod Koul]

On 11-06-20, 11:09, Jaroslav Kysela wrote:
> Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
> > On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
> > > On 10-06-20, 12:40, Jaroslav Kysela wrote:
> > > > Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > > > > For gapless playback call to snd_compr_drain_notify() after
> > > > > partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> > > > > rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> > > > > process the buffers for new track.
> > > > > 
> > > > > With existing code, if we are playing 3 tracks in gapless, after
> > > > > partial drain finished on previous track 1 the state is set to
> > > > > SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> > > > > after data write. With this state calls to snd_compr_next_track() and
> > > > > few other calls will fail as they expect the state to be in
> > > > > SNDRV_PCM_STATE_RUNNING.
> > > > > 
> > > > > Here is the sequence of events and state transitions:
> > > > > 
> > > > > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > > > > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > > > > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > > > > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > > > > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > > > > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > > > > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> > > > 
> > > > 
> > > > The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
> > > > is missing in this sequence?
> > > 
> > > It is supposed to be invoked by driver when partial drain is complete..
> > > both intel and sprd driver are calling this. snd_compr_stop is stop
> > > while draining case so legit
> > > 
> > 
> > Not sure I follow this statement, could you elaborate a bit?
> > snd_compr_stop putting the state to RUNNING seems fundamentally
> > broken to me, the whole point of snd_compr_stop is to take the
> > state out of RUNNING.
> 
> Yes. I agree. It seems that the acknowledge for the partial drain should be
> handled differently.

Yeah sorry I overlooked that case and was thinking of it being invoked
from driver!

Yes this would make the snd_compr_stop() behave incorrectly.. so this
cant be done as proposed.

But we still need to set the draining stream state properly and I am
thinking below now:

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 509290f2efa8..9aba851732d7 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -929,7 +929,9 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
        }
 
        stream->next_track = false;
-       return snd_compress_wait_for_drain(stream);
+       retval = snd_compress_wait_for_drain(stream);
+       stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+       return retval;
 }
 
-- 
~Vinod

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Jaroslav Kysela]

Dne 11. 06. 20 v 11:44 Vinod Koul napsal(a):
> On 11-06-20, 11:09, Jaroslav Kysela wrote:
>> Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
>>> On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
>>>> On 10-06-20, 12:40, Jaroslav Kysela wrote:
>>>>> Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
>>>>>> For gapless playback call to snd_compr_drain_notify() after
>>>>>> partial drain should put the state to SNDRV_PCM_STATE_RUNNING
>>>>>> rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
>>>>>> process the buffers for new track.
>>>>>>
>>>>>> With existing code, if we are playing 3 tracks in gapless, after
>>>>>> partial drain finished on previous track 1 the state is set to
>>>>>> SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
>>>>>> after data write. With this state calls to snd_compr_next_track() and
>>>>>> few other calls will fail as they expect the state to be in
>>>>>> SNDRV_PCM_STATE_RUNNING.
>>>>>>
>>>>>> Here is the sequence of events and state transitions:
>>>>>>
>>>>>> 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
>>>>>> 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
>>>>>> 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
>>>>>> 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
>>>>>> 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
>>>>>> 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
>>>>>> 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
>>>>>> 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
>>>>>> 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
>>>>>
>>>>>
>>>>> The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
>>>>> is missing in this sequence?
>>>>
>>>> It is supposed to be invoked by driver when partial drain is complete..
>>>> both intel and sprd driver are calling this. snd_compr_stop is stop
>>>> while draining case so legit
>>>>
>>>
>>> Not sure I follow this statement, could you elaborate a bit?
>>> snd_compr_stop putting the state to RUNNING seems fundamentally
>>> broken to me, the whole point of snd_compr_stop is to take the
>>> state out of RUNNING.
>>
>> Yes. I agree. It seems that the acknowledge for the partial drain should be
>> handled differently.
> 
> Yeah sorry I overlooked that case and was thinking of it being invoked
> from driver!
> 
> Yes this would make the snd_compr_stop() behave incorrectly.. so this
> cant be done as proposed.
> 
> But we still need to set the draining stream state properly and I am
> thinking below now:
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 509290f2efa8..9aba851732d7 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -929,7 +929,9 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>          }
>   
>          stream->next_track = false;
> -       return snd_compress_wait_for_drain(stream);
> +       retval = snd_compress_wait_for_drain(stream);
> +       stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +       return retval;
>   }

I see a race possibility when the last track is too small and the driver 
signals the end-of-track twice. In this case the partial drain should not end 
with the running state. It would be probably better to separate partial / last 
track acknowledgements.

					Jaroslav

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

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Vinod Koul]

On 11-06-20, 12:40, Jaroslav Kysela wrote:
> Dne 11. 06. 20 v 11:44 Vinod Koul napsal(a):
> > On 11-06-20, 11:09, Jaroslav Kysela wrote:
> > > Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
> > > > On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
> > > > > On 10-06-20, 12:40, Jaroslav Kysela wrote:
> > > > > > Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > > > > > > For gapless playback call to snd_compr_drain_notify() after
> > > > > > > partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> > > > > > > rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> > > > > > > process the buffers for new track.
> > > > > > > 
> > > > > > > With existing code, if we are playing 3 tracks in gapless, after
> > > > > > > partial drain finished on previous track 1 the state is set to
> > > > > > > SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> > > > > > > after data write. With this state calls to snd_compr_next_track() and
> > > > > > > few other calls will fail as they expect the state to be in
> > > > > > > SNDRV_PCM_STATE_RUNNING.
> > > > > > > 
> > > > > > > Here is the sequence of events and state transitions:
> > > > > > > 
> > > > > > > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > > > > > > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > > > > > > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > > > > > > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > > > > > > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > > > > > > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > > > > > > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> > > > > > 
> > > > > > 
> > > > > > The snd_compr_drain_notify() is called only from snd_compr_stop(). Something
> > > > > > is missing in this sequence?
> > > > > 
> > > > > It is supposed to be invoked by driver when partial drain is complete..
> > > > > both intel and sprd driver are calling this. snd_compr_stop is stop
> > > > > while draining case so legit
> > > > > 
> > > > 
> > > > Not sure I follow this statement, could you elaborate a bit?
> > > > snd_compr_stop putting the state to RUNNING seems fundamentally
> > > > broken to me, the whole point of snd_compr_stop is to take the
> > > > state out of RUNNING.
> > > 
> > > Yes. I agree. It seems that the acknowledge for the partial drain should be
> > > handled differently.
> > 
> > Yeah sorry I overlooked that case and was thinking of it being invoked
> > from driver!
> > 
> > Yes this would make the snd_compr_stop() behave incorrectly.. so this
> > cant be done as proposed.
> > 
> > But we still need to set the draining stream state properly and I am
> > thinking below now:
> > 
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index 509290f2efa8..9aba851732d7 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -929,7 +929,9 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> >          }
> >          stream->next_track = false;
> > -       return snd_compress_wait_for_drain(stream);
> > +       retval = snd_compress_wait_for_drain(stream);
> > +       stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> > +       return retval;
> >   }
> 
> I see a race possibility when the last track is too small and the driver
> signals the end-of-track twice. In this case the partial drain should not
> end with the running state. It would be probably better to separate partial
> / last track acknowledgements.

I completely agree that we should have separate acknowledgements here,
and going to rethink all state transitions for gapless here..

Thanks for the help
-- 
~Vinod

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Charles Keepax]

On Thu, Jun 11, 2020 at 03:14:23PM +0530, Vinod Koul wrote:
> On 11-06-20, 11:09, Jaroslav Kysela wrote:
> > Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
> > > On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
> > > > On 10-06-20, 12:40, Jaroslav Kysela wrote:
> > > > > Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > > > > > Here is the sequence of events and state transitions:
> > > > > > 
> > > > > > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > > > > > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > > > > > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > > > > > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > > > > > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > > > > > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > > > > > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> Yeah sorry I overlooked that case and was thinking of it being invoked
> from driver!
> 
> Yes this would make the snd_compr_stop() behave incorrectly.. so this
> cant be done as proposed.
> 
> But we still need to set the draining stream state properly and I am
> thinking below now:
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 509290f2efa8..9aba851732d7 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -929,7 +929,9 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
>         }
>  
>         stream->next_track = false;
> -       return snd_compress_wait_for_drain(stream);
> +       retval = snd_compress_wait_for_drain(stream);
> +       stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +       return retval;

This is looking better, I think you probably need to make the
switch to RUNNING conditional on snd_compress_wait_for_drain
succeeding, as the state should remain in DRAINING if we are
interrupted or some such.

I also have a slight concern that since
snd_compress_wait_for_drain, releases the lock the set_next_track
could come in before the state is moved to RUNNING, but I guess
from user-spaces perspective that is the same as it coming in
whilst the state is still DRAINING, so should be handled fine?

Thanks,
Charles

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Vinod Koul]

On 11-06-20, 10:42, Charles Keepax wrote:
> On Thu, Jun 11, 2020 at 03:14:23PM +0530, Vinod Koul wrote:
> > On 11-06-20, 11:09, Jaroslav Kysela wrote:
> > > Dne 11. 06. 20 v 10:46 Charles Keepax napsal(a):
> > > > On Wed, Jun 10, 2020 at 04:28:20PM +0530, Vinod Koul wrote:
> > > > > On 10-06-20, 12:40, Jaroslav Kysela wrote:
> > > > > > Dne 10. 06. 20 v 12:07 Srinivas Kandagatla napsal(a):
> > > > > > > Here is the sequence of events and state transitions:
> > > > > > > 
> > > > > > > 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> > > > > > > 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> > > > > > > 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> > > > > > > 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> > > > > > > 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > > 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> > > > > > > 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> > > > > > > 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> > > > > > > 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> > Yeah sorry I overlooked that case and was thinking of it being invoked
> > from driver!
> > 
> > Yes this would make the snd_compr_stop() behave incorrectly.. so this
> > cant be done as proposed.
> > 
> > But we still need to set the draining stream state properly and I am
> > thinking below now:
> > 
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index 509290f2efa8..9aba851732d7 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -929,7 +929,9 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
> >         }
> >  
> >         stream->next_track = false;
> > -       return snd_compress_wait_for_drain(stream);
> > +       retval = snd_compress_wait_for_drain(stream);
> > +       stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> > +       return retval;
> 
> This is looking better, I think you probably need to make the
> switch to RUNNING conditional on snd_compress_wait_for_drain
> succeeding, as the state should remain in DRAINING if we are
> interrupted or some such.

Hmmm, only interrupt would be terminate, so yes we should not do running
conditionally here..

> I also have a slight concern that since
> snd_compress_wait_for_drain, releases the lock the set_next_track
> could come in before the state is moved to RUNNING, but I guess
> from user-spaces perspective that is the same as it coming in
> whilst the state is still DRAINING, so should be handled fine?

yeah userspace is blocked on this call, till signalling for partial
drain is done. So it should work. But next_track 'should' be signalled
before this, but yes we need to recheck this logic here and ensure no
gaps are left in gapless :-)

-- 
~Vinod

Re: [RFC PATCH] ALSA: compress: Fix gapless playback state machine

[Pierre-Louis Bossart]

On 6/10/20 5:07 AM, Srinivas Kandagatla wrote:
> For gapless playback call to snd_compr_drain_notify() after
> partial drain should put the state to SNDRV_PCM_STATE_RUNNING
> rather than SNDRV_PCM_STATE_SETUP as the driver is ready to
> process the buffers for new track.
> 
> With existing code, if we are playing 3 tracks in gapless, after

The gapless playback only deals with transitions between two tracks of 
identical format. I am not sure what the reference to 3 tracks means.

> partial drain finished on previous track 1 the state is set to
> SNDRV_PCM_STATE_SETUP which is then moved to SNDRV_PCM_STATE_PREPARED
> after data write. With this state calls to snd_compr_next_track() and
> few other calls will fail as they expect the state to be in
> SNDRV_PCM_STATE_RUNNING.
> 
> Here is the sequence of events and state transitions:
> 
> 1. set_params (Track 1), state =  SNDRV_PCM_STATE_SETUP
> 2. set_metadata (Track 1), no state change, state = SNDRV_PCM_STATE_SETUP
> 3. fill and trigger start (Track 1), state = SNDRV_PCM_STATE_RUNNING
> 4. set_next_track (Track 2), state = SNDRV_PCM_STATE_RUNNING
> 5. partial_drain (Track 1), state = SNDRV_PCM_STATE_SETUP
> 6  snd_compr_drain_notify (Track 1), state = SNDRV_PCM_STATE_SETUP
> 7. fill data (Track 2), state = SNDRV_PCM_STATE_PREPARED
> 8. set_metadata (Track 3), no state change, state = SNDRV_PCM_STATE_PREPARED
> 9. set_next_track (Track 3), !! FAILURE as state != SNDRV_PCM_STATE_RUNNING
> 
> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress functions (v6)")
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> 
> I wonder who did gapless work on upstream so far?

IIRC the only users of gapless playback are Android platforms, where the 
HAL deals with the transitions. I am not aware of any plain vanilla 
Linux solution.

> 
>   include/sound/compress_driver.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 6ce8effa0b12..eabac33864c2 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -182,7 +182,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
>   	if (snd_BUG_ON(!stream))
>   		return;
>   
> -	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> +	stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
>   
>   	wake_up(&stream->runtime->sleep);
>   }
>