Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

On 02/04/2025 11:52, Jaroslav Kysela wrote:
>>>> Actually, this patch follows the same pattern, too.  It calls
>>>> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
>>>> to the suspend action immediately that sets to SUSPENDED.
>>>
>>> The previous state (suspended_state) will be confusing from the
>>> application with the proposed patch, because there will be RUNNING
>>> state instead PAUSED. This previous state is exported via API.
>>
>> No, it won't happen.  The condition of the new behavior is only when
>> SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
>> hence no state recovery happens after resume.  From the application
>> POV, it won't change.
> 
> The suspended_state can be obtained in snd_pcm_status64(). With Peter's
> patch, there will be RUNNING instead PAUSED, don't?

Yes, that is true, but it does not matter.
if the SNDRV_PCM_INFO_RESUME is not set, on resume nothing is going to
be done, the state remains SUSPENDED and the suspended_substate also
retained, so:

The stream was in RUNNING, after suspend:
state = SUSPENDED, suspended_state = RUNNING
After resume:
state = SUSPENDED, suspended_state = RUNNING
The the stream goes to xrun and got restarted.

The stream was in RUNNING, after suspend:
Without my patch / with my patch:
state = SUSPENDED, suspended_state = PAUSED / RUNNING
After resume:
state = SUSPENDED, suspended_state = RUNNING / RUNNING

The stream remains "not running" as resume is not supported and
application knows that it had left the stream paused.

When it tries to PAUSE_RELEASE the stream, in both case the core will
look at the state, which is SUSPENDED and thus the pause release fails
(it is not in paused state), so in both cases we go under a restart
(suspended_state is reset, ignored).

>>>> That's another possibility, yes.  Though, IMO, it makes the
>>>> pause-handling a bit more cumbersome.  With Peter's proposal, the
>>>> pause state change is always paired, so the driver doesn't consider
>>>> about that too much; that's the biggest merit.
>>>
>>> It depends, if we agree that releasing pause is a extra thing to do
>>> when we know that this step is just to minimize state changes for
>>> drivers.
>>>
>>> For my view, the drivers may also receive those triggers:
>>>
>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
>>> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
>>> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
>>>
>>> This will allow to handle all states properly without any side effects
>>> (like short unwanted DMA transfers).
>>>
>>> The drivers should probably activate those triggers conditionally to
>>> not break current state sequences.
>>
>> Hmm, that looks too complex, IMO.
>>
>> Another slightly more straightforward fix would be just to allow the
>> direct state transition from PAUSED to any state.  Then the remaining
>> piece is all about driver implementations.  And, this can be
>> conditionally operated, e.g. only when some extra flag is set.  When
>> no flag is set, we keep applying PAUSE_RELEASE before the transition
>> like now, including Peter's patch, that can work generically (but not
>> always ideally).
> 
> It looks also feasible. My proposal just allows the straight state
> identification from the trigger value in the drivers.

We could also add a flag that the driver can say that 'I know what I'm
doing' and in that case send just simple SUSPEND and trust the driver
that it really knows what it is doing.
However if the driver does not support RESUME, then things get a bit
complicated. I'm not entirely sure what drivers do when they support
RESUME and they went to suspend with paused state...

But, this is not that simple in ASoC, as there we can have at least four
drivers involved (machine, platform, cpu dai, codec), all of them has to
support this new fine graded state dependent trigger, right?

-- 
Péter