On Fri, 1 Nov 2013 21:52:44 +0200
Ville Syrjälä <ville.syrj...@linux.intel.com> wrote:
> On Fri, Nov 01, 2013 at 12:28:16PM -0700, Jesse Barnes wrote:
> > On VLV/BYT, we can adjust the CDclk frequency up or down based on the
> > max pixel clock we need to drive. Lowering it can save power, while
> > raising it is necessary to support high resolution.
> >
> > Add proper modeset_global_pipes and modeset_global_resources support to
> > perform this adjustment as necessary.
> >
> > v2: use punit interface for 320 and 266 MHz CDclk adjustments (Ville)
> > v3: reset GMBUS dividers too, since we changed CDclk (Ville)
> > v4: jump to highest voltage when going to 400MHz CDclk (Jesse)
> >
> > Signed-off-by: Jesse Barnes <jbar...@virtuousgeek.org>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 7 ++
> > drivers/gpu/drm/i915/intel_display.c | 176
> > +++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_i2c.c | 4 -
> > 3 files changed, 183 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 737d8a3..8a34dcc 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -363,6 +363,11 @@
> > #define PUNIT_OPCODE_REG_READ 6
> > #define PUNIT_OPCODE_REG_WRITE 7
> >
> > +#define PUNIT_REG_DSPFREQ 0x36
> > +#define DSPFREQSTAT_SHIFT 30
> > +#define DSPFREQSTAT_MASK (0x3 << DSPFREQSTAT_SHIFT)
> > +#define DSPFREQGUAR_SHIFT 14
> > +#define DSPFREQGUAR_MASK (0x3 << DSPFREQGUAR_SHIFT)
> > #define PUNIT_REG_PWRGT_CTRL 0x60
> > #define PUNIT_REG_PWRGT_STATUS 0x61
> > #define PUNIT_CLK_GATE 1
> > @@ -1453,6 +1458,8 @@
> > #define CZCLK_FREQ_MASK 0xf
> > #define GMBUSFREQ_VLV (VLV_DISPLAY_BASE + 0x6510)
> >
> > +#define CZCLK_CDCLK_FREQ_RATIO (VLV_DISPLAY_BASE + 0x6508)
> > +
>
> It's already there a few lines above, w/ fancy names for the bits and
> everything.
Oh missed that obviously, will drop and use the existing bits. Should
have known since gmbus uses this...
>
> > /*
> > * Palette regs
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index faa7548..2ff2a29 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3894,6 +3894,177 @@ static void i9xx_pfit_enable(struct intel_crtc
> > *crtc)
> > I915_WRITE(BCLRPAT(crtc->pipe), 0);
> > }
> >
> > +static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > +{
> > + int vco;
> > +
> > + switch (dev_priv->mem_freq) {
> > + default:
> > + case 800:
> > + vco = 800;
> > + break;
> > + case 1066:
> > + vco = 1600;
> > + break;
> > + case 1333:
> > + vco = 2000;
> > + break;
> > + }
> > +
> > + return vco;
> > +}
>
> We have two ways to get this information now. The other is in intel_i2c.c.
> Maybe we should unify a bit.
I'll take a look when I fix up the register define duplication.
>
> > +
> > +/* Adjust CDclk dividers to allow high res or save power if possible */
> > +static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 val, cmd;
> > +
> > + if (cdclk >= 320) /* jump to highest voltage for 400MHz too */
> > + cmd = 2;
> > + else if (cdclk == 266)
> > + cmd = 1;
> > + else
> > + cmd = 0;
> > +
> > + mutex_lock(&dev_priv->rps.hw_lock);
> > + val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> > + val &= ~DSPFREQGUAR_MASK;
> > + val |= (cmd << DSPFREQGUAR_SHIFT);
> > + vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> > + if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> > + DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> > + 50)) {
> > + DRM_ERROR("timed out waiting for CDclk change\n");
> > + }
> > + mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > + if (cdclk == 400) {
> > + u32 divider, vco;
> > +
> > + vco = valleyview_get_vco(dev_priv);
> > + divider = (vco * 10) / cdclk;
> > + divider = ((divider * 2) / 10) - 1;
>
> Why the *10/10?
>
> Just this should do:
> divider = (vco << 1) / cdclk - 1
Yeah *2 would be sufficient here. I just liked the 10 since I used it
elsewhere for fixed point bits.
>
> > +
> > + mutex_lock(&dev_priv->dpio_lock);
> > + /* adjust cdclk divider */
> > + val = vlv_cck_read(dev_priv, 0x6b);
> > + val &= ~0xf;
> > + val |= divider;
> > + vlv_cck_write(dev_priv, 0x6b, val);
> > + mutex_unlock(&dev_priv->dpio_lock);
> > + }
> > +
> > + mutex_lock(&dev_priv->dpio_lock);
> > + /* adjust self-refresh exit latency value */
> > + val = vlv_bunit_read(dev_priv, 0x11);
> > + val &= ~0x7f;
> > +
> > + /*
> > + * For high bandwidth configs, we set a higher latency in the bunit
> > + * so that the core display fetch happens in time to avoid underruns.
> > + */
> > + if (cdclk == 400)
> > + val |= 0x12;
> > + else
> > + val |= 0xc;
> > + vlv_bunit_write(dev_priv, 0x11, val);
> > + mutex_unlock(&dev_priv->dpio_lock);
> > +
> > + /* Since we changed the CDclk, we need to update the GMBUSFREQ too */
> > + intel_i2c_reset(dev);
> > +}
> > +
> > +static int valleyview_cur_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > + int cur_cdclk, vco;
> > + int divider;
> > +
> > + vco = valleyview_get_vco(dev_priv);
> > +
> > + mutex_lock(&dev_priv->dpio_lock);
> > + divider = vlv_cck_read(dev_priv, 0x6b);
> > + mutex_unlock(&dev_priv->dpio_lock);
> > +
> > + divider &= 0xf;
> > + divider = ((divider + 1) * 10) / 2;
> > +
> > + cur_cdclk = (vco * 10) / divider;
>
> Again *10/10 seems useless.
> Just 'cur_cdclk = (vco << 1) / (divider + 1)'
>
> But again we have a bit of code duplication w/ intel_i2c.c.
Yeah and the reg contents should match the CCK read, so these could be
unified.
>
> > +
> > + return cur_cdclk;
> > +}
> > +
> > +static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
> > + int max_pixclk)
> > +{
> > + int cur_cdclk;
> > +
> > + cur_cdclk = valleyview_cur_cdclk(dev_priv);
> > +
> > + /*
> > + * Really only a few cases to deal with, as only 4 CDclks are supported:
> > + * 200MHz
> > + * 267MHz
> > + * 320MHz
> > + * 400MHz
> > + * So we check to see whether we're above 90% of the lower bin and
> > + * adjust if needed.
> > + */
> > + if (max_pixclk > 288000) {
> > + return 400;
> > + } else if (max_pixclk <= 288000 && max_pixclk > 240300) {
>
> Assuming the 267 mhz is in fact 266.666... that would be just 240000.
>
> Also the <= check is useless. Just > is enough.
I just liked documenting the range. But I can drop that.
>
> > + return 320;
> > + } else
> > + return 266;
> > + /* Looks like the 200MHz CDclk freq doesn't work on some configs */
>
> Too bad. But I guess we just need to avoid it for now. Maybe we can get
> it working later.
Yeah if we can do some more testing and figure out the limitations we
might be able to enable it. Should save a bit more power, especially
if we can drop the voltage even further for the 200MHz clock.
>
> > +}
> > +
> > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > +{
> > + struct drm_device *dev = dev_priv->dev;
> > + struct intel_crtc *intel_crtc;
> > + int max_pixclk = 0;
> > +
> > + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> > + base.head) {
> > + if (!intel_crtc->base.enabled)
> > + continue;
> > +
> > + if (max_pixclk < intel_crtc->config.adjusted_mode.clock)
> > + max_pixclk = intel_crtc->config.adjusted_mode.clock;
>
> Should be .crtc_clock actually.
will fix.
>
> > + }
> > +
> > + return max_pixclk;
> > +}
> > +
> > +static void valleyview_modeset_global_pipes(struct drm_device *dev,
> > + unsigned *prepare_pipes)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_crtc *intel_crtc;
> > + int max_pixclk = intel_mode_max_pixclk(dev_priv);
> > + int cur_cdclk = valleyview_cur_cdclk(dev_priv);
> > +
> > + if (valleyview_calc_cdclk(dev_priv, max_pixclk) == cur_cdclk)
> > + return;
> > +
> > + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> > + base.head)
> > + if (intel_crtc->base.enabled)
>
> intel_crtc->active maybe? Although I guess they should be the same when
> this gets called.
Either works, I was looking at the other global_resources and mode
setting code for this.
Thanks for the review, will re-post.
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
