Hi Laurent

...

The rsnd_soc_dai_trigger() function takes a spinlock, making the context atomic, which regmap doesn't like as it locks a mutex.

It might be possible to fix this by setting the fast_io field in both the regmap_config and regmap_bus structures in sound/soc/sh/rcar/gen.c. regmap will then use a spinlock instead of a mutex. However, even if I believe that change makes sense and should be done, another atomic context issue will come from the rcar-dmac driver which allocates memory in the prep_dma_cyclic function, called by rsnd_dma_start() from rsnd_soc_dai_trigger() with the spinlock help.

What context is the rsnd_soc_dai_trigger() function called in by the alsa core ? If it's guaranteed to be a sleepable context, would it make sense to replace the rsnd_priv spinlock with a mutex ?

Answering myself here, that's not an option, as the trigger function is called in atomic context (replacing the spinlock with a mutex produced a clear BUG) due to snd_pcm_lib_write1() taking a spinlock.

Now we have a core issue. On one side there's rsnd_soc_dai_trigger() being called in atomic context, and on the other side the function ends up calling dmaengine_prep_dma_cyclic() which needs to allocate memory. To make this more func the DMA engine API is undocumented and completely silent on whether the prep functions are allowed to sleep. The question is, who's wrong ?

Now, if you're tempted to say that I'm wrong by allocating memory with GFP_KERNEL in the DMA engine driver, please read on first :-) I've tried replacing GFP_KERNEL with GFP_ATOMIC allocations, and then ran into a problem more complex to solve.

The rcar-dmac DMA engine driver uses runtime PM. When not used, the device is suspended. The driver calls pm_runtime_get_sync() to resume the device, and needs to do so when a descriptor is submitted. This operation, currently performed in the tx_submit handler, could be moved to the prep_dma_cyclic or issue_pending handler, but the three operations are called in a row from rsnd_dma_start(), itself ultimately called from snd_pcm_lib_write1() with the spinlock held. This means I have no place in my DMA engine driver where I can resume the device.

One could argue that the rcar-dmac driver could use a work queue to handle power management. That's correct, but the additional complexity, which would be required in *all* DMA engine drivers, seem too high to me. If we need to go that way, this is really a task that the DMA engine core should handle.

Let's start by answering the background question and updating the DMA engine API documentation once and for good : in which context are drivers allowed to call the prep, tx_submit and issue_pending functions ?

rsnd driver (and sound/soc/sh/fsi driver too) is using prep_dma_cyclic() now, but, it had been used prep_slave_single() before. Then, it used work queue in dai_trigger function. How about to use same method in prep_dma_cyclic() ? Do you think your issue will be solved if sound driver calls prep_dma_cyclic() from work queue ?

Best regards --- Kuninori Morimoto

On Wed, Jul 23, 2014 at 06:35:17PM -0700, Kuninori Morimoto wrote:

Hi Laurent again

...

...

...

The rsnd_soc_dai_trigger() function takes a spinlock, making the context atomic, which regmap doesn't like as it locks a mutex.

It might be possible to fix this by setting the fast_io field in both the regmap_config and regmap_bus structures in sound/soc/sh/rcar/gen.c. regmap will then use a spinlock instead of a mutex. However, even if I believe that change makes sense and should be done, another atomic context issue will come from the rcar-dmac driver which allocates memory in the prep_dma_cyclic function, called by rsnd_dma_start() from rsnd_soc_dai_trigger() with the spinlock help.

What context is the rsnd_soc_dai_trigger() function called in by the alsa core ? If it's guaranteed to be a sleepable context, would it make sense to replace the rsnd_priv spinlock with a mutex ?

Answering myself here, that's not an option, as the trigger function is called in atomic context (replacing the spinlock with a mutex produced a clear BUG) due to snd_pcm_lib_write1() taking a spinlock.

Now we have a core issue. On one side there's rsnd_soc_dai_trigger() being called in atomic context, and on the other side the function ends up calling dmaengine_prep_dma_cyclic() which needs to allocate memory. To make this more func the DMA engine API is undocumented and completely silent on whether the prep functions are allowed to sleep. The question is, who's wrong ?

Now, if you're tempted to say that I'm wrong by allocating memory with GFP_KERNEL in the DMA engine driver, please read on first :-) I've tried replacing GFP_KERNEL with GFP_ATOMIC allocations, and then ran into a problem more complex to solve.

