Re: [Intel-gfx] [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
On Thu, May 09, 2019 at 09:17:54AM +0100, Chris Wilson wrote: > Quoting Imre Deak (2019-05-09 07:19:47) > > By disabling a power domain asynchronously we can restrict holding a > > reference on that power domain to the actual code sequence that > > requires the power to be on for the HW access it's doing, by also > > avoiding unneeded on-off-on togglings of the power domain (since the > > disabling happens with a delay). > > > > One benefit is potential power saving due to the following two reasons: > > 1. The fact that we will now be holding the reference only for the > >necessary duration by the end of the patchset. While simply not > >delaying the disabling has the same benefit, it has the problem that > >frequent on-off-on power switching has its own power cost (see the 2. > >point below) and the debug trace for power well on/off events will > >cause a lot of dmesg spam (see details about this further below). > > 2. Avoiding the power cost of freuqent on-off-on power switching. This > >requires us to find the optimal disabling delay based on the measured > >power cost of on->off and off->on switching of each power well vs. > >the power of keeping the given power well on. > > > >In this patchset I'm not providing this optimal delay for two > >reasons: > >a) I don't have the means yet to perform the measurement (with high > > enough signal-to-noise ratio, or with the help of an energy > > counter that takes switching into account). I'm currently looking > > for a way to measure this. > > > >b) Before reducing the disabling delay we need an alternative way for > > debug tracing powerwell on/off events. Simply avoiding/throttling > > the debug messages is not a solution, see further below. > > > >Note that even in the case where we can't measure any considerable > >power cost of frequent on-off switching of powerwells, it still would > >make sense to do the disabling asynchronously (with 0 delay) to avoid > >blocking on the disabling. On VLV I measured this disabling time > >overhead to be 1ms on average with a worst case of 4ms. > > Good point. Again, that would be nice to show in a microbenchmark -- the > latency will still impact the wakeup path (I expect), I thought intel_display_power_grab_async_put_ref() would alleviate that. > the challenge is > identifying userspace path impacted and relating that to workloads. The > further challenge is that since this is power centric, we need to > measure the power vs latency / throughput of such workloads. Ok. I can only promise to do these as a follow-up. We'd need most of all some way for the power measurement, I asked about it from Art et al, but ideas are welcome. The time measurement is easier, I will put together some report about it across all platforms. > > > In the case of the AUX power domains on ICL we would also need to keep > > the sequence where we hold the power reference short, the way it would > > be by the end of this patchset where we hold it only for the actual AUX > > transfer. Anything else would make the locking we need for ICL TypeC > > ports (whenever we hold a reference on any AUX power domain) rather > > problematic, adding for instance unnecessary lockdep dependencies to > > the required TypeC port lock. > > > > I chose the disabling delay to be 100msec for now to avoid the unneeded > > toggling (and so not to introduce dmesg spamming) in the DP MST sideband > > signaling code. We could optimize this delay later, once we have the > > means to measure the switching power cost (see above). > > > > Note that simply removing/throttling the debug tracing for power well > > on/off events is not a solution. We need to know the exact spots of > > these events and cannot rely only on incorrect register accesses caught > > (due to not holding a wakeref at the time of access). Incorrect > > powerwell enabling/disabling could lead to other problems, for instance > > we need to keep certain powerwells enabled for the duration of modesets > > and AUX transfers. > > > > v2: > > - Clarify the commit log parts about power cost measurement and the > > problem of simply removing/throttling debug tracing. (Chris) > > - Optimize out local wakeref vars at intel_runtime_pm_put_raw() and > > intel_display_power_put_async() call sites if > > CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris) > > - Rebased on v2 of the wakeref w/o power-on guarantee patch. > > - Add missing docbook headers. > > > > Cc: Chris Wilson > > Cc: Ville Syrjala > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/i915_drv.h | 5 + > > drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++- > > drivers/gpu/drm/i915/intel_runtime_pm.h | 8 + > > 3 files changed, 368 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index d0257808734c..5801f5407589 100644 > >
Re: [Intel-gfx] [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
Quoting Imre Deak (2019-05-09 07:19:47)
> By disabling a power domain asynchronously we can restrict holding a
> reference on that power domain to the actual code sequence that
> requires the power to be on for the HW access it's doing, by also
> avoiding unneeded on-off-on togglings of the power domain (since the
> disabling happens with a delay).
>
> One benefit is potential power saving due to the following two reasons:
> 1. The fact that we will now be holding the reference only for the
>necessary duration by the end of the patchset. While simply not
>delaying the disabling has the same benefit, it has the problem that
>frequent on-off-on power switching has its own power cost (see the 2.
>point below) and the debug trace for power well on/off events will
>cause a lot of dmesg spam (see details about this further below).
> 2. Avoiding the power cost of freuqent on-off-on power switching. This
>requires us to find the optimal disabling delay based on the measured
>power cost of on->off and off->on switching of each power well vs.
>the power of keeping the given power well on.
>
>In this patchset I'm not providing this optimal delay for two
>reasons:
>a) I don't have the means yet to perform the measurement (with high
> enough signal-to-noise ratio, or with the help of an energy
> counter that takes switching into account). I'm currently looking
> for a way to measure this.
>
>b) Before reducing the disabling delay we need an alternative way for
> debug tracing powerwell on/off events. Simply avoiding/throttling
> the debug messages is not a solution, see further below.
>
>Note that even in the case where we can't measure any considerable
>power cost of frequent on-off switching of powerwells, it still would
>make sense to do the disabling asynchronously (with 0 delay) to avoid
>blocking on the disabling. On VLV I measured this disabling time
>overhead to be 1ms on average with a worst case of 4ms.
Good point. Again, that would be nice to show in a microbenchmark -- the
latency will still impact the wakeup path (I expect), the challenge is
identifying userspace path impacted and relating that to workloads. The
further challenge is that since this is power centric, we need to
measure the power vs latency / throughput of such workloads.
> In the case of the AUX power domains on ICL we would also need to keep
> the sequence where we hold the power reference short, the way it would
> be by the end of this patchset where we hold it only for the actual AUX
> transfer. Anything else would make the locking we need for ICL TypeC
> ports (whenever we hold a reference on any AUX power domain) rather
> problematic, adding for instance unnecessary lockdep dependencies to
> the required TypeC port lock.
>
> I chose the disabling delay to be 100msec for now to avoid the unneeded
> toggling (and so not to introduce dmesg spamming) in the DP MST sideband
> signaling code. We could optimize this delay later, once we have the
> means to measure the switching power cost (see above).
>
> Note that simply removing/throttling the debug tracing for power well
> on/off events is not a solution. We need to know the exact spots of
> these events and cannot rely only on incorrect register accesses caught
> (due to not holding a wakeref at the time of access). Incorrect
> powerwell enabling/disabling could lead to other problems, for instance
> we need to keep certain powerwells enabled for the duration of modesets
> and AUX transfers.
>
> v2:
> - Clarify the commit log parts about power cost measurement and the
> problem of simply removing/throttling debug tracing. (Chris)
> - Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
> intel_display_power_put_async() call sites if
> CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
> - Rebased on v2 of the wakeref w/o power-on guarantee patch.
> - Add missing docbook headers.
>
> Cc: Chris Wilson
> Cc: Ville Syrjala
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +
> drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++-
> drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
> 3 files changed, 368 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d0257808734c..5801f5407589 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -834,6 +834,11 @@ struct i915_power_domains {
>
> struct mutex lock;
> int domain_use_count[POWER_DOMAIN_NUM];
> +
> + struct delayed_work async_put_work;
> + intel_wakeref_t async_put_wakeref;
> + u64 async_put_domains[2];
Fwiw, in the forcewake code we used the term 'auto' for the extra
reference held by the timer for the delayed released.
There we used a timer per fw_domain, which are much fewer in number than
power_domains!, but that has the
[Intel-gfx] [PATCH v2 04/11] drm/i915: Add support for asynchronous display power disabling
By disabling a power domain asynchronously we can restrict holding a
reference on that power domain to the actual code sequence that
requires the power to be on for the HW access it's doing, by also
avoiding unneeded on-off-on togglings of the power domain (since the
disabling happens with a delay).
One benefit is potential power saving due to the following two reasons:
1. The fact that we will now be holding the reference only for the
necessary duration by the end of the patchset. While simply not
delaying the disabling has the same benefit, it has the problem that
frequent on-off-on power switching has its own power cost (see the 2.
point below) and the debug trace for power well on/off events will
cause a lot of dmesg spam (see details about this further below).
2. Avoiding the power cost of freuqent on-off-on power switching. This
requires us to find the optimal disabling delay based on the measured
power cost of on->off and off->on switching of each power well vs.
the power of keeping the given power well on.
In this patchset I'm not providing this optimal delay for two
reasons:
a) I don't have the means yet to perform the measurement (with high
enough signal-to-noise ratio, or with the help of an energy
counter that takes switching into account). I'm currently looking
for a way to measure this.
b) Before reducing the disabling delay we need an alternative way for
debug tracing powerwell on/off events. Simply avoiding/throttling
the debug messages is not a solution, see further below.
Note that even in the case where we can't measure any considerable
power cost of frequent on-off switching of powerwells, it still would
make sense to do the disabling asynchronously (with 0 delay) to avoid
blocking on the disabling. On VLV I measured this disabling time
overhead to be 1ms on average with a worst case of 4ms.
In the case of the AUX power domains on ICL we would also need to keep
the sequence where we hold the power reference short, the way it would
be by the end of this patchset where we hold it only for the actual AUX
transfer. Anything else would make the locking we need for ICL TypeC
ports (whenever we hold a reference on any AUX power domain) rather
problematic, adding for instance unnecessary lockdep dependencies to
the required TypeC port lock.
I chose the disabling delay to be 100msec for now to avoid the unneeded
toggling (and so not to introduce dmesg spamming) in the DP MST sideband
signaling code. We could optimize this delay later, once we have the
means to measure the switching power cost (see above).
Note that simply removing/throttling the debug tracing for power well
on/off events is not a solution. We need to know the exact spots of
these events and cannot rely only on incorrect register accesses caught
(due to not holding a wakeref at the time of access). Incorrect
powerwell enabling/disabling could lead to other problems, for instance
we need to keep certain powerwells enabled for the duration of modesets
and AUX transfers.
v2:
- Clarify the commit log parts about power cost measurement and the
problem of simply removing/throttling debug tracing. (Chris)
- Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
intel_display_power_put_async() call sites if
CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
- Rebased on v2 of the wakeref w/o power-on guarantee patch.
- Add missing docbook headers.
Cc: Chris Wilson
Cc: Ville Syrjala
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/i915_drv.h | 5 +
drivers/gpu/drm/i915/intel_runtime_pm.c | 362 +++-
drivers/gpu/drm/i915/intel_runtime_pm.h | 8 +
3 files changed, 368 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0257808734c..5801f5407589 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -834,6 +834,11 @@ struct i915_power_domains {
struct mutex lock;
int domain_use_count[POWER_DOMAIN_NUM];
+
+ struct delayed_work async_put_work;
+ intel_wakeref_t async_put_wakeref;
+ u64 async_put_domains[2];
+
struct i915_power_well *power_wells;
};
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 89d6309bb1f7..829d483d27e9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -60,6 +60,19 @@
* present for a given platform.
*/
+static intel_wakeref_t intel_runtime_pm_get_raw(struct drm_i915_private *i915);
+static void
+__intel_runtime_pm_put(struct drm_i915_private *i915, intel_wakeref_t wref,
+ bool wakelock);
+
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+static void
+intel_runtime_pm_put_raw(struct drm_i915_private *i915, intel_wakeref_t wref);
+#else
+#define intel_runtime_pm_put_raw(i915, wref) \
+
