Messages in this thread | | | Date | Fri, 10 Apr 2015 12:47:45 +0200 | From | Lars-Peter Clausen <> | Subject | Re: [PATCH] dmaengine: pl330: get rid of pm_runtime_irq_safe() |
| |
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
| |