From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915/vlv: modeset_global_* for VLV v4 Date: Fri, 1 Nov 2013 21:52:44 +0200 Message-ID: <20131101195244.GG13047@intel.com> References: <1383333216-1428-1-git-send-email-jbarnes@virtuousgeek.org> <1383334096-890-1-git-send-email-jbarnes@virtuousgeek.org> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 68369EE613 for ; Fri, 1 Nov 2013 12:52:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1383334096-890-1-git-send-email-jbarnes@virtuousgeek.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > --- > 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. > /* > * 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 *c= rtc) > 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 =3D 800; > + break; > + case 1066: > + vco =3D 1600; > + break; > + case 1333: > + vco =3D 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. > + > +/* 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 =3D dev->dev_private; > + u32 val, cmd; > + > + if (cdclk >=3D 320) /* jump to highest voltage for 400MHz too */ > + cmd =3D 2; > + else if (cdclk =3D=3D 266) > + cmd =3D 1; > + else > + cmd =3D 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + val =3D vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); > + val &=3D ~DSPFREQGUAR_MASK; > + val |=3D (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) =3D=3D (cmd << DSPFREQSTAT_SHIFT), > + 50)) { > + DRM_ERROR("timed out waiting for CDclk change\n"); > + } > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (cdclk =3D=3D 400) { > + u32 divider, vco; > + > + vco =3D valleyview_get_vco(dev_priv); > + divider =3D (vco * 10) / cdclk; > + divider =3D ((divider * 2) / 10) - 1; Why the *10/10? Just this should do: divider =3D (vco << 1) / cdclk - 1 > + > + mutex_lock(&dev_priv->dpio_lock); > + /* adjust cdclk divider */ > + val =3D vlv_cck_read(dev_priv, 0x6b); > + val &=3D ~0xf; > + val |=3D 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 =3D vlv_bunit_read(dev_priv, 0x11); > + val &=3D ~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 =3D=3D 400) > + val |=3D 0x12; > + else > + val |=3D 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 =3D valleyview_get_vco(dev_priv); > + > + mutex_lock(&dev_priv->dpio_lock); > + divider =3D vlv_cck_read(dev_priv, 0x6b); > + mutex_unlock(&dev_priv->dpio_lock); > + > + divider &=3D 0xf; > + divider =3D ((divider + 1) * 10) / 2; > + > + cur_cdclk =3D (vco * 10) / divider; Again *10/10 seems useless. Just 'cur_cdclk =3D (vco << 1) / (divider + 1)' But again we have a bit of code duplication w/ intel_i2c.c. > + > + return cur_cdclk; > +} > + > +static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > + int max_pixclk) > +{ > + int cur_cdclk; > + > + cur_cdclk =3D 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 <=3D 288000 && max_pixclk > 240300) { Assuming the 267 mhz is in fact 266.666... that would be just 240000. Also the <=3D check is useless. Just > is enough. > + 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. > +} > + > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct intel_crtc *intel_crtc; > + int max_pixclk =3D 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 =3D intel_crtc->config.adjusted_mode.clock; Should be .crtc_clock actually. > + } > + > + return max_pixclk; > +} > + > +static void valleyview_modeset_global_pipes(struct drm_device *dev, > + unsigned *prepare_pipes) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct intel_crtc *intel_crtc; > + int max_pixclk =3D intel_mode_max_pixclk(dev_priv); > + int cur_cdclk =3D valleyview_cur_cdclk(dev_priv); > + > + if (valleyview_calc_cdclk(dev_priv, max_pixclk) =3D=3D 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. > + *prepare_pipes |=3D (1 << intel_crtc->pipe); > +} > + > +static void valleyview_modeset_global_resources(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + int max_pixclk =3D intel_mode_max_pixclk(dev_priv); > + int cur_cdclk =3D valleyview_cur_cdclk(dev_priv); > + int req_cdclk =3D valleyview_calc_cdclk(dev_priv, max_pixclk); > + > + if (req_cdclk !=3D cur_cdclk) > + valleyview_set_cdclk(dev, req_cdclk); > +} > + > static void valleyview_crtc_enable(struct drm_crtc *crtc) > { > struct drm_device *dev =3D crtc->dev; > @@ -10336,6 +10507,11 @@ static void intel_init_display(struct drm_device= *dev) > } > } else if (IS_G4X(dev)) { > dev_priv->display.write_eld =3D g4x_write_eld; > + } else if (IS_VALLEYVIEW(dev)) { > + dev_priv->display.modeset_global_resources =3D > + valleyview_modeset_global_resources; > + dev_priv->display.modeset_global_pipes =3D > + valleyview_modeset_global_pipes; > } > = > /* Default just returns -ENODEV to indicate unsupported */ > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/inte= l_i2c.c > index 2ca17b1..1263409 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -87,10 +87,6 @@ static void gmbus_set_freq(struct drm_i915_private *de= v_priv) > = > BUG_ON(!IS_VALLEYVIEW(dev_priv->dev)); > = > - /* Skip setting the gmbus freq if BIOS has already programmed it */ > - if (I915_READ(GMBUSFREQ_VLV) !=3D 0xA0) > - return; > - > /* Obtain SKU information */ > mutex_lock(&dev_priv->dpio_lock); > hpll_freq =3D > -- = > 1.8.3.1 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC