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 <[email protected]>
> ---
>
> 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 <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
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
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 <[email protected]>
>> ---
>>
>> 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 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
>
--
Jaroslav Kysela <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
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
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 <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
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
--
~Vinod
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