On 2020/08/20 14:52 Sascha Hauer [off-list ref] wrote:

On Wed, Aug 19, 2020 at 01:08:29PM +0200, Lars-Peter Clausen wrote:

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

Seems Lars major concern is on the race condition between next descriptor
and sdma_channel_terminate_work which free the last terminated descriptor,
not the ability of vchan to support multi descriptors. But anyway, I think we
should take care vchan_get_all_descriptors to free descriptors during terminate
phase in case it's done in worker like sdma_channel_terminate_work, since that
may free the next descriptor wrongly. That's what my patch attached in
0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch
https://www.spinics.net/lists/arm-kernel/msg829972.html

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.

Sorry Sascha, seems that's another topic and your intention is to make sure only
software involved in sdma_prep_* and all HW moved into one function inside
sdma_start_desc. I agree that will make code more clean but my concern is 
sdma_start_desc is protect by spin_lock which should be short as possible while
some HW touch as context_load may cost some time. Anyway, that's another topic,
maybe we can refine it in the future.

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

--

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel