| Message ID | 20200610100729.362-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ALSA: compress: Fix gapless playback state machine | expand |
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); > } >
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); >> } >> > >
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..
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); > } >
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
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 >
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; }
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
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
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
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 :-)
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); }
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(-)