Hi, On 01-01-17 21:24, Chris Wilson wrote:
On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except for intel_set_rps(). Since intel_set_rps can for example be called from sysfs store functions, there is no guarantee this is already done, so add an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> --- drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4b12637..cc4fbd7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) { - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + /* Wake up the media well, as that takes a lot less + * power than the Render well. + */ + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); valleyview_set_rps(dev_priv, val);Both powerwells are woken for rps. (Taking one but not the other has no benefit, and very misleading.)
The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the existing code in vlv_set_rps_idle(). > The forcewake is already held by the
lower level routines, taking the wakelock in the caller is an optimisation that is only interesting if there is a danger from the forcewake being dropped mid-sequence (due to preemption whatever).
We're also accessing the punit in valleyview_set_rps() and I've seen several patches (in the android x86 kernel) suggesting that we need to take a wakelock while doing this. Regards, Hans _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
