| Message ID | 1423234598-14781-5-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > RP Interrupt Up/Down Frequency Limits register (A014) definition > has changed for SKL. Updated the gen6_rps_limits function as per that > > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 215b200..db24b48 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) > * ourselves, instead of doing a rmw cycle (which might result in us clearing > * all limits and the gpu stuck at whatever frequency it is at atm). > */ > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) Spurious name change, it doesn't seem to add anything or clear up any confusion with vlv. -Chris
On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote: > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > > From: Akash Goel <akash.goel@intel.com> > > > > RP Interrupt Up/Down Frequency Limits register (A014) definition > > has changed for SKL. Updated the gen6_rps_limits function as per that > > > > Signed-off-by: Akash Goel <akash.goel@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 215b200..db24b48 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) > > * ourselves, instead of doing a rmw cycle (which might result in us clearing > > * all limits and the gpu stuck at whatever frequency it is at atm). > > */ > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > Spurious name change, it doesn't seem to add anything or clear up any > confusion with vlv. Fine will keep the original name, thought would be better to give a generic name to the function and abstract the platform specific differences inside its definition. > -Chris >
On Mon, Feb 09, 2015 at 10:26:33AM +0530, Akash Goel wrote: > On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote: > > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > > > From: Akash Goel <akash.goel@intel.com> > > > > > > RP Interrupt Up/Down Frequency Limits register (A014) definition > > > has changed for SKL. Updated the gen6_rps_limits function as per that > > > > > > Signed-off-by: Akash Goel <akash.goel@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 215b200..db24b48 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) > > > * ourselves, instead of doing a rmw cycle (which might result in us clearing > > > * all limits and the gpu stuck at whatever frequency it is at atm). > > > */ > > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > > > Spurious name change, it doesn't seem to add anything or clear up any > > confusion with vlv. > Fine will keep the original name, thought would be better to give a > generic name to the function and abstract the platform specific > differences inside its definition. Generic would be intel_rps_limits(). I am wary of using get(), the common idiom is for a getter to return ownership as well, e.g. kref_get(), intel_uncore_forcewake_get(). Also outside of trivial getters and setters, get is such a generic verb that I don't think it adds much self-documentating value, especially when breaking established patterns. -Chris
On Mon, 2015-02-09 at 11:03 +0000, Chris Wilson wrote: > On Mon, Feb 09, 2015 at 10:26:33AM +0530, Akash Goel wrote: > > On Fri, 2015-02-06 at 15:43 +0000, Chris Wilson wrote: > > > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > > > > From: Akash Goel <akash.goel@intel.com> > > > > > > > > RP Interrupt Up/Down Frequency Limits register (A014) definition > > > > has changed for SKL. Updated the gen6_rps_limits function as per that > > > > > > > > Signed-off-by: Akash Goel <akash.goel@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index 215b200..db24b48 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) > > > > * ourselves, instead of doing a rmw cycle (which might result in us clearing > > > > * all limits and the gpu stuck at whatever frequency it is at atm). > > > > */ > > > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > > > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > > > > > Spurious name change, it doesn't seem to add anything or clear up any > > > confusion with vlv. > > Fine will keep the original name, thought would be better to give a > > generic name to the function and abstract the platform specific > > differences inside its definition. > > Generic would be intel_rps_limits(). I am wary of using get(), the > common idiom is for a getter to return ownership as well, e.g. > kref_get(), intel_uncore_forcewake_get(). Also outside of trivial getters > and setters, get is such a generic verb that I don't think it adds much > self-documentating value, especially when breaking established patterns. Understood, using get() is a misnomer & would be inconsistent with the existing naming patterns. Will rename it to 'intel_rps_limits'. Thanks for the clarification. > -Chris >
On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > RP Interrupt Up/Down Frequency Limits register (A014) definition > has changed for SKL. Updated the gen6_rps_limits function as per that > > Signed-off-by: Akash Goel <akash.goel@intel.com> Ah, this is the change I was looking for earlier. Comment below though: > --- > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 215b200..db24b48 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) > * ourselves, instead of doing a rmw cycle (which might result in us clearing > * all limits and the gpu stuck at whatever frequency it is at atm). > */ > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) > { > u32 limits; > > @@ -3633,9 +3633,15 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > * the hw runs at the minimal clock before selecting the desired > * frequency, if the down threshold expires in that window we will not > * receive a down interrupt. */ > - limits = dev_priv->rps.max_freq_softlimit << 24; > - if (val <= dev_priv->rps.min_freq_softlimit) > - limits |= dev_priv->rps.min_freq_softlimit << 16; > + if (IS_GEN9(dev_priv->dev)) { > + limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23; > + if (val <= dev_priv->rps.min_freq_softlimit) > + limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14; I believe the values here are in 16.666 Mhz, the power spec I have gives examples: [31:23]=54d: No interrupt if already >=900MHz [22:14]=18d: No interrupt if already <= 300MHz and 54 * 16.66.. = 900. > + } else { > + limits = dev_priv->rps.max_freq_softlimit << 24; > + if (val <= dev_priv->rps.min_freq_softlimit) > + limits |= dev_priv->rps.min_freq_softlimit << 16; > + } > > return limits; > } > @@ -3778,7 +3784,7 @@ static void gen6_set_rps(struct drm_device *dev, u8 val) > /* Make sure we continue to get interrupts > * until we hit the minimum or maximum frequencies. > */ > - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, gen6_rps_limits(dev_priv, val)); > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, get_rps_limits(dev_priv, val)); > I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val)); > > POSTING_READ(GEN6_RPNSWREQ); > -- > 1.9.2 >
On Tue, Feb 17, 2015 at 02:44:56PM +0000, Damien Lespiau wrote: > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > > + if (IS_GEN9(dev_priv->dev)) { > > + limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23; > > + if (val <= dev_priv->rps.min_freq_softlimit) > > + limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14; > > I believe the values here are in 16.666 Mhz, the power spec I have gives > examples: > [31:23]=54d: No interrupt if already >=900MHz > [22:14]=18d: No interrupt if already <= 300MHz > > and 54 * 16.66.. = 900. Right, so RP_STATE_CAP is documented using 50Mhz units, so the above is correct. How about unifying all values for SKL to the same units (always 16.66 Mhz units)? that would remove some of the confusion a bit.
On Tue, Feb 17, 2015 at 03:01:39PM +0000, Damien Lespiau wrote: > On Tue, Feb 17, 2015 at 02:44:56PM +0000, Damien Lespiau wrote: > > On Fri, Feb 06, 2015 at 08:26:35PM +0530, akash.goel@intel.com wrote: > > > + if (IS_GEN9(dev_priv->dev)) { > > > + limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23; > > > + if (val <= dev_priv->rps.min_freq_softlimit) > > > + limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14; > > > > I believe the values here are in 16.666 Mhz, the power spec I have gives > > examples: > > [31:23]=54d: No interrupt if already >=900MHz > > [22:14]=18d: No interrupt if already <= 300MHz > > > > and 54 * 16.66.. = 900. > > Right, so RP_STATE_CAP is documented using 50Mhz units, so the above is > correct. How about unifying all values for SKL to the same units (always > 16.66 Mhz units)? that would remove some of the confusion a bit. Right, so going to back here, that looks correct: Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 215b200..db24b48 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3623,7 +3623,7 @@ static void ironlake_disable_drps(struct drm_device *dev) * ourselves, instead of doing a rmw cycle (which might result in us clearing * all limits and the gpu stuck at whatever frequency it is at atm). */ -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) +static u32 get_rps_limits(struct drm_i915_private *dev_priv, u8 val) { u32 limits; @@ -3633,9 +3633,15 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) * the hw runs at the minimal clock before selecting the desired * frequency, if the down threshold expires in that window we will not * receive a down interrupt. */ - limits = dev_priv->rps.max_freq_softlimit << 24; - if (val <= dev_priv->rps.min_freq_softlimit) - limits |= dev_priv->rps.min_freq_softlimit << 16; + if (IS_GEN9(dev_priv->dev)) { + limits = (dev_priv->rps.max_freq_softlimit * GEN9_FREQ_SCALER) << 23; + if (val <= dev_priv->rps.min_freq_softlimit) + limits |= (dev_priv->rps.min_freq_softlimit * GEN9_FREQ_SCALER) << 14; + } else { + limits = dev_priv->rps.max_freq_softlimit << 24; + if (val <= dev_priv->rps.min_freq_softlimit) + limits |= dev_priv->rps.min_freq_softlimit << 16; + } return limits; } @@ -3778,7 +3784,7 @@ static void gen6_set_rps(struct drm_device *dev, u8 val) /* Make sure we continue to get interrupts * until we hit the minimum or maximum frequencies. */ - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, gen6_rps_limits(dev_priv, val)); + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, get_rps_limits(dev_priv, val)); I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val)); POSTING_READ(GEN6_RPNSWREQ);