The rcar-dmac DMA engine driver uses runtime PM. When not used, the device is suspended. The driver calls pm_runtime_get_sync() to resume the device, and needs to do so when a descriptor is submitted. This operation, currently performed in the tx_submit handler, could be moved to the prep_dma_cyclic or issue_pending handler, but the three operations are called in a row from rsnd_dma_start(), itself ultimately called from snd_pcm_lib_write1() with the spinlock held. This means I have no place in my DMA engine driver where I can resume the device.

One could argue that the rcar-dmac driver could use a work queue to handle power management. That's correct, but the additional complexity, which would be required in *all* DMA engine drivers, seem too high to me. If we need to go that way, this is really a task that the DMA engine core should handle.

Let's start by answering the background question and updating the DMA engine API documentation once and for good : in which context are drivers allowed to call the prep, tx_submit and issue_pending functions ?

rsnd driver (and sound/soc/sh/fsi driver too) is using prep_dma_cyclic() now, but, it had been used prep_slave_single() before. Then, it used work queue in dai_trigger function. How about to use same method in prep_dma_cyclic() ? Do you think your issue will be solved if sound driver calls prep_dma_cyclic() from work queue ?

Sorry, this doesn't solve issue. dmaengine_prep_dma_cyclic() is used in ${LINUX}/sound/core/pcm_dmaengine.c, and the situation is same as ours.

Hmm.. In my quick check, other DMAEngine drivers are using GFP_ATOMIC in cyclic/prep_slave_sg functions...

And thats partially right. You need to use GFP_NOWAIT.

Hi Vinod,

On Thursday 24 July 2014 10:22:48 Vinod Koul wrote:

On Wed, Jul 23, 2014 at 01:07:43PM +0200, Laurent Pinchart wrote:

...

...

The rsnd_soc_dai_trigger() function takes a spinlock, making the context atomic, which regmap doesn't like as it locks a mutex.

It might be possible to fix this by setting the fast_io field in both the regmap_config and regmap_bus structures in sound/soc/sh/rcar/gen.c. regmap will then use a spinlock instead of a mutex. However, even if I believe that change makes sense and should be done, another atomic context issue will come from the rcar-dmac driver which allocates memory in the prep_dma_cyclic function, called by rsnd_dma_start() from rsnd_soc_dai_trigger() with the spinlock help.

What context is the rsnd_soc_dai_trigger() function called in by the alsa core ? If it's guaranteed to be a sleepable context, would it make sense to replace the rsnd_priv spinlock with a mutex ?

Answering myself here, that's not an option, as the trigger function is called in atomic context (replacing the spinlock with a mutex produced a clear BUG) due to snd_pcm_lib_write1() taking a spinlock.

Now we have a core issue. On one side there's rsnd_soc_dai_trigger() being called in atomic context, and on the other side the function ends up calling dmaengine_prep_dma_cyclic() which needs to allocate memory. To make this more func the DMA engine API is undocumented and completely silent on whether the prep functions are allowed to sleep. The question is, who's wrong ?

Now, if you're tempted to say that I'm wrong by allocating memory with GFP_KERNEL in the DMA engine driver, please read on first :-) I've tried replacing GFP_KERNEL with GFP_ATOMIC allocations, and then ran into a problem more complex to solve.

The rcar-dmac DMA engine driver uses runtime PM. When not used, the device is suspended. The driver calls pm_runtime_get_sync() to resume the device, and needs to do so when a descriptor is submitted. This operation, currently performed in the tx_submit handler, could be moved to the prep_dma_cyclic or issue_pending handler, but the three operations are called in a row from rsnd_dma_start(), itself ultimately called from snd_pcm_lib_write1() with the spinlock held. This means I have no place in my DMA engine driver where I can resume the device.

One could argue that the rcar-dmac driver could use a work queue to handle power management. That's correct, but the additional complexity, which would be required in *all* DMA engine drivers, seem too high to me. If we need to go that way, this is really a task that the DMA engine core should handle.

Let's start by answering the background question and updating the DMA engine API documentation once and for good : in which context are drivers allowed to call the prep, tx_submit and issue_pending functions ?

I think this was bought up sometime back and we have cleared that all _prep functions can be invoked in atomic context.

This is the reason why we have been pushing folks to use GFP_NOWAIT is memory allocations during prepare.

From the replies I've received it's pretty clear that the prep functions need

to be callable from atomic context. I'll respond to this in more depth in a reply to Russell's e-mail.

Thanks for pointing out documentation doesn't say so, will send a patch for that.

