| Message ID | 1507292020-14212-12-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sagar Arun Kamble (2017-10-06 13:13:40) > Defined new struct intel_rc6 to hold RC6 specific state and > intel_ring_pstate to hold ring specific state. > > v2: s/intel_ring_pstate/intel_llc_pstate and rebase. (Chris) > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 10 ++++++++ > drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++++++++++++---------- > 3 files changed, 54 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 470807c..154f231 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -2502,7 +2502,7 @@ static int intel_runtime_suspend(struct device *kdev) > struct drm_i915_private *dev_priv = to_i915(dev); > int ret; > > - if (WARN_ON_ONCE(!(dev_priv->pm.rps.enabled && intel_rc6_enabled()))) > + if (WARN_ON_ONCE(!(dev_priv->pm.rc6.enabled && intel_rc6_enabled()))) > return -ENODEV; > > if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv))) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 45944a8..a07aa71 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1363,8 +1363,18 @@ struct intel_rps { > struct intel_rps_ei ei; > }; > > +struct intel_rc6 { > + bool enabled; > +}; > + > +struct intel_llc_pstate { > + bool configured; > +}; > + > struct intel_gen6_power_mgmt { > struct intel_rps rps; > + struct intel_rc6 rc6; > + struct intel_llc_pstate llc_pstate; > struct delayed_work autoenable_work; > > /* > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 03264fe..df36a6f 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -7873,7 +7873,12 @@ static void intel_init_emon(struct drm_i915_private *dev_priv) > > static inline void intel_update_ring_freq(struct drm_i915_private *i915) > { > + if (READ_ONCE(i915->pm.llc_pstate.configured)) > + return; Tell me about how you expect the locking around this function to be. The READ_ONCE() implies that we are doing a optimistic peek outside of a lock, but then we set configured without acquiring a lock, so I assume we are inside some lock. That looks true for all, we don't need READ_ONCE() anymore as we only inspect inside the mutex (and so READ_ONCE is giving the wrong impression). > + > gen6_update_ring_freq(i915); > + > + i915->pm.llc_pstate.configured = true; > } > > void intel_disable_gt_powersave(struct drm_i915_private *dev_priv) > { > - if (!READ_ONCE(dev_priv->pm.rps.enabled)) > - return; > - > mutex_lock(&dev_priv->pm.pcu_lock); > > intel_disable_rc6(dev_priv); > intel_disable_rps(dev_priv); > + if (HAS_LLC(dev_priv)) > + dev_priv->pm.llc_pstate.configured = false; Always clear it? If no llc, it can never be configured. Hmm, better if we just made it symmetrical with s/intel_update_ring_freq/intel_enable_llc_pstate/ and intel_disable_llc_pstate here. > > - dev_priv->pm.rps.enabled = false; > mutex_unlock(&dev_priv->pm.pcu_lock); > } > @@ -8080,7 +8103,10 @@ static void __intel_autoenable_gt_powersave(struct work_struct *work) > struct intel_engine_cs *rcs; > struct drm_i915_gem_request *req; > > - if (READ_ONCE(dev_priv->pm.rps.enabled)) > + if (READ_ONCE(dev_priv->pm.rps.enabled) && > + READ_ONCE(dev_priv->pm.rc6.enabled) && > + !(HAS_LLC(dev_priv) ^ > + READ_ONCE(dev_priv->pm.llc_pstate.configured))) > goto out; This optimisation has lost its appeal :) Kill it, if we need something like it we can try again later. -Chris
On 10/6/2017 6:25 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2017-10-06 13:13:40) >> Defined new struct intel_rc6 to hold RC6 specific state and >> intel_ring_pstate to hold ring specific state. >> >> v2: s/intel_ring_pstate/intel_llc_pstate and rebase. (Chris) >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 2 +- >> drivers/gpu/drm/i915/i915_drv.h | 10 ++++++++ >> drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++++++++++++---------- >> 3 files changed, 54 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 470807c..154f231 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -2502,7 +2502,7 @@ static int intel_runtime_suspend(struct device *kdev) >> struct drm_i915_private *dev_priv = to_i915(dev); >> int ret; >> >> - if (WARN_ON_ONCE(!(dev_priv->pm.rps.enabled && intel_rc6_enabled()))) >> + if (WARN_ON_ONCE(!(dev_priv->pm.rc6.enabled && intel_rc6_enabled()))) >> return -ENODEV; >> >> if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv))) >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 45944a8..a07aa71 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1363,8 +1363,18 @@ struct intel_rps { >> struct intel_rps_ei ei; >> }; >> >> +struct intel_rc6 { >> + bool enabled; >> +}; >> + >> +struct intel_llc_pstate { >> + bool configured; >> +}; >> + >> struct intel_gen6_power_mgmt { >> struct intel_rps rps; >> + struct intel_rc6 rc6; >> + struct intel_llc_pstate llc_pstate; >> struct delayed_work autoenable_work; >> >> /* >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 03264fe..df36a6f 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -7873,7 +7873,12 @@ static void intel_init_emon(struct drm_i915_private *dev_priv) >> >> static inline void intel_update_ring_freq(struct drm_i915_private *i915) >> { >> + if (READ_ONCE(i915->pm.llc_pstate.configured)) >> + return; > Tell me about how you expect the locking around this function to be. > > The READ_ONCE() implies that we are doing a optimistic peek outside of a > lock, but then we set configured without acquiring a lock, so I assume > we are inside some lock. > > That looks true for all, we don't need READ_ONCE() anymore as we only > inspect inside the mutex (and so READ_ONCE is giving the wrong > impression). > >> + >> gen6_update_ring_freq(i915); >> + >> + i915->pm.llc_pstate.configured = true; >> } >> >> void intel_disable_gt_powersave(struct drm_i915_private *dev_priv) >> { >> - if (!READ_ONCE(dev_priv->pm.rps.enabled)) >> - return; >> - >> mutex_lock(&dev_priv->pm.pcu_lock); >> >> intel_disable_rc6(dev_priv); >> intel_disable_rps(dev_priv); >> + if (HAS_LLC(dev_priv)) >> + dev_priv->pm.llc_pstate.configured = false; > Always clear it? If no llc, it can never be configured. > Hmm, better if we just made it symmetrical with > s/intel_update_ring_freq/intel_enable_llc_pstate/ and > intel_disable_llc_pstate here. Will update. > >> >> - dev_priv->pm.rps.enabled = false; >> mutex_unlock(&dev_priv->pm.pcu_lock); >> } >> @@ -8080,7 +8103,10 @@ static void __intel_autoenable_gt_powersave(struct work_struct *work) >> struct intel_engine_cs *rcs; >> struct drm_i915_gem_request *req; >> >> - if (READ_ONCE(dev_priv->pm.rps.enabled)) >> + if (READ_ONCE(dev_priv->pm.rps.enabled) && >> + READ_ONCE(dev_priv->pm.rc6.enabled) && >> + !(HAS_LLC(dev_priv) ^ >> + READ_ONCE(dev_priv->pm.llc_pstate.configured))) >> goto out; > This optimisation has lost its appeal :) > > Kill it, if we need something like it we can try again later. > -Chris Sure. Understood that using READ_ONCE inside lock was unnecessary. Will remove this triple condition.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 470807c..154f231 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -2502,7 +2502,7 @@ static int intel_runtime_suspend(struct device *kdev) struct drm_i915_private *dev_priv = to_i915(dev); int ret; - if (WARN_ON_ONCE(!(dev_priv->pm.rps.enabled && intel_rc6_enabled()))) + if (WARN_ON_ONCE(!(dev_priv->pm.rc6.enabled && intel_rc6_enabled()))) return -ENODEV; if (WARN_ON_ONCE(!HAS_RUNTIME_PM(dev_priv))) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 45944a8..a07aa71 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1363,8 +1363,18 @@ struct intel_rps { struct intel_rps_ei ei; }; +struct intel_rc6 { + bool enabled; +}; + +struct intel_llc_pstate { + bool configured; +}; + struct intel_gen6_power_mgmt { struct intel_rps rps; + struct intel_rc6 rc6; + struct intel_llc_pstate llc_pstate; struct delayed_work autoenable_work; /* diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 03264fe..df36a6f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -7873,7 +7873,12 @@ static void intel_init_emon(struct drm_i915_private *dev_priv) static inline void intel_update_ring_freq(struct drm_i915_private *i915) { + if (READ_ONCE(i915->pm.llc_pstate.configured)) + return; + gen6_update_ring_freq(i915); + + i915->pm.llc_pstate.configured = true; } void intel_init_gt_powersave(struct drm_i915_private *dev_priv) @@ -7966,7 +7971,8 @@ void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv) void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv) { - dev_priv->pm.rps.enabled = true; /* force disabling */ + dev_priv->pm.rps.enabled = true; /* force RPS disabling */ + dev_priv->pm.rc6.enabled = true; /* force RC6 disabling */ intel_disable_gt_powersave(dev_priv); gen6_reset_rps_interrupts(dev_priv); @@ -7974,6 +7980,9 @@ void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv) static void intel_disable_rc6(struct drm_i915_private *dev_priv) { + if (!READ_ONCE(dev_priv->pm.rc6.enabled)) + return; + if (INTEL_GEN(dev_priv) >= 9) gen9_disable_rc6(dev_priv); else if (IS_CHERRYVIEW(dev_priv)) @@ -7982,10 +7991,15 @@ static void intel_disable_rc6(struct drm_i915_private *dev_priv) valleyview_disable_rc6(dev_priv); else if (INTEL_GEN(dev_priv) >= 6) gen6_disable_rc6(dev_priv); + + dev_priv->pm.rc6.enabled = false; } static void intel_disable_rps(struct drm_i915_private *dev_priv) { + if (!READ_ONCE(dev_priv->pm.rps.enabled)) + return; + if (INTEL_GEN(dev_priv) >= 9) gen9_disable_rps(dev_priv); else if (IS_CHERRYVIEW(dev_priv)) @@ -7996,24 +8010,30 @@ static void intel_disable_rps(struct drm_i915_private *dev_priv) gen6_disable_rps(dev_priv); else if (IS_IRONLAKE_M(dev_priv)) ironlake_disable_drps(dev_priv); + + dev_priv->pm.rps.enabled = false; } void intel_disable_gt_powersave(struct drm_i915_private *dev_priv) { - if (!READ_ONCE(dev_priv->pm.rps.enabled)) - return; - mutex_lock(&dev_priv->pm.pcu_lock); intel_disable_rc6(dev_priv); intel_disable_rps(dev_priv); + if (HAS_LLC(dev_priv)) + dev_priv->pm.llc_pstate.configured = false; - dev_priv->pm.rps.enabled = false; mutex_unlock(&dev_priv->pm.pcu_lock); } static void intel_enable_rc6(struct drm_i915_private *dev_priv) { + /* We shouldn't be disabling as we submit, so this should be less + * racy than it appears! + */ + if (READ_ONCE(dev_priv->pm.rc6.enabled)) + return; + if (IS_CHERRYVIEW(dev_priv)) cherryview_enable_rc6(dev_priv); else if (IS_VALLEYVIEW(dev_priv)) @@ -8024,10 +8044,18 @@ static void intel_enable_rc6(struct drm_i915_private *dev_priv) gen8_enable_rc6(dev_priv); else if (INTEL_GEN(dev_priv) >= 6) gen6_enable_rc6(dev_priv); + + dev_priv->pm.rc6.enabled = true; } static void intel_enable_rps(struct drm_i915_private *dev_priv) { + /* We shouldn't be disabling as we submit, so this should be less + * racy than it appears! + */ + if (READ_ONCE(dev_priv->pm.rps.enabled)) + return; + if (IS_CHERRYVIEW(dev_priv)) { cherryview_enable_rps(dev_priv); } else if (IS_VALLEYVIEW(dev_priv)) { @@ -8048,16 +8076,12 @@ static void intel_enable_rps(struct drm_i915_private *dev_priv) WARN_ON(dev_priv->pm.rps.efficient_freq < dev_priv->pm.rps.min_freq); WARN_ON(dev_priv->pm.rps.efficient_freq > dev_priv->pm.rps.max_freq); + + dev_priv->pm.rps.enabled = true; } void intel_enable_gt_powersave(struct drm_i915_private *dev_priv) { - /* We shouldn't be disabling as we submit, so this should be less - * racy than it appears! - */ - if (READ_ONCE(dev_priv->pm.rps.enabled)) - return; - /* Powersaving is controlled by the host when inside a VM */ if (intel_vgpu_active(dev_priv)) return; @@ -8069,7 +8093,6 @@ void intel_enable_gt_powersave(struct drm_i915_private *dev_priv) if (HAS_LLC(dev_priv)) intel_update_ring_freq(dev_priv); - dev_priv->pm.rps.enabled = true; mutex_unlock(&dev_priv->pm.pcu_lock); } @@ -8080,7 +8103,10 @@ static void __intel_autoenable_gt_powersave(struct work_struct *work) struct intel_engine_cs *rcs; struct drm_i915_gem_request *req; - if (READ_ONCE(dev_priv->pm.rps.enabled)) + if (READ_ONCE(dev_priv->pm.rps.enabled) && + READ_ONCE(dev_priv->pm.rc6.enabled) && + !(HAS_LLC(dev_priv) ^ + READ_ONCE(dev_priv->pm.llc_pstate.configured))) goto out; rcs = dev_priv->engine[RCS]; @@ -8110,7 +8136,10 @@ static void __intel_autoenable_gt_powersave(struct work_struct *work) void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv) { - if (READ_ONCE(dev_priv->pm.rps.enabled)) + if (READ_ONCE(dev_priv->pm.rps.enabled) && + READ_ONCE(dev_priv->pm.rc6.enabled) && + !(HAS_LLC(dev_priv) ^ + READ_ONCE(dev_priv->pm.llc_pstate.configured))) return; if (IS_IRONLAKE_M(dev_priv)) {
Defined new struct intel_rc6 to hold RC6 specific state and intel_ring_pstate to hold ring specific state. v2: s/intel_ring_pstate/intel_llc_pstate and rebase. (Chris) Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 10 ++++++++ drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++++++++++++---------- 3 files changed, 54 insertions(+), 15 deletions(-)