Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1517614 > unrolled thread

Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-08 23:20 +0100
    Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support Marek Szyprowski <m.szyprowski@samsung.com> - 2016-11-09 16:10 +0100
      Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-10 01:40 +0100
        Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-10 02:20 +0100

#1517614 — Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support

On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
> Hi Luis
> 
> 
> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
> > On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
> > > This patch uses recently introduced device links to track the runtime pm
> > > state of the master's device. This way each SYSMMU controller is runtime
> > > activated when its master's device is active
> > instead of?
> 
> instead of keeping SYSMMU controller runtime active all the time.

I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ? Because as far as I can tell this was painted to help
with suspend/resume ?

> > BTW what is the master device of a SYSMMU? I have no clue about these
> > IOMMU devices here.
> 
> Here is a more detailed description of IOMMU hardware I wrote a few days ago
> for Ulf:
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html
> 
> In short: there is a SYSMMU controller and its master device - a device,
> which performs DMA operations. That SYSMMU sits in between system memory
> and the master device, so it performs mapping of DMA addresses to physical
> memory addresses on each DMA operation.

So you seek a run time power optimization ? Or a fix on suspend? Or both?

> > > and can save/restore its state instead of being enabled all the time.
> > I take it this means currently even if the master device is disabled
> > (whatever that is) all SYSMMU controllers are kept enabled, is that right?
> > The issue here is this wastes power? Or what is the issue?
> 
> Yes, the issue here is the fact that SYSMMU is kept active all the time,
> what in turn prevent the power domain for turning off even if master device
> doesn't do anything and is already suspended. This directly (some clocks
> enabled) and in-directly (leakage current) causes power looses.

Thanks for the confirmation so really the biggest concern here was run time PM.

> > > This way SYSMMU controllers no
> > > longer prevents respective power domains to be turned off when master's
> > > device is not used.
> > So when the master device is idle we want to also remove power from the
> > controllers ? How much power does this save on a typical device in the
> > market BTW ?
> 
> The main purpose of this patchset is to let power domains to be turned off,
> because with the current code all domains are all the time turned on,
> because
> SYSMMU controllers prevent them from turning them off.

I see.. I think the audio folks already addressed this with DAPM, but granted
this was for audio. Then I was also referred to the DRM / Audio component
framework, has this been looked into? v4l folks have v4l async stuff but
its not clear if that help with run time PM. I'm mentioning these given it'd be
silly to re-invent the wheel, additionally if we now have a generic solution
everyone can jump on board with there is quite a bit of work we can do to
dump a lot of old legacy crap.

> If you want I can measure the power consumption of the idle board with all
> domains enabled and disabled if you want to see the numbers. On the other
> board
> disabling most power domains in idle state (the clocks were already
> disabled)
> gave me about 20mA savings (at 3.7V), what is a significant value for the
> battery powered device.

Thanks, this means nothing to me, however it would be value-add to the commit log
as anyone reviewing  this can understand what the goal / savings was for exactly.

> > 
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > ---
> > >   drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
> > >   1 file changed, 94 insertions(+), 131 deletions(-)
> > I'm reviewing the device link patches now but since this is a demo of
> > use of that I'll note the changes here are pretty large and it makes
> > it terribly difficult for review. Is there any way this patch can be split
> > up in to logical atomic pieces that only do one task upon change ?
> 
> I will try to split it a bit, but I cannot promise that much can be done
> to improve readability for someone not very familiar with the driver
> internals.

I've heard this before, I don't buy it but lets see!

  Luis

[toc] | [next] | [standalone]

#1518241

Hi Luis,


On 2016-11-08 23:14, Luis R. Rodriguez wrote:
> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
>> Hi Luis
>>
>>
>> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
>>>> This patch uses recently introduced device links to track the runtime pm
>>>> state of the master's device. This way each SYSMMU controller is runtime
>>>> activated when its master's device is active
>>> instead of?
>> instead of keeping SYSMMU controller runtime active all the time.
> I thought Rafael's work was for suspend/resume, not for runtime suspend.
> Is it for both ?

