DMA Transfer Synchronization Issue in Out-of-Tree Sound Card Driver

DMA Transfer Synchronization Issue in Out-of-Tree Sound Card Driver

[Louis Chauvet]

Hello everyone,

I am currently developing an out-of-tree driver for a sound card that 
utilizes XDMA for sample transfers. I am currently using a kernel 6.5 with 
the latest xdma driver cherry-picked, but I don't think any changes since 
6.5 is addressing my issue.

My initial issue pertains to the completion of DMA transfers between a 
start and a stop command in ALSA. If the interval is too brief, the 
transfer does not conclude properly, leading to distorted samples. A 
straightforward solution to this problem was to adequately wait for the 
transfer to finish upon the stop, ie. sleeping until we know the hardware 
is done with the transfert (the XDMA controller does not support stopping 
in the middle of a transfer).

To address this DMA issue, I have created a patch [1] that guarantees the 
completion of the DMA transfer upon the return of xdma_synchronize. This 
means xdma_synchronize now sleeps, but looking at other drivers around it 
appears expected to be able to do so.

Regarding the audio implementation, the following patch enforces the 
synchronization:

	int playback_trigger(struct snd_pcm_substream *substream, int command)
	{
		struct my_dev *my_dev =	snd_pcm_substream_chip(substream);

		switch (command) {
		case SNDRV_PCM_TRIGGER_START:
			/* Synchronize on start, because the trigger stop is called from an IRQ	context	*/
			if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
				dmaengine_synchronize(my_dev->playback_dma_chan);
			pipe_start(&my_dev->playback, substream);
			break;
		case SNDRV_PCM_TRIGGER_STOP:
			pipe_stop(&my_dev->playback, substream);
			break;
		default:
			return -EINVAL;
		}
		return 0;
	}

In order for a sleepable function like dmaengine_synchronize() to work in 
the trigger callbacks, the audio card nonatomic flag had to be set. It 
basically leads the sound core towards the use of a mutex instead of a 
spinlock.

	static int probe([...])	{
		struct snd_pcm *pcm;
		[...]
		/* This flag is needed to be able to sleep in start/stop callbacks */
		pcm->nonatomic = true;
                [...]
		snd_pcm_set_managed_buffer_all(pcm, [...]);
	}

This approach generally works well, but leads to "scheduling while
atomic" errors. Indeed, the IRQ handler from the XDMA driver invokes a
function within the sound subsystem, which subsequently acquires a
mutex...

At the moment, the only solution I've found is to replace the 
wait_for_completion() in the XDMA driver [2] with a busy wait loop. 
However, this approach seems incorrect, as all other synchronization 
functions in other DMA drivers are sleeping, which should not cause an 
issue.

The problem might be related to the sound driver. Should I avoid manually 
using dmaengine_synchronize? How to achieve the same effect in this case? 
Perhaps there is a more traditional way to properly clean the stream in 
the sound subsystem which I overlooked?

Could someone please provide guidance on how to resolve this issue?

Thanks,
Louis Chauvet

[1]: https://lore.kernel.org/all/20240327-digigram-xdma-fixes-v1-2-45f4a52c0283@bootlin.com/
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/dma/xilinx/xdma.c#L550

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: DMA Transfer Synchronization Issue in Out-of-Tree Sound Card Driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Tue, May 21, 2024 at 10:31:12AM +0200, Louis Chauvet wrote:

> To address this DMA issue, I have created a patch [1] that guarantees the 
> completion of the DMA transfer upon the return of xdma_synchronize. This 
> means xdma_synchronize now sleeps, but looking at other drivers around it 
> appears expected to be able to do so.

You need to set the nonatomic flag for the PCM to allow this, the
default is that triggers run in atomic context.

> 
> 		switch (command) {
> 		case SNDRV_PCM_TRIGGER_START:
> 			/* Synchronize on start, because the trigger stop is called from an IRQ	context	*/
> 			if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> 				dmaengine_synchronize(my_dev->playback_dma_chan);

If any dmaengine work is needed put it in the generic dmaengine code and
allow it to be configured there (ideally through discovery through the
API).

> The problem might be related to the sound driver. Should I avoid manually 
> using dmaengine_synchronize? How to achieve the same effect in this case? 
> Perhaps there is a more traditional way to properly clean the stream in 
> the sound subsystem which I overlooked?

If there's no way of resetting things without blocking then I'm not sure
you can do much better though I might be forgetting something, it does
seem like disappointing hardware design and application behaviour.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: DMA Transfer Synchronization Issue in Out-of-Tree Sound Card Driver

[Takashi Iwai]

On Tue, 21 May 2024 20:32:59 +0200,
Mark Brown wrote:
> 
> On Tue, May 21, 2024 at 10:31:12AM +0200, Louis Chauvet wrote:
> 
> > To address this DMA issue, I have created a patch [1] that guarantees the 
> > completion of the DMA transfer upon the return of xdma_synchronize. This 
> > means xdma_synchronize now sleeps, but looking at other drivers around it 
> > appears expected to be able to do so.
> 
> You need to set the nonatomic flag for the PCM to allow this, the
> default is that triggers run in atomic context.

Right, that's a most straightforward solution.  It implies that the
period updates must be in non-atomic, i.e. use a threaded irq handler
in most cases.

If the synchronization is needed for assuring the hardware stop, there
is an alternative with PCM sync_stop callback, too.  The callback is
called at each time after a stream gets stopped before the next action
(that is, either prepare, hw_params or close).  It's only for
stopping, and there is no similar way for sync of a stream start,
though.


thanks,

Takashi

Re: DMA Transfer Synchronization Issue in Out-of-Tree Sound Card Driver

[Louis Chauvet]

Le 22/05/24 - 07:52, Takashi Iwai a écrit :
> On Tue, 21 May 2024 20:32:59 +0200,
> Mark Brown wrote:
> > 
> > On Tue, May 21, 2024 at 10:31:12AM +0200, Louis Chauvet wrote:
> > 
> > > To address this DMA issue, I have created a patch [1] that guarantees the 
> > > completion of the DMA transfer upon the return of xdma_synchronize. This 
> > > means xdma_synchronize now sleeps, but looking at other drivers around it 
> > > appears expected to be able to do so.
> > 
> > You need to set the nonatomic flag for the PCM to allow this, the
> > default is that triggers run in atomic context.
> 
> Right, that's a most straightforward solution.  It implies that the
> period updates must be in non-atomic, i.e. use a threaded irq handler
> in most cases.
> 
> If the synchronization is needed for assuring the hardware stop, there
> is an alternative with PCM sync_stop callback, too.  The callback is
> called at each time after a stream gets stopped before the next action
> (that is, either prepare, hw_params or close).  It's only for
> stopping, and there is no similar way for sync of a stream start,
> though.
> 
> 
> thanks,
> 
> Takashi
> 

Hi!

Thank you for your prompt responses!

I have currently implemented the solution with sync_stop, as it is 
precisely what I need to do, and it works perfectly.

As I may need to backport this driver up to 4.19, sync_stop was not yet 
available, so I will look into the threaded IRQ solution, which sounds 
promising.

Thank you both very much!

Best regards,
Louis Chauvet

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com