I wish that was all that is missing from the documentation ;-) Luckily Maxime Ripard has sent a patch that documents DMA engine from a DMA engine driver's point of view. While not perfect (I'm going to review it), it's a nice starting point to (hopefully) get to a properly documented framework.

On issue_pending and tx_submit, yes these should be allowed to be called from atomic context too.

I'll take this opportunity to question why we have a separation between tx_submit and issue_pending. What's the rationale for that, especially given that dma_issue_pending_all() might kick in at any point and issue pending transfers for all devices. A driver could thus see its submitted but not issued transactions being issued before it explicitly calls dma_async_issue_pending().

The DMA_PRIVATE capability flag seems to play a role here, but it's far from being clear how that mechanism is supposed to work. This should be documented as well, and any light you could shed on this dark corner of the API would help.

Similarly, the DMA engine API is split in functions with different prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various unprefixed niceties such as async_tx_ack or txd_lock. If there's a rationale for that (beyond just historical reasons) it should also be documented, otherwise a cleanup would help all the confused DMA engine users (myself included). I might be able to find a bit of time to work on that, but I'll first need to correctly understand where we come from and where we are. Again, information would be welcome and fully appreciated.

Lastly, just to clarify the callback invoked after descriptor is complete can also be used to submit new descriptors, so folks are dropping locking before invoking the callback

On Fri, Aug 01, 2014 at 03:30:20PM +0100, Russell King - ARM Linux wrote:

On Fri, Aug 01, 2014 at 10:51:26AM +0200, Laurent Pinchart wrote:

...

I'll take this opportunity to question why we have a separation between tx_submit and issue_pending. What's the rationale for that, especially given that dma_issue_pending_all() might kick in at any point and issue pending transfers for all devices. A driver could thus see its submitted but not issued transactions being issued before it explicitly calls dma_async_issue_pending().

A prepared but not submitted transaction is not a pending transaction.

The split is necessary so that a callback can be attached to the transaction. This partially comes from the async-tx API, and also gets a lot of use with the slave API.

The prepare function allocates the descriptor and does the initial setup, but does not mark the descriptor as a pending transaction. It returns the descriptor, and the caller is then free to add a callback function and data pointer to the descriptor before finally submitting it. This sequence must occur in a timely manner as some DMA engine implementations hold a lock between the prepare and submit callbacks (Dan explicitly permits this as part of the API.)

...

The DMA_PRIVATE capability flag seems to play a role here, but it's far from being clear how that mechanism is supposed to work. This should be documented as well, and any light you could shed on this dark corner of the API would help.

Why do you think that DMA_PRIVATE has a bearing in the callbacks? It doesn't. DMA_PRIVATE is all about channel allocation as I explained yesterday, and whether the channel is available for async_tx usage.

A channel marked DMA_PRIVATE is not available for async_tx usage at any moment. A channel without DMA_PRIVATE is available for async_tx usage until it is allocated for the slave API - at which point the generic DMA engine code will mark the channel with DMA_PRIVATE, thereby taking it away from async_tx API usage. When the slave API frees the channel, DMA_PRIVATE will be cleared, making the channel available for async_tx usage.

So, basically, DMA_PRIVATE set -> async_tx usage not allowed. DMA_PRIVATE clear -> async_tx usage permitted. It really is that simple.

...

Similarly, the DMA engine API is split in functions with different prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various unprefixed niceties such as async_tx_ack or txd_lock. If there's a rationale for that (beyond just historical reasons) it should also be documented, otherwise a cleanup would help all the confused DMA engine users (myself included).

dmaengine_* are generally the interface functions to the DMA engine code, which have been recently introduced to avoid the multiple levels of pointer indirection having to be typed in every driver.

dma_async_* are the DMA engine interface functions for the async_tx API.

dma_* predate the dmaengine_* naming, and are probably too generic, so should probably end up being renamed to dmaengine_*.

txd_* are all about the DMA engine descriptors.

async_tx_* are the higher level async_tx API functions.

Ah looks like I repeated the good answers from you. Should have read all replied first

Hi Russell,

On Friday 01 August 2014 15:30:20 Russell King - ARM Linux wrote:

On Fri, Aug 01, 2014 at 10:51:26AM +0200, Laurent Pinchart wrote:

...

I'll take this opportunity to question why we have a separation between tx_submit and issue_pending. What's the rationale for that, especially given that dma_issue_pending_all() might kick in at any point and issue pending transfers for all devices. A driver could thus see its submitted but not issued transactions being issued before it explicitly calls dma_async_issue_pending().

