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

[Vinod Koul <vkoul@kernel.org>]

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