On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote:
For the first option, which is potentially more performant, we have to leave the atomic PCM context and we are not sure if we are allowed to. For the second option, we would have to divide the dma_device terminate_all into an atomic sync and an async one, which would align with the dmaengine API, giving it the option to ensure termination in an atomic context. Based on my understanding, most of them are synchronous anyways, for the currently async ones we would have to implement busy waits. However, with this approach, we reach the WARN_ON [6] inside of an atomic context, indicating we might not do the right thing.
I don't know how feasible this is to implement in the SDMA dmaengine driver. But I think what is should do is to have some flag to indicate if a terminate is in progress. If a new transfer is issued while terminate is in progress the transfer should go on a list. Once terminate finishes it should check the list and start the transfer if there are any on the list.
The list is already there in form of the vchan helpers the driver uses.
I think the big mistake the driver makes is to configure fields in struct sdma_channel and also the hardware directly in sdma_prep_memcpy(), sdma_prep_slave_sg() and sdma_prep_dma_cyclic(). All information should be stored in the struct sdma_desc allocated in the prep functions and only be used when it's time to fire that specific descriptor.
More specifically sdma_config_write() may not be called from sdma_prep_slave_sg() or sdma_prep_dma_cyclic(), but instead must be called from sdma_start_desc(). sdma_config_ownership() also must be called later in sdma_start_desc(). 'direction' must be a member of struct sdma_desc, not of struct sdma_channel.
Overall this sounds like a fair amount of work to do, but should be feasible and IMO is a step in the right direction.
Sascha