[alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

[Takashi Iwai]

On Thu, 21 Nov 2019 22:17:41 +0100,
Sridharan, Ranjani wrote:
> 
> On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai <tiwai at suse.de> wrote:
> 
>     On Thu, 21 Nov 2019 21:46:17 +0100,
>     Sridharan, Ranjani wrote:
>     >
>     >     >
>     >     > Hi Takashi,
>     >     >
>     >     > Sorry the stress tests took a while. 
>     >     > As we discussed earlier, adding the sync_stop() op didnt quite
>     help the
>     >     SOF
>     >     > driver in removing the delayed work for snd_pcm_period_elapsed(). 
>     >   
>     >     Yeah, that's understandable.  If the stop operation itself needs
>     some
>     >     serialization, sync_stop() won't influence at all.
>     >   
>     >     However, now after these discussions, I have some concerns in the
>     >     current code:
>     >   
>     >     - The async work started by schedule_work() may be executed
>     >       (literally) immediately.  So if the timing or the serialization
>     >       matters, it doesn't guarantee at all.  The same level of
>     concurrency
>     >       can happen at any time.
>     >   
>     >     - The period_elapsed work might be pending at prepare or other
>     >       operation;
>     >       the async work means also that it doesn't guarantee its execution
>     in
>     >       time, and it might be delayed much, and the PCM core might go to
>     >       prepare or other state even before the work is executed.
>     >   
>     >     The second point can be fixed easily now with sync_stop.  You can
>     just
>     >     put flush_work() in sync_stop in addition to synchronize_irq().
>     >   
>     >     But the first point is still unclear.  More exactly, which operation
>     >     does it conflict?  Does it the playback drain?  Then it might take
>     >     very long (up to seconds) to block the next operation?
>     >
>     > Hi Takashi,
>     >
>     > As I understand the original intention for adding the
>     period_elapsed_work()
>     > was  that snd_pcm_period_elapsed() could cause a STOP trigger while the
>     > current IPC interrupt is still being handled.
>     > In this case, the STOP trigger generates an IPC to the DSP but the host
>     never
>     > misses the IPC response from the DSP because it is still handling the
>     previous
>     > interrupt.
>    
>     OK, that makes sense.  So the issue is that the trigger stop itself
>     requires the ack via the interrupt and it can't be caught because it's
>     being called from the irq handler itself.
>    
>     In that case, though, another solution would be to make the trigger-
>     stop an async work (but conditionally) while processing the normal
>     period_elapsed in the irq handler.  That is, set some flag before
>     calling snd_pcm_period_elapsed(), and in the trigger-stop, check the
>     flag.  If the flag is set, schedule the work and return.  And, you'll
>     sync this async work with sync_stop().  In that way, the period
>     handling is processed without any delay more lightly.
> 
> OK, that makes sense. Thanks for the suggestion.
> Regarding your previous comment about adding flush_work() to the sync_stop()
> op, would that still be required?

Yes, that's needed no matter which way is used; the pending work must
be synced at some point.


Takashi