Re: [PATCH] dmaengine: pl330: get rid of pm_runtime_irq_safe()

[Lars-Peter Clausen <lars@metafoo.de>]

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