A prepared but not submitted transaction is not a pending transaction.

The split is necessary so that a callback can be attached to the transaction. This partially comes from the async-tx API, and also gets a lot of use with the slave API.

The prepare function allocates the descriptor and does the initial setup, but does not mark the descriptor as a pending transaction. It returns the descriptor, and the caller is then free to add a callback function and data pointer to the descriptor before finally submitting it.

No disagreement on that. However, as Geert pointed out, my question was related to the split between dmaengine_submit() and dma_async_issue_pending(), not between the prep_* functions and dmaengine_submit().

This sequence must occur in a timely manner as some DMA engine implementations hold a lock between the prepare and submit callbacks (Dan explicitly permits this as part of the API.)

That really triggers a red alarm in the part of my brain that deals with API design, but I suppose it would be too difficult to change that.

...

The DMA_PRIVATE capability flag seems to play a role here, but it's far from being clear how that mechanism is supposed to work. This should be documented as well, and any light you could shed on this dark corner of the API would help.

Why do you think that DMA_PRIVATE has a bearing in the callbacks? It doesn't.

Not on callbacks, but on how pending descriptors are pushed to the hardware. The flag is explicitly checked in dma_issue_pending_all().

DMA_PRIVATE is all about channel allocation as I explained yesterday, and whether the channel is available for async_tx usage.

A channel marked DMA_PRIVATE is not available for async_tx usage at any moment. A channel without DMA_PRIVATE is available for async_tx usage until it is allocated for the slave API - at which point the generic DMA engine code will mark the channel with DMA_PRIVATE, thereby taking it away from async_tx API usage. When the slave API frees the channel, DMA_PRIVATE will be cleared, making the channel available for async_tx usage.

So, basically, DMA_PRIVATE set -> async_tx usage not allowed. DMA_PRIVATE clear -> async_tx usage permitted. It really is that simple.

DMA_PRIVATE is a dma_device flag, not a dma_chan flag. As soon as one channel is allocated by __dma_request_channel() the whole device is marked with DMA_PRIVATE, making all channels private. What am I missing ?

...

Similarly, the DMA engine API is split in functions with different prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various unprefixed niceties such as async_tx_ack or txd_lock. If there's a rationale for that (beyond just historical reasons) it should also be documented, otherwise a cleanup would help all the confused DMA engine users (myself included).

dmaengine_* are generally the interface functions to the DMA engine code, which have been recently introduced to avoid the multiple levels of pointer indirection having to be typed in every driver.

dma_async_* are the DMA engine interface functions for the async_tx API.

dma_* predate the dmaengine_* naming, and are probably too generic, so should probably end up being renamed to dmaengine_*.

Thank you for the confirmation. I'll see if I can cook up a patch. It will likely be pretty large and broad though, but I guess there's no way around it.

txd_* are all about the DMA engine descriptors.

async_tx_* are the higher level async_tx API functions.

Thank you for the information. How about the dma_async_* functions, should they be renamed to dmaengine_* as well ? Or are some of them part of the async_tx_* API ?

Hi Russell,

On Monday 04 August 2014 18:54:58 Russell King - ARM Linux wrote:

On Mon, Aug 04, 2014 at 07:00:36PM +0200, Laurent Pinchart wrote:

...

On Friday 01 August 2014 15:30:20 Russell King - ARM Linux wrote:

...

This sequence must occur in a timely manner as some DMA engine

implementations hold a lock between the prepare and submit callbacks (Dan explicitly permits this as part of the API.)

That really triggers a red alarm in the part of my brain that deals with API design, but I suppose it would be too difficult to change that.

Mine to, but there's not a lot which can be done about it without changing a lot of users.

Well, the good side is that it "only" requires enough motivation and free time then :-)

...

...

...

The DMA_PRIVATE capability flag seems to play a role here, but it's far from being clear how that mechanism is supposed to work. This should be documented as well, and any light you could shed on this dark corner of the API would help.

Why do you think that DMA_PRIVATE has a bearing in the callbacks? It doesn't.

Not on callbacks, but on how pending descriptors are pushed to the hardware. The flag is explicitly checked in dma_issue_pending_all().

Right. So, let me put a question to you - what do you think is the effect of the check in dma_issue_pending_all()?

I'll give you a hint - disregard the comment at the top of the function, because that's out of date.

