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

From: "Sridharan, Ranjani" <ranjani.sridharan@intel.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field
Date: Thu, 21 Nov 2019 13:17:41 -0800	[thread overview]
Message-ID: <CAFQqKeU4pA_t=veRm0635pZmxpn=C9sNPPUEHkgavynPYxqwFg@mail.gmail.com> (raw)
In-Reply-To: <s5hftihrkzs.wl-tiwai@suse.de>

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
> >
> >
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel