[alsa-devel] async between dmaengine_pcm_dma_complete and snd_pcm_release

[Vinod Koul]

On Thu, Oct 10, 2013 at 07:53:33PM +0200, Lars-Peter Clausen wrote:
> On 10/10/2013 06:10 PM, Vinod Koul wrote:
> >On Thu, Oct 10, 2013 at 09:46:34AM +0200, Lars-Peter Clausen wrote:
> >
> >+ Dan
> >
> >>[...]
> >>>
> >>>>>>On the other hand that last part could get tricky as the
> >>>>>>dmaengine_terminate_all() might be call from within the callback.
> >>>>>It's tricky indeed in case xrun happens. we should avoid possible deadlock.
> >>>>
> >>>>I think we'll eventually need to versions of dmaengine_terminate_all(). A
> >>>>sync version which makes sure that the tasklet has finished and a non-sync
> >>>>version that only makes sure that no new callbacks are started. I think the
> >>>>sync version should be the default with an optional async version which must
> >>>>be used, if it can run from within the callback. So we'd call the async
> >>>>version in the pcm_trigger callback and the sync version in the pcm_close
> >>>>callback.
> >>>Yes this can be done. We can name this disable_callback cmd. The cmd will tell
> >>>dma driver to disable all callback on the channel. This can be invoked from the
> >>>TRIGEGR_STOP and then terminate_all in the free
> >>>
> >>
> >>I think we should make it the default behavior of dmaengine_terminate_all()
> >>to wait for the tasklet to finish.
> >No we cant since terminate can get invoked from callback itself!
> 
> That's why I'm suggesting to introduce a
> dmaenigne_terminate_all_async() which needs to be called in case
> >>Since this is what almost always want,
> >>except in this case where you might end up calling dmaeinge_terminate_all()
> >>from within the callback. Internally this can be implemented as two separate
> >>commands. So leave the DMA_TERMINATE_ALL as it is and add a new
> >>DMA_SYNC_CALLBACKS (or whatever it will be named) command. This command will
> >>internally call tasklet_kill().
> >Implementations will have tasklet for dmaengine and not per channel. So
> >tasklet_kill is not option!
> 
> Most drivers actually have the tasklet per channel.
But NOT all, so thats why we cant make this as requirement. I saw quite a few
driver had common taklet. So this cant be ignored!

> >
> >>Then we have two new functions
> >>dmaengine_terminate_all_async() which will just issue the DMA_TERMINATE_ALL
> >>command, the other function is dmaengine_sync_callbacks() which will issue
> >>the DMA_SYNC_CALLBACKS command. dmaengine_terminate_all() will then first
> >>call dmaengine_terminate_all_async() and then dmaengine_sync_callbacks().
> >>The ALSA code would have to be updated first to call
> >>dmaengine_terminate_all_async() for TRIGGER_STOP and
> >>dmaengine_sync_callbacks() on the pcm_close path.
> >In order to avoid the complication for getting called from the callback itself,
> >I was thinking that we tell dma driver explictly that from now on dont invoke
> >the callbacks (we might be in callback context), then we return. DMA engine will
> >not issue any new callbacks. And also give chance to existing callback to
> >return.
> >Then the client can safely call terminate_all() to abort the channel from non
> >callback context!
> 
> But you'll gain nothing by that. The race condition still exists.
> You need a mechanism to to synchronize the execution of the callback
> against dmaengine_terminate_all(). One simple way for drivers with a
> per channel tasklet is to use tasklet_kill() for this. But other
> possibilities also exists as well.
No we are elimnating it. When you tell driver that you dont want it to invoke
the callback, you give current one which maybe from callback context a chance to
complete. So from that point no invoking callback on that channel. Disabling the channel
interrupts will help as well.

The terminate_all is gauranteed to be invoked from a non callback context. So
driver cna handle it easily

~Vinod