I suppose the idea is that dma_issue_pending_all() is only used for memcpy offload, and can thus ignore channels used for slave transfers.

The code seems to be buggy though. A DMA engine that can serve both memcpy and slave transfers could have one channel allocated for memcpy first, then a second channel allocated for slave transfers. This would cause the DMA_PRIVATE flag to be set, which will prevent dma_issue_pending_all() from calling the device_issue_pending operation of the memcpy channel.

...

...

DMA_PRIVATE is all about channel allocation as I explained yesterday, and whether the channel is available for async_tx usage.

A channel marked DMA_PRIVATE is not available for async_tx usage at any moment. A channel without DMA_PRIVATE is available for async_tx usage until it is allocated for the slave API - at which point the generic DMA engine code will mark the channel with DMA_PRIVATE, thereby taking it away from async_tx API usage. When the slave API frees the channel, DMA_PRIVATE will be cleared, making the channel available for async_tx usage.

So, basically, DMA_PRIVATE set -> async_tx usage not allowed. DMA_PRIVATE clear -> async_tx usage permitted. It really is that simple.

DMA_PRIVATE is a dma_device flag, not a dma_chan flag. As soon as one channel is allocated by __dma_request_channel() the whole device is marked with DMA_PRIVATE, making all channels private. What am I missing ?

I can't answer that - I don't know why the previous authors decided to make it a DMA-device wide property - presumably there are DMA controllers where this matters.

If I understand the history correctly, the reason to make DMA_PRIVATE a device flag is to avoid starving slaves by allocating all channels of a device for memcpy. If the DMA_PRIVATE flag is set by the DMA engine driver that works as expected. If the DMA engine can be used for both, though, there's no guarantee, and the behaviour won't be very predictable.

By the way, shouldn't DMA_PRIVATE be renamed to something more explicit, such as DMA_NO_MEMCPY or DMA_SLAVE_ONLY ?

However, one thing to realise is that a dma_device is a virtual concept - it is a set of channels which share a common set of properties. It is not a physical device. It is entirely reasonable for a set of channels on a physical device to be shared between two different dma_device instances and handed out by the driver code as needed.

When the channels are independent, sure, but they sometimes share hardware resources. For instance the Renesas R-Car Gen2 SoCs have 2 generic-purpose DMA engines usable for both memcpy and slave transfers, each of them having 15 channels. Each DMA engine arbitrates memory accesses from the different channels, using either fixed priorities, or a round-robin arbitration. In that case it wouldn't make much sense to split the 15 channels across several dma_device instances.

I actually have the opposite problem, in my case channels of physically separate DMA engines can be used interchangeably to serve the system's slaves. Using the DMA engine DT bindings, DT nodes of the slaves currently reference a specific DMA engine, even if they can be served by both. This leads to limited dynamic channel allocation capabilities (especially when taking into account lazy channel allocation as mentioned in another mail in this thread).

...

...

...

Similarly, the DMA engine API is split in functions with different prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various unprefixed niceties such as async_tx_ack or txd_lock. If there's a rationale for that (beyond just historical reasons) it should also be documented, otherwise a cleanup would help all the confused DMA engine users (myself included).

dmaengine_* are generally the interface functions to the DMA engine code, which have been recently introduced to avoid the multiple levels of pointer indirection having to be typed in every driver.

dma_async_* are the DMA engine interface functions for the async_tx API.

dma_* predate the dmaengine_* naming, and are probably too generic, so should probably end up being renamed to dmaengine_*.

Thank you for the confirmation. I'll see if I can cook up a patch. It will likely be pretty large and broad though, but I guess there's no way around it.

...

txd_* are all about the DMA engine descriptors.

async_tx_* are the higher level async_tx API functions.

Thank you for the information. How about the dma_async_* functions, should they be renamed to dmaengine_* as well ? Or are some of them part of the async_tx_* API ?

Well, these:

dma_async_device_register dma_async_device_unregister dma_async_tx_descriptor_init

are more DMA engine core <-> DMA engine driver interface functions than user functions. The remainder of the dma_async_* functions are internal to the async_tx API.

I was also thinking about dma_async_issue_pending(). Isn't that function part of the DMA engine API rather than the async API ?

On Wednesday 06 August 2014, Geert Uytterhoeven wrote:

...

I actually have the opposite problem, in my case channels of physically separate DMA engines can be used interchangeably to serve the system's slaves. Using the DMA engine DT bindings, DT nodes of the slaves currently reference a specific DMA engine, even if they can be served by both. This leads to limited dynamic channel allocation capabilities (especially when taking into account lazy channel allocation as mentioned in another mail in this thread).

What about adding a property to the first one, referencing the second (or the other way around, don't know what's the easiest to implement)?

    dmac0: dma-controller@e6700000 {
            ...
            renesas,alternative = <&dmac1>;
            ...
    };

    dmac1: dma-controller@e6720000 {
            ...
    };

That would avoid having to bind a slave device explicitly to a single dmac, or having to bind all slave devices to all dmacs.

We have a perfectly fine way to express this with the existing binding already: you just list channels for both (or more) controllers for each slave, and let the dma subsystem pick one. This was a compromise we reached when we initially introduced the dma slave binding, the downside being that we have to name every reference from a slave to a controller, even though almost all of them are "rx", "tx" or "data".

I believe what happened though is that the initial implementation in the kernel was to just pick the first channel for a given name and try that but give up if it fails. This works for the majority of users and I had expected someone to implement a smarter strategy as needed.

The easiest way would be to just randomize the order of the channels during lookup and then try them all, but there is a potential problem with this failing sometimes in nondeterministic ways. Another alternative would be for the dma controller to report back some form of "utilization" number to the dma subsystem and have the common code pick the least utilized engine that is connected to that slave.

Arnd

Hi Vinod,

On Friday 01 August 2014 22:37:44 Vinod Koul wrote:

On Fri, Aug 01, 2014 at 10:51:26AM +0200, Laurent Pinchart wrote:

...

On Thursday 24 July 2014 10:22:48 Vinod Koul wrote:

...

On Wed, Jul 23, 2014 at 01:07:43PM +0200, Laurent Pinchart wrote:

...

...

The rsnd_soc_dai_trigger() function takes a spinlock, making the context atomic, which regmap doesn't like as it locks a mutex.

It might be possible to fix this by setting the fast_io field in both the regmap_config and regmap_bus structures in sound/soc/sh/rcar/gen.c. regmap will then use a spinlock instead of a mutex. However, even if I believe that change makes sense and should be done, another atomic context issue will come from the rcar-dmac driver which allocates memory in the prep_dma_cyclic function, called by rsnd_dma_start() from rsnd_soc_dai_trigger() with the spinlock help.

What context is the rsnd_soc_dai_trigger() function called in by the alsa core ? If it's guaranteed to be a sleepable context, would it make sense to replace the rsnd_priv spinlock with a mutex ?

Answering myself here, that's not an option, as the trigger function is called in atomic context (replacing the spinlock with a mutex produced a clear BUG) due to snd_pcm_lib_write1() taking a spinlock.

Now we have a core issue. On one side there's rsnd_soc_dai_trigger() being called in atomic context, and on the other side the function ends up calling dmaengine_prep_dma_cyclic() which needs to allocate memory. To make this more func the DMA engine API is undocumented and completely silent on whether the prep functions are allowed to sleep. The question is, who's wrong ?

Now, if you're tempted to say that I'm wrong by allocating memory with GFP_KERNEL in the DMA engine driver, please read on first :-) I've tried replacing GFP_KERNEL with GFP_ATOMIC allocations, and then ran into a problem more complex to solve.

The rcar-dmac DMA engine driver uses runtime PM. When not used, the device is suspended. The driver calls pm_runtime_get_sync() to resume the device, and needs to do so when a descriptor is submitted. This operation, currently performed in the tx_submit handler, could be moved to the prep_dma_cyclic or issue_pending handler, but the three operations are called in a row from rsnd_dma_start(), itself ultimately called from snd_pcm_lib_write1() with the spinlock held. This means I have no place in my DMA engine driver where I can resume the device.

One could argue that the rcar-dmac driver could use a work queue to handle power management. That's correct, but the additional complexity, which would be required in *all* DMA engine drivers, seem too high to me. If we need to go that way, this is really a task that the DMA engine core should handle.

Let's start by answering the background question and updating the DMA engine API documentation once and for good : in which context are drivers allowed to call the prep, tx_submit and issue_pending functions ?

I think this was bought up sometime back and we have cleared that all _prep functions can be invoked in atomic context.

This is the reason why we have been pushing folks to use GFP_NOWAIT is memory allocations during prepare.

From the replies I've received it's pretty clear that the prep functions need to be callable from atomic context. I'll respond to this in more depth in a reply to Russell's e-mail.

...

Thanks for pointing out documentation doesn't say so, will send a patch for that.

