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

On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai tiwai@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?

Thanks, Ranjani

thanks,

Takashi

...

Adding Keyon who added this change to add more and clarify your concerns.

Thanks, Ranjani

thanks,

Takashi