| Message ID | 1395279079-12704-9-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 19, 2014 at 06:31:15PM -0700, Ben Widawsky wrote: > We have a need for duplicated parsing of the RP_STATE_CAPS register (and > the setting of the associated fields). To reuse some code, we can > extract the function into a simple helper. > > This patch also addresses the fact that we missed doing this for gen8, > something we should have done anyway. > > This could be two patches, one to extract, and one to add gen8, but it's > trivial enough that I think one is fine. I will accept a request to > split it. Please notice the fix addressed by v2 below. > > Valleyview is left untouched because it is different. > > v2: Logically rebased on top of > commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac > Author: Jeff McGee <jeff.mcgee@intel.com> > Date: Tue Feb 4 11:32:31 2014 -0600 > > drm/i915: Restore rps/rc6 on reset > > Note with the above change the fix for gen8 is also handled (which was > not the case in Jeff's original patch). > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> By setting max_freq_soft before querying overclocking frequencies, we force the user to have to manually raise the max freq through sysfs, right? Hasn't the user already explicitly asked for overclocking through the BIOS setting in the first place, so isn't that a needless burden upon the user? -Chris
On Thu, Mar 20, 2014 at 07:28:29AM +0000, Chris Wilson wrote: > On Wed, Mar 19, 2014 at 06:31:15PM -0700, Ben Widawsky wrote: > > We have a need for duplicated parsing of the RP_STATE_CAPS register (and > > the setting of the associated fields). To reuse some code, we can > > extract the function into a simple helper. > > > > This patch also addresses the fact that we missed doing this for gen8, > > something we should have done anyway. > > > > This could be two patches, one to extract, and one to add gen8, but it's > > trivial enough that I think one is fine. I will accept a request to > > split it. Please notice the fix addressed by v2 below. > > > > Valleyview is left untouched because it is different. > > > > v2: Logically rebased on top of > > commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac > > Author: Jeff McGee <jeff.mcgee@intel.com> > > Date: Tue Feb 4 11:32:31 2014 -0600 > > > > drm/i915: Restore rps/rc6 on reset > > > > Note with the above change the fix for gen8 is also handled (which was > > not the case in Jeff's original patch). > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > By setting max_freq_soft before querying overclocking frequencies, we > force the user to have to manually raise the max freq through sysfs, > right? Hasn't the user already explicitly asked for overclocking through > the BIOS setting in the first place, so isn't that a needless burden > upon the user? > -Chris > It's debatable, and if my memory serves, we've debated it before. Overclocking has a range. BIOS enables the user to select a value within that range. Selecting the highest possible value for the user is a policy decision (IMO). If BIOS/punit wanted to control this, it should set rp0 equal to the max overclock frequency, and not even bother letting the driver deal with it. By not selecting anything, we're not making any decision. Daniel, please notice he did put the r-b tag on it.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8a64ecc..ab9e992 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3248,6 +3248,27 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev) I915_WRITE(GEN6_PMINTRMSK, ~enabled_intrs); } +static void parse_rp_state_cap(struct drm_i915_private *dev_priv, u32 rp_state_cap) +{ + /* All of these values are in units of 50MHz */ + dev_priv->rps.cur_freq = 0; + /* static values from HW: RP0 < RPe < RP1 < RPn (min_freq) */ + dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff; + dev_priv->rps.rp0_freq = (rp_state_cap >> 0) & 0xff; + dev_priv->rps.min_freq = (rp_state_cap >> 16) & 0xff; + /* XXX: only BYT has a special efficient freq */ + dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq; + /* hw_max = RP0 until we check for overclocking */ + dev_priv->rps.max_freq = dev_priv->rps.rp0_freq; + + /* Preserve min/max settings in case of re-init */ + if (dev_priv->rps.max_freq_softlimit == 0) + dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq; + + if (dev_priv->rps.min_freq_softlimit == 0) + dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq; +} + static void gen8_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3266,6 +3287,7 @@ static void gen8_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_RC_CONTROL, 0); rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + parse_rp_state_cap(dev_priv, rp_state_cap); /* 2b: Program RC6 thresholds.*/ I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16); @@ -3354,23 +3376,7 @@ static void gen6_enable_rps(struct drm_device *dev) rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS); - /* All of these values are in units of 50MHz */ - dev_priv->rps.cur_freq = 0; - /* static values from HW: RP0 < RPe < RP1 < RPn (min_freq) */ - dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff; - dev_priv->rps.rp0_freq = (rp_state_cap >> 0) & 0xff; - dev_priv->rps.min_freq = (rp_state_cap >> 16) & 0xff; - /* XXX: only BYT has a special efficient freq */ - dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq; - /* hw_max = RP0 until we check for overclocking */ - dev_priv->rps.max_freq = dev_priv->rps.rp0_freq; - - /* Preserve min/max settings in case of re-init */ - if (dev_priv->rps.max_freq_softlimit == 0) - dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq; - - if (dev_priv->rps.min_freq_softlimit == 0) - dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq; + parse_rp_state_cap(dev_priv, rp_state_cap); /* disable the counters and set deterministic thresholds */ I915_WRITE(GEN6_RC_CONTROL, 0);
We have a need for duplicated parsing of the RP_STATE_CAPS register (and the setting of the associated fields). To reuse some code, we can extract the function into a simple helper. This patch also addresses the fact that we missed doing this for gen8, something we should have done anyway. This could be two patches, one to extract, and one to add gen8, but it's trivial enough that I think one is fine. I will accept a request to split it. Please notice the fix addressed by v2 below. Valleyview is left untouched because it is different. v2: Logically rebased on top of commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac Author: Jeff McGee <jeff.mcgee@intel.com> Date: Tue Feb 4 11:32:31 2014 -0600 drm/i915: Restore rps/rc6 on reset Note with the above change the fix for gen8 is also handled (which was not the case in Jeff's original patch). Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)