On Thu, Oct 19, 2017 at 05:43:29PM +0000, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Store the punit DSPFREQUAR value into cdclk_state->voltage on
> VLV/CHV. Since we can actually read that out from the hardware
> this can give us a bit more cross checking between the hardware
> and software state.
>
> v2: Don't break waiting for cdclk change on VLV/CHV
>
> Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 66 +++++++++++++++++++++++++++++---------
> 1 file changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 522222f8bb50..9cc4374797eb 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -437,13 +437,45 @@ static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> return 200000;
> }
>
> +static u8 vlv_calc_voltage(struct drm_i915_private *dev_priv, int cdclk)
> +{
> + if (IS_VALLEYVIEW(dev_priv)) {
> + if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
> + return 2;
> + else if (cdclk >= 266667)
> + return 1;
> + else
> + return 0;
> + } else {
> + /*
> + * Specs are full of misinformation, but testing on actual
> + * hardware has shown that we just need to write the desired
> + * CCK divider into the Punit register.
> + */
> + return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> + }
> +}
> +
> static void vlv_get_cdclk(struct drm_i915_private *dev_priv,
> struct intel_cdclk_state *cdclk_state)
> {
> + u32 val;
> +
> cdclk_state->vco = vlv_get_hpll_vco(dev_priv);
> cdclk_state->cdclk = vlv_get_cck_clock(dev_priv, "cdclk",
> CCK_DISPLAY_CLOCK_CONTROL,
> cdclk_state->vco);
> +
> + mutex_lock(&dev_priv->pcu_lock);
> + val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> + mutex_unlock(&dev_priv->pcu_lock);
> +
> + if (IS_VALLEYVIEW(dev_priv))
> + cdclk_state->voltage = (val & DSPFREQGUAR_MASK) >>
> + DSPFREQGUAR_SHIFT;
> + else
> + cdclk_state->voltage = (val & DSPFREQGUAR_MASK_CHV) >>
> + DSPFREQGUAR_SHIFT_CHV;
Oh interresting that vlv is the only one where we can actually read it. :(
I was going to ask pointers to spec, but just by {vlv,chv}_set_cdclk it was
possible to verify this is right.
> }
>
> static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> @@ -486,7 +518,19 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> const struct intel_cdclk_state *cdclk_state)
> {
> int cdclk = cdclk_state->cdclk;
> - u32 val, cmd;
> + u32 val, cmd = cdclk_state->voltage;
> +
> + switch (cdclk) {
> + case 400000:
> + case 333333:
> + case 320000:
> + case 266667:
> + case 200000:
> + break;
> + default:
> + MISSING_CASE(cdclk);
> + return;
> + }
I believe this should be in a separated patch, no?!
Seems something we were already missing anyways.
But anyways, if you decide to split or not
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> /* There are cases where we can end up here with power domains
> * off and a CDCLK frequency other than the minimum, like when
> @@ -496,13 +540,6 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> */
> intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
>
> - if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
> - cmd = 2;
> - else if (cdclk == 266667)
> - cmd = 1;
> - else
> - cmd = 0;
> -
> mutex_lock(&dev_priv->pcu_lock);
> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> val &= ~DSPFREQGUAR_MASK;
> @@ -562,7 +599,7 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> const struct intel_cdclk_state *cdclk_state)
> {
> int cdclk = cdclk_state->cdclk;
> - u32 val, cmd;
> + u32 val, cmd = cdclk_state->voltage;
>
> switch (cdclk) {
> case 333333:
> @@ -583,13 +620,6 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> */
> intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
>
> - /*
> - * Specs are full of misinformation, but testing on actual
> - * hardware has shown that we just need to write the desired
> - * CCK divider into the Punit register.
> - */
> - cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> -
> mutex_lock(&dev_priv->pcu_lock);
> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> val &= ~DSPFREQGUAR_MASK_CHV;
> @@ -1859,11 +1889,15 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
>
> intel_state->cdclk.logical.cdclk = cdclk;
> + intel_state->cdclk.logical.voltage =
> + vlv_calc_voltage(dev_priv, cdclk);
>
> if (!intel_state->active_crtcs) {
> cdclk = vlv_calc_cdclk(dev_priv, 0);
>
> intel_state->cdclk.actual.cdclk = cdclk;
> + intel_state->cdclk.actual.voltage =
> + vlv_calc_voltage(dev_priv, cdclk);
> } else {
> intel_state->cdclk.actual =
> intel_state->cdclk.logical;
> --
> 2.13.6
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
