On 04/10/2015 11:57 AM, Robert Baldyga wrote:
On 04/10/2015 10:04 AM, Lars-Peter Clausen wrote:On 04/10/2015 08:57 AM, Robert Baldyga wrote:As using pm_runtime_irq_safe() causes power domain is always enabled, we want to get rid of it to reach better power efficiency. For this purpose we call pm_runtime_get()/pm_runtime_put() only in DMA channel allocate/free code. DMA channels are always requested and freed in non-atomic context, so we don't need pm_runtime_irq_safe().I wonder how useful this is considering that pretty much always a channel is requested. I think we need an extension to the dmaengine API that allows a channel consumer to notify the driver that the channel that it requested is currently not in use. E.g. something like dmaengine_pm_{get,put}(struct dma_chan *). These functions would have the restriction that they can only be called from a non-atomic context, whereas issue_pending() and friends can still be called from a atomic context. So dmaengine_pm_get() would kind of be a notification that consumer intends to do something in the near future whereas dmaengine_pm_put() would be a notification that it is not going to use the channel in the near future. E.g. for audio DMA the audio driver could call dmaengine_pm_get() when the PCM device is opened and dmaengine_pm_put() when it is closed. Whereas issue_pending is called when the audio is started.I see. I'm considering how to do it. It would need to make changes in all clients, or at least doing dmaengine_pm_get() by default while requesting channel.
To maintain backwards compatibility when a channel is requested it should implicitly also get a reference. If the driver doesn't need the channel immediately it can call dmaengine_pm_put() after requesting the channel. This allows adding support for this to the dmaengine clients one by one rather than having to do it for all at once.
However separating clock enable/disable (which can be done in atomic
context) from runtime PM (which we prefer to use in non-atomic context)
still seems to be good idea. While dmaengine_pm_{get,put} could be
called when client is going to use DMA channel in near future, clock
could be enabled on demand and disabled immediately after each
operation. It can provide some gain, especially in cases when time
interval between dmaengine_pm_get() and dmaengine_pm_put() is much
longer than period when we actually are using DMA hardware.
Yes, dmaengine_pm_{get,put} would be a incremental improvement on top of this patch. I'm just trying to point out that doing pm_runtime_get_sync() in alloc_chan_resources() alone is not that effective from a runtime power-management point of view since you pretty much always have a channel requested. So you always end up with a reference and no power-saving will happen.
- Lars -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