I wish that was all that is missing from the documentation ;-) Luckily Maxime Ripard has sent a patch that documents DMA engine from a DMA engine driver's point of view. While not perfect (I'm going to review it), it's a nice starting point to (hopefully) get to a properly documented framework.

Sure, please do point out any other instance.

Don't worry, I will :-)

Russell did improve it a lot. But then Documentation gets not so quick updates. Yes new users like you can list issues far more easily than others.

Also now commit log provides a very valuable source of why a particular thing was done.

Come on. That's the theory (and we should of course aim for it). We all know the difference between theory and practice : in theory they're the same.

More seriously speaking, I've dived into the git history more times than I should have to retain some mental sanity, and it leaves way too many questions unanswered.

...

...

from atomic context too.

I'll take this opportunity to question why we have a separation between tx_submit and issue_pending. What's the rationale for that, especially given that dma_issue_pending_all() might kick in at any point and issue pending transfers for all devices. A driver could thus see its submitted but not issued transactions being issued before it explicitly calls dma_async_issue_pending().

The API states that you need to get a channel, then prepare a descriptor and submit it back. Prepare can be in any order. The submit order is the one which is run on dmaengine. The submit marks the descriptor as pending. Typically you should have a pending_list which the descriptor should be pushed to.

And lastly invoke dma_async_issue_pending() to start the pending ones.

You have the flexibility to prepare descriptors and issue in the order you like. You can also attach the callback required for descriptors here.

The question was why is there a dma_async_issue_pending() operation at all ? Why can't dmaengine_submit() triggers the transfer start ? The only explanation is a small comment in dmaengine.h that states

* This allows drivers to push copies to HW in batches, * reducing MMIO writes where possible.

I don't think that's applicable for DMA slave transfers. Is it still applicable for anything else ?

Quoting git log, the reason is

commit c13c8260da3155f2cefb63b0d1b7dcdcb405c644 Author: Chris Leech christopher.leech@intel.com Date: Tue May 23 17:18:44 2006 -0700

[I/OAT]: DMA memcpy subsystem

Provides an API for offloading memory copies to DMA devices

Signed-off-by: Chris Leech christopher.leech@intel.com Signed-off-by: David S. Miller davem@davemloft.net

;-)

...

The DMA_PRIVATE capability flag seems to play a role here, but it's far from being clear how that mechanism is supposed to work. This should be documented as well, and any light you could shed on this dark corner of the API would help.

Ah it is not so dark.

if you look closely at dmaengine channel allocation it is only for marking if channel is privately to be used or for async_tx. Thus slave devices must set DMA_PRIVATE

In order to avoid scattering one topic across multiple mails, I'll pursue this one in a reply to Russell.

...

Similarly, the DMA engine API is split in functions with different prefixes (mostly dmaengine_*, dma_async_*, dma_*, and various unprefixed niceties such as async_tx_ack or txd_lock. If there's a rationale for that (beyond just historical reasons) it should also be documented, otherwise a cleanup would help all the confused DMA engine users (myself included). I might be able to find a bit of time to work on that, but I'll first need to correctly understand where we come from and where we are. Again, information would be welcome and fully appreciated.

History. DMA engine was developed for async_tx. (hence async_tx)

I think most of dmaengine APIs are dmaengine_. the async_ ones are specifically for async_tx usage. txd ones are descriptor related.

Ditto here.

On Mon, Aug 04, 2014 at 08:03:45PM +0200, Lars-Peter Clausen wrote:

If the hardware has scatter gather support it allows the driver to chain the descriptors before submitting them, which reduces the latency between the transfers as well as the IO over overhead.

While partially true, that's not the full story...

BTW, you're talking about stuff in DMA engine not being clear, but you're using confusing terminology. Descriptors vs transactions. The prepare functions return a transaction. Descriptors are the hardware data structures which describe the transaction. I'll take what you're talking about above as "chain the previous transaction descriptors to the next transaction descriptors".

The flaw with the current implementation is that there is only one global chain per channel instead of e.g. having the possibility to build up a chain in a driver and then submit and start the chain. Some drivers have virtual channels where each channel basically acts as the chain and once issue pending is called it is the chain is mapped to a real channel which then executes it.

Most DMA engines are unable to program anything except the parameters for the next stage of the transfer. In order to switch between "channels", many DMA engine implementations need the help of the CPU to reprogram the physical channel configuration. Chaining two different channels which may ultimately end up on the same physical channel would be a bug in that case.

Where the real flaw exists is the way that a lot of people write their DMA engine drivers - in particular how they deal with the end of a transfer.

Many driver implementations receive an interrupt from the DMA controller, and either queue a tasklet, or they check the existing transfer, mark it as completed in some way, and queue a tasklet.

When the tasklet runs, they then look to see if there's another transfer which they can start, and they then start it.

That is horribly inefficient - it is much better to do all the DMA manipulation in IRQ context. So, when the channel completes the existing transfer, you move the transaction to the queue of completed transfers and queue the tasklet, check whether there's a transaction for the same channel pending, and if so, start it immediately.

This means that your inter-transfer gap is reduced down from the interrupt latency plus tasklet latency, to just the interrupt latency.

Controllers such as OMAP (if their hardware scatter chains were used) do have the ability to reprogram the entire channel configuration from an appropriate transaction, and so /could/ start the next transfer entirely automatically - but I never added support for the hardware scatterlists as I have been told that TI measurements indicated that it did not gain any performance to use them. Had this been implemented, it would mean that OMAP would only need to issue an interrupt to notify completion of a transfer (so the driver would only have to work out how many dma transactions had been completed.)

In this case, it is important that we do batch up the entries (since an already in progress descriptor should not be modified), but I suspect in the case of slave DMA, it is rarely the case that there is more than one or two descriptors queued at any moment.

On Mon, Aug 04, 2014 at 06:50:17PM +0200, Laurent Pinchart wrote:

The question was why is there a dma_async_issue_pending() operation at all ? Why can't dmaengine_submit() triggers the transfer start ? The only explanation is a small comment in dmaengine.h that states

• This allows drivers to push copies to HW in batches,
• reducing MMIO writes where possible.

I don't think that's applicable for DMA slave transfers. Is it still applicable for anything else ?

why not?

If your hw supports sg-lists and say length of 8 and you prepare two descriptors for lengths of 3 and 5. While in issue pending what prevents you from submiiting them in one shot to hardware while still getting interrupt.

I know designware and intel-dma do support that. It is different point that drivers don't

On Wed, Jul 23, 2014 at 01:07:43PM +0200, Laurent Pinchart wrote:

Now we have a core issue. On one side there's rsnd_soc_dai_trigger() being called in atomic context, and on the other side the function ends up calling dmaengine_prep_dma_cyclic() which needs to allocate memory. To make this more func the DMA engine API is undocumented and completely silent on whether the prep functions are allowed to sleep. The question is, who's wrong ?

For slave DMA drivers, there is the expectation that the prepare functions will be callable from tasklet context, without any locks held by the driver. So, it's expected that the prepare functions will work from tasklet context.

I don't think we've ever specified whether they should be callable from interrupt context, but in practice, we have drivers which do exactly that, so I think the decision has already been made - they will be callable from IRQ context, and so GFP_ATOMIC is required in the driver.

The rcar-dmac DMA engine driver uses runtime PM. When not used, the device is suspended. The driver calls pm_runtime_get_sync() to resume the device, and needs to do so when a descriptor is submitted. This operation, currently performed in the tx_submit handler, could be moved to the prep_dma_cyclic or issue_pending handler, but the three operations are called in a row from rsnd_dma_start(), itself ultimately called from snd_pcm_lib_write1() with the spinlock held. This means I have no place in my DMA engine driver where I can resume the device.

Right, runtime PM with DMA engine drivers is hard. The best that can be done right now is to pm_runtime_get() in the alloc_chan_resources() method and put it in free_chan_resources() if you don't want to do the workqueue thing.

There's a problem with the workqueue thing though - by doing so, you make it asynchronous to the starting of the DMA. The DMA engine API allows for delayed starting (it's actually the normal thing for DMA engine), but that may not always be appropriate or desirable.

One could argue that the rcar-dmac driver could use a work queue to handle power management. That's correct, but the additional complexity, which would be required in *all* DMA engine drivers, seem too high to me. If we need to go that way, this is really a task that the DMA engine core should handle.

As I mention above, the problem with that is getting the workqueue to run soon enough that it doesn't cause a performance degredation or other issues.

There's also expectations from other code - OMAP for example explicitly needs DMA to be started on the hardware before the audio block can be enabled (from what I remember, it tickless an erratum if this is not done.)

Let's start by answering the background question and updating the DMA engine API documentation once and for good : in which context are drivers allowed to call the prep, tx_submit and issue_pending functions ?

IRQs-off contexts. :)