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

[Lars-Peter Clausen]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/