On 2020/08/17 15:29 Benjamin Bara - SKIDATA <Benjamin.Bara@xxxxxxxxxxx> wrote: > We think this is not an i.MX6-specific problem, but a problem of the > DMAengine usage from the PCM. > In case of a XRUN, the DMA channel is never closed but first a > SNDRV_PCM_TRIGGER_STOP next a SNDRV_PCM_TRIGGER_START is triggered. > The SNDRV_PCM_TRIGGER_STOP simply executes a > dmaengine_terminate_async() [1] but does not await the termination by calling > dmaengine_synchronize(), which is required as stated by the docu [2]. > Anyways, we are not able to fix it in the pcm_dmaengine layer either at the > end of SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete > interrupt handler) or at the beginning of SNDRV_PCM_TRIGGER_START (called > from a PCM ioctl), since the dmaengine_synchronize() requires a non-atomic > context. > > Based on my understanding, most of the DMA implementations don't even > implement device_synchronize and if they do, it might not really be necessary > since the terminate_all operation is synchron. > > With the i.MX6, it looks a bit different: > Since [4], the terminate_all operation really schedules a worker which waits > the required ~1ms and then does the context freeing. > Now, the ioctl(SNDRV_PCM_IOCTL_PREPARE) and the following > ioctl(SNDRV_PCM_IOCTL_READI_FRAMES), > which are called from US to handle/recover from a XRUN, are in a race with the > terminate_worker. > If the terminate_worker finishes earlier, everything is fine. > Otherwise, the sdma_prep_dma_cyclic() is called, sets up everything and as > soon as it is scheduled out to wait for data, the terminate_worker is scheduled > and kills it. > In this case, we wait in [5] until the timeout is reached and return with -EIO. > > Based on our understanding, there exist two different fixing approaches: > We thought that the pcm_dmaengine should handle this by either > synchronizing the DMA on a trigger or terminating it synchronously. > However, as we are in an atomic context, we either have to give up the atomic > context of the PCM to finish the termination or we have to design a > synchronous terminate variant which is callable from an atomic context. > > 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. > > Ad Failure Log (at bottom): > I haven't added the ioctl syscalls, but this is basically the output with additional > prints to be able to follow the code execution path. > A XRUN (buffer size is 480 but 960 available) leads to a > SNDRV_PCM_TRIGGER_STOP. > This leads to terminate_async, starting the terminate_worker. > Next, the XRUN recovery triggers SNDRV_PCM_TRIGGER_START, calling > sdma_prep_dma_cyclic and then waits for the DMA in wait_for_avail(). > Next we see the two freeings, first the old, then the newly added one; so the > terminate_worker is back at work. > Now the DMA is terminated, while we are still waiting on data from it. > > What do you think about it? Is any of the provided solutions practicable? > If you need further information or additional logging, feel free to ask. busy_wait is not good I think, would you please have a try with the attached patch which is based on https://lkml.org/lkml/2020/8/11/111? The basic idea is to keep the freed descriptor into another list for freeing in later terminate_worker instead of freeing directly all in terminate_worker by vchan_get_all_descriptors which may break next descriptor coming soon
Attachment:
0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch
Description: 0001-dmaengine-imx-sdma-add-terminated-list-for-freed-des.patch
