Re: [PATCH 03/31] drm/i915: Separate RPS and RC6 handling for BDW

Szwichtenberg, Radoslaw
2017-09-20T12:31:37Z

On Wed, 2017-09-20 at 11:14 +0000, Szwichtenberg, Radoslaw wrote:
> On Tue, 2017-09-19 at 23:11 +0530, Sagar Arun Kamble wrote:
> > This patch separates RC6 and RPS enabling for BDW.
> > RC6/RPS Disabling are handled through gen6 functions.
> >
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 27 +++++++++++++++------------
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index f78a1e8..6de69ae 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -6613,7 +6613,7 @@ static void gen9_enable_rc6(struct drm_i915_private
> > *dev_priv)
> >   intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >
> > -static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> > +static void gen8_enable_rc6(struct drm_i915_private *dev_priv)
> >  {
> >   struct intel_engine_cs *engine;
> >   enum intel_engine_id id;
> > @@ -6645,16 +6645,18 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >   if (intel_enable_rc6() & INTEL_RC6_ENABLE)
> >   rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> >   intel_print_rc6_info(dev_priv, rc6_mask);
> > - if (IS_BROADWELL(dev_priv))
> > - I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > - GEN7_RC_CTL_TO_MODE |
> > - rc6_mask);
> > - else
> > - I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > - GEN6_RC_CTL_EI_MODE(1) |
> > - rc6_mask);
> > + I915_WRITE(GEN6_RC_CONTROL, GEN6_RC_CTL_HW_ENABLE |
> > + GEN7_RC_CTL_TO_MODE |
> > + rc6_mask);
> >
> > - /* 4 Program defaults and thresholds for RPS*/
> > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> > +}
> > +
> > +static void gen8_enable_rps(struct drm_i915_private *dev_priv)
> > +{
> > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > + /* 1 Program defaults and thresholds for RPS*/
> >   I915_WRITE(GEN6_RPNSWREQ,
> >      HSW_FREQUENCY(dev_priv->rps.rp1_freq));
> >   I915_WRITE(GEN6_RC_VIDEO_FREQ,
> > @@ -6674,7 +6676,7 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >
> >   I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> >
> > - /* 5: Enable RPS */
> > + /* 2: Enable RPS */
> >   I915_WRITE(GEN6_RP_CONTROL,
> >      GEN6_RP_MEDIA_TURBO |
> >      GEN6_RP_MEDIA_HW_NORMAL_MODE |
> > @@ -6683,7 +6685,7 @@ static void gen8_enable_rps(struct drm_i915_private
> > *dev_priv)
> >      GEN6_RP_UP_BUSY_AVG |
> >      GEN6_RP_DOWN_IDLE_AVG);
> >
> > - /* 6: Ring frequency + overclocking (our driver does this later */
> > + /* 3: Ring frequency + overclocking (our driver does this later */
>
> This comment looks invalid (no overclocking done here). Also closing bracket
> missing and maybe one white line to be removed :)
>
> -Radek
Forgot to mention - beside this comment all changes look good to me (you can
have my r-b after this is fixed).
> >
> >   reset_rps(dev_priv, gen6_set_rps);
> >
> > @@ -7976,6 +7978,7 @@ void intel_enable_gt_powersave(struct drm_i915_private
> > *dev_priv)
> >   if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv))
> >   gen6_update_ring_freq(dev_priv);
> >   } else if (IS_BROADWELL(dev_priv)) {
> > + gen8_enable_rc6(dev_priv);
> >   gen8_enable_rps(dev_priv);
> >   gen6_update_ring_freq(dev_priv);
> >   } else if (INTEL_GEN(dev_priv) >= 6) {
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx