On to, 2015-06-25 at 14:54 -0700, Bob Paauwe wrote:
> Broxton is using a different register and different bit ordering
> for rps status capabilities.
>
> Also GT perf freqency register is different for Broxton so update
> that.
>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 21 ++++++++++++++++-----
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++++----
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f3b8062..8a2a456a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1132,9 +1132,9 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> (rgvstat & MEMSTAT_PSTATE_MASK) >> MEMSTAT_PSTATE_SHIFT);
> } else if (IS_GEN6(dev) || (IS_GEN7(dev) && !IS_VALLEYVIEW(dev)) ||
> IS_BROADWELL(dev) || IS_GEN9(dev)) {
> - u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> - u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
> - u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + u32 rp_state_limits;
> + u32 gt_perf_status;
> + u32 rp_state_cap;
> u32 rpmodectl, rpinclimit, rpdeclimit;
> u32 rpstat, cagf, reqf;
> u32 rpupei, rpcurup, rpprevup;
> @@ -1142,6 +1142,15 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> int max_freq;
>
> + rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
> + if (IS_BROXTON(dev)) {
> + rp_state_cap = I915_READ(BXT_RP_STATE_CAP);
> + gt_perf_status = I915_READ(BXT_GT_PERF_STATUS);
> + } else {
> + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> + }
> +
> /* RPSTAT1 is in the GT power well */
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> if (ret)
> @@ -1229,7 +1238,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> seq_printf(m, "Down threshold: %d%%\n",
> dev_priv->rps.down_threshold);
>
> - max_freq = (rp_state_cap & 0xff0000) >> 16;
> + max_freq = (IS_BROXTON(dev) ? rp_state_cap >> 0 :
> + rp_state_cap >> 16) & 0xff;
> max_freq *= (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
> seq_printf(m, "Lowest (RPN) frequency: %dMHz\n",
> intel_gpu_freq(dev_priv, max_freq));
> @@ -1239,7 +1249,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
> seq_printf(m, "Nominal (RP1) frequency: %dMHz\n",
> intel_gpu_freq(dev_priv, max_freq));
>
> - max_freq = rp_state_cap & 0xff;
> + max_freq = (IS_BROXTON(dev) ? rp_state_cap >> 16 :
> + rp_state_cap >> 0) & 0xff;
> max_freq *= (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
> seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n",
> intel_gpu_freq(dev_priv, max_freq));
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fa9ccb87..17b5ca2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2734,8 +2734,10 @@ enum skl_disp_power_wells {
> #define GEN6_GT_THREAD_STATUS_CORE_MASK 0x7
>
> #define GEN6_GT_PERF_STATUS (MCHBAR_MIRROR_BASE_SNB + 0x5948)
> +#define BXT_GT_PERF_STATUS (MCHBAR_MIRROR_BASE_SNB + 0x7070)
Nitpick: the format in GT_PERF_STATUS is the same on SKL and BXT so you
could have redefined GEN6_GT_PERF_STATUS to be a wrapper macro similar
to what you did in v1 of this patch.
> #define GEN6_RP_STATE_LIMITS (MCHBAR_MIRROR_BASE_SNB + 0x5994)
> #define GEN6_RP_STATE_CAP (MCHBAR_MIRROR_BASE_SNB + 0x5998)
> +#define BXT_RP_STATE_CAP 0x138170
>
> #define INTERVAL_1_28_US(us) (((us) * 100) >> 7)
> #define INTERVAL_1_33_US(us) (((us) * 3) >> 2)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 32ff034..213da42 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4288,13 +4288,21 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> u32 ddcc_status = 0;
> int ret;
>
> - rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> /* All of these values are in units of 50MHz */
On SKL/BXT the unit is 16.66MHz, so the above comment could be
clarified.
> dev_priv->rps.cur_freq = 0;
> /* static values from HW: RP0 > RP1 > RPn (min_freq) */
> - dev_priv->rps.rp0_freq = (rp_state_cap >> 0) & 0xff;
> - dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff;
> - dev_priv->rps.min_freq = (rp_state_cap >> 16) & 0xff;
> + if (IS_BROXTON(dev)) {
> + rp_state_cap = I915_READ(BXT_RP_STATE_CAP);
> + dev_priv->rps.rp0_freq = (rp_state_cap >> 16) & 0xff;
> + dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff;
> + dev_priv->rps.min_freq = (rp_state_cap >> 0) & 0xff;
> + } else {
> + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + dev_priv->rps.rp0_freq = (rp_state_cap >> 0) & 0xff;
> + dev_priv->rps.rp1_freq = (rp_state_cap >> 8) & 0xff;
> + dev_priv->rps.min_freq = (rp_state_cap >> 16) & 0xff;
> + }
> +
Nitpick: this could be factored out to a separate function returning
rpn,rp0,rp1 which could be also used then in i915_frequency_info.
All the above can be addressed as a follow-up, so this patch looks ok to
me:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
> if (IS_SKYLAKE(dev)) {
> /* Store the frequency values in 16.66 MHZ units, which is
> the natural hardware unit for SKL */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
