[alsa-devel] async between dmaengine_pcm_dma_complete and snd_pcm_release

[Lars-Peter Clausen]

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.

>
>> 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.

- Lars