| Message ID | 20171019174329.30912-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 19, 2017 at 05:43:29PM +0000, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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@intel.com> > Cc: Manasi Navare <manasi.d.navare@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > 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@intel.com> > > /* 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 >
On Thu, Oct 19, 2017 at 04:42:52PM -0700, Rodrigo Vivi wrote: > On Thu, Oct 19, 2017 at 05:43:29PM +0000, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > 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@intel.com> > > Cc: Manasi Navare <manasi.d.navare@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > 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. Yeah, the iosf sideband mailbox is much nicer because you have actual read vs. write opcodes in addition to the port+register offset information which just specify what you want to access. > > > } > > > > 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. I was going back and forth between removing this from the chv_set_cdclk(), and in the end I decided to keep it. And looks like I then accidentally added it into vlv_set_cdclk() as well thinking both had it originally. I guess I'm still leaning towards keeping it, so I'll go ahead and split this out to a separate patch. > > But anyways, if you decide to split or not > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > /* 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 > >
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; } 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; + } /* 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;