| Message ID | 1341240671-5843-5-git-send-email-eugeni.dodonov@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2 Jul 2012 11:51:05 -0300 Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > Most of the RPS and RC6 enabling functionality is similar to what we had > on Gen6/Gen7, so we preserve most of the registers. > > Note that Haswell only has RC6, so account for that as well. As suggested > by Daniel Vetter, to reduce the amount of changes in the patch, we still > write the RC6p/RC6pp thresholds, but those are ignored on Haswell. > > Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++++++++++++++------------ > 2 files changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index f17de3d..9d5bf06 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4156,6 +4156,7 @@ > #define GEN6_RP_UP_IDLE_MIN (0x1<<3) > #define GEN6_RP_UP_BUSY_AVG (0x2<<3) > #define GEN6_RP_UP_BUSY_CONT (0x4<<3) > +#define GEN7_RP_DOWN_IDLE_AVG (0x2<<0) > #define GEN6_RP_DOWN_IDLE_CONT (0x1<<0) > #define GEN6_RP_UP_THRESHOLD 0xA02C > #define GEN6_RP_DOWN_THRESHOLD 0xA030 > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 29720d2..a3ee1b1 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2402,20 +2402,24 @@ static void gen6_enable_rps(struct drm_device *dev) > I915_WRITE(GEN6_RC6p_THRESHOLD, 100000); > I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */ > > + /* Check if we are enabling RC6 */ > rc6_mode = intel_enable_rc6(dev_priv->dev); > if (rc6_mode & INTEL_RC6_ENABLE) > rc6_mask |= GEN6_RC_CTL_RC6_ENABLE; > > - if (rc6_mode & INTEL_RC6p_ENABLE) > - rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE; > + /* We don't use those on Haswell */ > + if (!IS_HASWELL(dev)) { > + if (rc6_mode & INTEL_RC6p_ENABLE) > + rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE; > > - if (rc6_mode & INTEL_RC6pp_ENABLE) > - rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE; > + if (rc6_mode & INTEL_RC6pp_ENABLE) > + rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE; > + } > > DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n", > - (rc6_mode & INTEL_RC6_ENABLE) ? "on" : "off", > - (rc6_mode & INTEL_RC6p_ENABLE) ? "on" : "off", > - (rc6_mode & INTEL_RC6pp_ENABLE) ? "on" : "off"); > + (rc6_mask & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off", > + (rc6_mask & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off", > + (rc6_mask & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off"); Belongs in a separate patch, but meh. > > I915_WRITE(GEN6_RC_CONTROL, > rc6_mask | > @@ -2433,10 +2437,19 @@ static void gen6_enable_rps(struct drm_device *dev) > I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, > dev_priv->max_delay << 24 | > dev_priv->min_delay << 16); > - I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000); > - I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000); > - I915_WRITE(GEN6_RP_UP_EI, 100000); > - I915_WRITE(GEN6_RP_DOWN_EI, 5000000); > + > + if (IS_HASWELL(dev)) { > + I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400); > + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000); > + I915_WRITE(GEN6_RP_UP_EI, 66000); > + I915_WRITE(GEN6_RP_DOWN_EI, 350000); > + } else { > + I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000); > + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000); > + I915_WRITE(GEN6_RP_UP_EI, 100000); > + I915_WRITE(GEN6_RP_DOWN_EI, 5000000); > + } > + I can't really comment much on this other than what I said below. It'd be nice if the policy didn't change between platforms unless we know/understand why. > I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10); > I915_WRITE(GEN6_RP_CONTROL, > GEN6_RP_MEDIA_TURBO | > @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev) > GEN6_RP_MEDIA_IS_GFX | > GEN6_RP_ENABLE | > GEN6_RP_UP_BUSY_AVG | > - GEN6_RP_DOWN_IDLE_CONT); > + (IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT); I think this warrants an explanation. I think we would ideally like to avoid different algorithms for different platforms. > > if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0, > 500))
On 07/02/2012 02:49 PM, Ben Widawsky wrote: > On Mon, 2 Jul 2012 11:51:05 -0300 > Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > >> Most of the RPS and RC6 enabling functionality is similar to what we had >> on Gen6/Gen7, so we preserve most of the registers. >> >> Note that Haswell only has RC6, so account for that as well. As suggested >> by Daniel Vetter, to reduce the amount of changes in the patch, we still >> write the RC6p/RC6pp thresholds, but those are ignored on Haswell. >> >> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> Daniel, to please Ben Widawsky, you could add an extra paragraph in my message, saying: 'On Haswell, the suggested values for the RP UP and RP DOWN interrupts have changed in a way to have lower thresholds for all the operations. However, the suggested values for pre-Haswell machines stay the same. In order to follow what the hardware designers suggest, we program those as suggested for each generation now. Also, Haswell introduces a new scheduling bit for the RP DOWN scheduling, which we now use by default'. Eugeni
On Mon, 02 Jul 2012 17:02:59 -0300 Eugeni Dodonov <eugeni.dodonov@linux.intel.com> wrote: > On 07/02/2012 02:49 PM, Ben Widawsky wrote: > > On Mon, 2 Jul 2012 11:51:05 -0300 > > Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > > > >> Most of the RPS and RC6 enabling functionality is similar to what we had > >> on Gen6/Gen7, so we preserve most of the registers. > >> > >> Note that Haswell only has RC6, so account for that as well. As suggested > >> by Daniel Vetter, to reduce the amount of changes in the patch, we still > >> write the RC6p/RC6pp thresholds, but those are ignored on Haswell. > >> > >> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > Daniel, to please Ben Widawsky, you could add an extra paragraph in my > message, saying: > > 'On Haswell, the suggested values for the RP UP and RP DOWN interrupts > have changed in a way to have lower thresholds for all the operations. > However, the suggested values for pre-Haswell machines stay the same. In > order to follow what the hardware designers suggest, we program those as > suggested for each generation now. > > Also, Haswell introduces a new scheduling bit for the RP DOWN > scheduling, which we now use by default'. > > Eugeni The code itself states the above perfectly well. I was really trying to understand the, "why."
On Mon, 2 Jul 2012 11:51:05 -0300, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10); > I915_WRITE(GEN6_RP_CONTROL, > GEN6_RP_MEDIA_TURBO | > @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev) > GEN6_RP_MEDIA_IS_GFX | > GEN6_RP_ENABLE | > GEN6_RP_UP_BUSY_AVG | > - GEN6_RP_DOWN_IDLE_CONT); > + (IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT); And with one small typo does everything break... -Chris
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f17de3d..9d5bf06 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4156,6 +4156,7 @@ #define GEN6_RP_UP_IDLE_MIN (0x1<<3) #define GEN6_RP_UP_BUSY_AVG (0x2<<3) #define GEN6_RP_UP_BUSY_CONT (0x4<<3) +#define GEN7_RP_DOWN_IDLE_AVG (0x2<<0) #define GEN6_RP_DOWN_IDLE_CONT (0x1<<0) #define GEN6_RP_UP_THRESHOLD 0xA02C #define GEN6_RP_DOWN_THRESHOLD 0xA030 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 29720d2..a3ee1b1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2402,20 +2402,24 @@ static void gen6_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_RC6p_THRESHOLD, 100000); I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */ + /* Check if we are enabling RC6 */ rc6_mode = intel_enable_rc6(dev_priv->dev); if (rc6_mode & INTEL_RC6_ENABLE) rc6_mask |= GEN6_RC_CTL_RC6_ENABLE; - if (rc6_mode & INTEL_RC6p_ENABLE) - rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE; + /* We don't use those on Haswell */ + if (!IS_HASWELL(dev)) { + if (rc6_mode & INTEL_RC6p_ENABLE) + rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE; - if (rc6_mode & INTEL_RC6pp_ENABLE) - rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE; + if (rc6_mode & INTEL_RC6pp_ENABLE) + rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE; + } DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n", - (rc6_mode & INTEL_RC6_ENABLE) ? "on" : "off", - (rc6_mode & INTEL_RC6p_ENABLE) ? "on" : "off", - (rc6_mode & INTEL_RC6pp_ENABLE) ? "on" : "off"); + (rc6_mask & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off", + (rc6_mask & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off", + (rc6_mask & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off"); I915_WRITE(GEN6_RC_CONTROL, rc6_mask | @@ -2433,10 +2437,19 @@ static void gen6_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, dev_priv->max_delay << 24 | dev_priv->min_delay << 16); - I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000); - I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000); - I915_WRITE(GEN6_RP_UP_EI, 100000); - I915_WRITE(GEN6_RP_DOWN_EI, 5000000); + + if (IS_HASWELL(dev)) { + I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400); + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000); + I915_WRITE(GEN6_RP_UP_EI, 66000); + I915_WRITE(GEN6_RP_DOWN_EI, 350000); + } else { + I915_WRITE(GEN6_RP_UP_THRESHOLD, 10000); + I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 1000000); + I915_WRITE(GEN6_RP_UP_EI, 100000); + I915_WRITE(GEN6_RP_DOWN_EI, 5000000); + } + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10); I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO | @@ -2444,7 +2457,7 @@ static void gen6_enable_rps(struct drm_device *dev) GEN6_RP_MEDIA_IS_GFX | GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG | - GEN6_RP_DOWN_IDLE_CONT); + (IS_HASWELL(dev)) ? GEN7_RP_DOWN_IDLE_AVG : GEN6_RP_DOWN_IDLE_CONT); if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0, 500))
Most of the RPS and RC6 enabling functionality is similar to what we had on Gen6/Gen7, so we preserve most of the registers. Note that Haswell only has RC6, so account for that as well. As suggested by Daniel Vetter, to reduce the amount of changes in the patch, we still write the RC6p/RC6pp thresholds, but those are ignored on Haswell. Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++++++++++++++------------ 2 files changed, 26 insertions(+), 12 deletions(-)