Yes, it solves both problems, although the suspend/resume was easy to 
workaround
just by using LATE_SLEEP_OPS.

> Because as far as I can tell this was painted to help
> with suspend/resume ?

It also helped to remove the suspend/resume workaround.

>>> BTW what is the master device of a SYSMMU? I have no clue about these
>>> IOMMU devices here.
>> Here is a more detailed description of IOMMU hardware I wrote a few days ago
>> for Ulf:
>> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html
>>
>> In short: there is a SYSMMU controller and its master device - a device,
>> which performs DMA operations. That SYSMMU sits in between system memory
>> and the master device, so it performs mapping of DMA addresses to physical
>> memory addresses on each DMA operation.
> So you seek a run time power optimization ? Or a fix on suspend? Or both?

The main reason for using device links was to implement proper runtime power
optimization.

>>>> and can save/restore its state instead of being enabled all the time.
>>> I take it this means currently even if the master device is disabled
>>> (whatever that is) all SYSMMU controllers are kept enabled, is that right?
>>> The issue here is this wastes power? Or what is the issue?
>> Yes, the issue here is the fact that SYSMMU is kept active all the time,
>> what in turn prevent the power domain for turning off even if master device
>> doesn't do anything and is already suspended. This directly (some clocks
>> enabled) and in-directly (leakage current) causes power looses.
> Thanks for the confirmation so really the biggest concern here was run time PM.
>
>>>> This way SYSMMU controllers no
>>>> longer prevents respective power domains to be turned off when master's
>>>> device is not used.
>>> So when the master device is idle we want to also remove power from the
>>> controllers ? How much power does this save on a typical device in the
>>> market BTW ?
>> The main purpose of this patchset is to let power domains to be turned off,
>> because with the current code all domains are all the time turned on,
>> because
>> SYSMMU controllers prevent them from turning them off.
> I see.. I think the audio folks already addressed this with DAPM, but granted
> this was for audio. Then I was also referred to the DRM / Audio component
> framework, has this been looked into? v4l folks have v4l async stuff but
> its not clear if that help with run time PM. I'm mentioning these given it'd be
> silly to re-invent the wheel, additionally if we now have a generic solution
> everyone can jump on board with there is quite a bit of work we can do to
> dump a lot of old legacy crap.

Right, probably some workarounds here and there can be removed. However 
components
and v4l-async solutions are for resolving only probe and registration 
issues and they
are some kind of pool for grouping devices and triggering special 
callback once all
requested devices in the pool have probed.

>> If you want I can measure the power consumption of the idle board with all
>> domains enabled and disabled if you want to see the numbers. On the other
>> board
>> disabling most power domains in idle state (the clocks were already
>> disabled)
>> gave me about 20mA savings (at 3.7V), what is a significant value for the
>> battery powered device.
> Thanks, this means nothing to me, however it would be value-add to the commit log
> as anyone reviewing  this can understand what the goal / savings was for exactly.
>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
>>>>    1 file changed, 94 insertions(+), 131 deletions(-)
>>> I'm reviewing the device link patches now but since this is a demo of
>>> use of that I'll note the changes here are pretty large and it makes
>>> it terribly difficult for review. Is there any way this patch can be split
>>> up in to logical atomic pieces that only do one task upon change ?
>> I will try to split it a bit, but I cannot promise that much can be done
>> to improve readability for someone not very familiar with the driver
>> internals.
> I've heard this before, I don't buy it but lets see!

Somehow I managed to split this all-in-one patch into several smaller 
changes
in v5 and v6 was posted yesterday ago.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1518600

On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi Luis,
>
>
> On 2016-11-08 23:14, Luis R. Rodriguez wrote:
>>
>> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
>>>
>>> Hi Luis
>>>
>>>
>>> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
>>>>
>>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
>>>>>
>>>>> This patch uses recently introduced device links to track the runtime
>>>>> pm
>>>>> state of the master's device. This way each SYSMMU controller is
>>>>> runtime
>>>>> activated when its master's device is active
>>>>
>>>> instead of?
>>>
>>> instead of keeping SYSMMU controller runtime active all the time.
>>
>> I thought Rafael's work was for suspend/resume, not for runtime suspend.
>> Is it for both ?
>
>
> Yes, it solves both problems, although the suspend/resume was easy to
> workaround just by using LATE_SLEEP_OPS.

Right, but that's just in this particular case, because the dependency
chain is of length 2. :-)

If you had a longer chain, you might in theory use  the _noirq() stage
somehow, but that has limitations.

>> Because as far as I can tell this was painted to help
>> with suspend/resume ?

It helps with three things, (async) suspend/resume, runtime PM and
shutdown (that last part is the hardest to figure out).  The ordering
in which all of these are carried out is analogous and cannot be
determined correctly by the device registration ordering itself in
general (which has been a known fact for years, but some localized
workarounds were put in some places to work around that).

Moreover, even if the list ordering (of dpm_list, for instance) is
correct, it still doesn't guarantee the right ordering of actions that
are carried out asynchronously.  They are all started in the list
order, but they may be running in parallel with each other and
complete at different times.  For this reason, there needs to be a way
to ensure that, say, the suspend operations for consumer devices
complete before their suppliers will become unavailable and so on.

Both runtime PM and system suspend/resume have this problem.  It is
not present in the system shutdown case, but it still helps to get a
correct list ordering (ie. such that won't cause supplier devices to
be shut down before their consumers) in this case too.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1518613

On Thu, Nov 10, 2016 at 01:36:29AM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > Hi Luis,
> >
> >
> > On 2016-11-08 23:14, Luis R. Rodriguez wrote:
> >>
> >> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
> >>>
> >>> Hi Luis
> >>>
> >>>
> >>> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
> >>>>
> >>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
> >>>>>
> >>>>> This patch uses recently introduced device links to track the runtime
> >>>>> pm
> >>>>> state of the master's device. This way each SYSMMU controller is
> >>>>> runtime
> >>>>> activated when its master's device is active
> >>>>
> >>>> instead of?
> >>>
> >>> instead of keeping SYSMMU controller runtime active all the time.
> >>
> >> I thought Rafael's work was for suspend/resume, not for runtime suspend.
> >> Is it for both ?
> >
> >
> > Yes, it solves both problems, although the suspend/resume was easy to
> > workaround just by using LATE_SLEEP_OPS.
> 
> Right, but that's just in this particular case, because the dependency
> chain is of length 2. :-)
> 
> If you had a longer chain, you might in theory use  the _noirq() stage
> somehow, but that has limitations.
> 
> >> Because as far as I can tell this was painted to help
> >> with suspend/resume ?
> 
> It helps with three things, (async) suspend/resume, runtime PM and
> shutdown (that last part is the hardest to figure out).  The ordering
> in which all of these are carried out is analogous and cannot be
> determined correctly by the device registration ordering itself in
> general (which has been a known fact for years, but some localized
> workarounds were put in some places to work around that).

Thanks for the clarification, this is due to the implicit sort you
had explained (and I provided notes for on ksummit-discuss) right?

Can you itemize a few of the workarounds that are used today?
As you clarify below, getting this order of device registration
correct does not necessarily guarantee devices will wait for their
provider to be ready.

> Moreover, even if the list ordering (of dpm_list, for instance) is
> correct, it still doesn't guarantee the right ordering of actions that
> are carried out asynchronously.  They are all started in the list
> order, but they may be running in parallel with each other and
> complete at different times.  For this reason, there needs to be a way
> to ensure that, say, the suspend operations for consumer devices
> complete before their suppliers will become unavailable and so on.

Thanks this helps as well!

> Both runtime PM and system suspend/resume have this problem.  It is
> not present in the system shutdown case, but it still helps to get a
> correct list ordering (ie. such that won't cause supplier devices to
> be shut down before their consumers) in this case too.

Is the fact that its not on shutdown just because we don't care about
being sloppy about shutdown ? Shouldn't some devices care about that
order ?

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web