On 02/12/2016 03:18 AM, Ville Syrjälä wrote:
On Thu, Feb 11, 2016 at 03:22:08PM -0800, clinton.a.taylor@xxxxxxxxx wrote:From: Clint Taylor <clinton.a.taylor@xxxxxxxxx> Track VCO frequency of SKL instead of the boot CDCLK and allow modeset to set cdclk based on the max required pixel clock based on VCO selected. The vco should be tracked at the atomic level and all CRTCs updated if the required vco is changed. At this time the eDP pll is configured inside the encoder which has no visibility into the atomic state. When eDP v1.4 panel that require the 8640 vco are available this may need to be investigated. V1: initial version V2: add vco tracking in intel_dp_compute_config(), rename skl_boot_cdclk. V3: rebase, V2 feedback not possible as encoders are not aware of atomic. V4: track target vco is atomic state. modeset all CRTCs if vco changes Signed-off-by: Clint Taylor <clinton.a.taylor@xxxxxxxxx> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@xxxxxxxxxxxxxxx> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_ddi.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 97 +++++++++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_dp.c | 10 ++-- drivers/gpu/drm/i915/intel_drv.h | 4 ++ 5 files changed, 97 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..f65dd1a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,7 +1822,7 @@ struct drm_i915_private { int num_fence_regs; /* 8 on pre-965, 16 otherwise */ unsigned int fsb_freq, mem_freq, is_ddr3; - unsigned int skl_boot_cdclk; + unsigned int skl_vco_freq; unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; unsigned int max_dotclk_freq; unsigned int hpll_freq; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6d5b09f..285adab 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; cdclk_freq = dev_priv->display.get_display_clock_speed(dev); - dev_priv->skl_boot_cdclk = cdclk_freq; + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);This should really read out the vco from the hardware. But I think we can leave that for later. The problem really is the 540MHz case since it can be using either vco frequency (IIRC).if (skl_sanitize_cdclk(dev_priv)) DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9e2273b..ef4ac34 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) return (freq - 1000) / 500; } -static unsigned int skl_cdclk_get_vco(unsigned int freq) +unsigned int skl_cdclk_get_vco(unsigned int freq) { unsigned int i; @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) void skl_init_cdclk(struct drm_i915_private *dev_priv) { - unsigned int required_vco; - /* DPLL0 not enabled (happens on early BIOS versions) */ if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { /* enable DPLL0 */ - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); - skl_dpll0_enable(dev_priv, required_vco); + if (dev_priv->skl_vco_freq != 8640) { + dev_priv->skl_vco_freq = 8100; + } + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); } /* set CDCLK to the frequency the BIOS chose */ - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); + skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 );We really shouldn't change the cdclk if there are active outputs. This whole area definitely needs more work, for BXT too I already have some BXT patches lined up in some branch that frob around these parts a bit, so maybe I should extend that stuff to SKL as well. In the meantime we don't want to cause a regression at least, so maybe we can just do something like: if (!LCPLL_ENABLE) { ... cdclk = vco == 8100 ? ...; } else { cdclk = dev_priv->cdclk_freq; } set_cdclk(cdclk);/* enable DBUF power */ I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); @@ -5847,7 +5847,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) { uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); - int freq = dev_priv->skl_boot_cdclk; + int freq = dev_priv->cdclk_freq; /* * check if the pre-os intialized the display @@ -5871,11 +5871,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) /* All well; nothing to sanitize */ return false; sanitize: - /* - * As of now initialize with max cdclk till - * we get dynamic cdclk support - * */ - dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq; + skl_init_cdclk(dev_priv); /* we did have to sanitize */ @@ -9845,6 +9841,68 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) broadwell_set_cdclk(dev, req_cdclk); } +static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->dev); + int max_pixclk = ilk_max_pixel_rate(state); + int cdclk; + + /* + * FIXME should also account for plane ratio + * once 64bpp pixel formats are supported. + */A bit of formatting issue with the comment. The stars should be lining up :)+ + if (to_intel_atomic_state(state)->vco_target == 8640) { + /* vco 8640 */ + if (max_pixclk > 540000) + cdclk = 617140; + else if (max_pixclk > 432000) + cdclk = 540000; + else if (max_pixclk > 308570) + cdclk = 432000; + else + cdclk = 308570; + } + else { + /* VCO 8100 */ + if (max_pixclk > 540000) + cdclk = 675000; + else if (max_pixclk > 450000) + cdclk = 540000; + else if (max_pixclk > 337500) + cdclk = 450000; + else + cdclk = 337500; + } + + /* + * FIXME move the cdclk caclulation to + * compute_config() so we can fail gracegully. + */ + if (cdclk > dev_priv->max_cdclk_freq) { + DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n", + cdclk, dev_priv->max_cdclk_freq); + cdclk = dev_priv->max_cdclk_freq; + } + + to_intel_atomic_state(state)->cdclk = cdclk; + + return 0; +} + +static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state) +{ + struct drm_device *dev = old_state->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; + + skl_set_cdclk(dev_priv, req_cdclk); + + if (to_intel_atomic_state(old_state)->vco_target) { + dev_priv->skl_vco_freq = to_intel_atomic_state(old_state)->vco_target; + }skl_set_cdclk() really needs to be taught to do the 'disable+enable pll' dance so that it can actually change the vco frequency. But I think we can leave that for a followup so that we can move this forward. A FIXME comment here would be good though so we don't forget.
Ok
+} + static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { @@ -13219,11 +13277,13 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) static int intel_modeset_checks(struct drm_atomic_state *state) { + struct drm_device *dev = state->dev; struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_i915_private *dev_priv = state->dev->dev_private; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; int ret = 0, i; + unsigned int target_vco; if (!check_digital_port_conflicts(state)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); @@ -13249,8 +13309,14 @@ static int intel_modeset_checks(struct drm_atomic_state *state) */ if (dev_priv->display.modeset_calc_cdclk) { ret = dev_priv->display.modeset_calc_cdclk(state); + target_vco = to_intel_atomic_state(state)->vco_target; - if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq) + if (IS_SKYLAKE(dev) || (IS_KABYLAKE(dev))) { + if (((target_vco) && (dev_priv->skl_vco_freq != target_vco)) ||Hmm. If target_vco is 0, then skl_modeset_calc_cdclk() has already done something wrong. So I think if the target_vco wasn't set by the encoder(s) you'll need to just set '->vco_target = dev_priv->vco' before calling .modeset_calc_cdclk().
modeset_calc_cdclk defaults to the 8100 VCO. This is fine for all eDP link rates except the new V1.4 rates. If target_vco is 0 copying in dev_priv = vco is the correct thing to do. Thanks!
BTW the vco_target vs. target_vco is bothering me a bit. In fact I think it's pretty clear that it's a target value simply due to the fact that it's part of the atomic state. So we can probably just call it something like cdclk_pll_vco (keeping the name somewhat generic in case we need the same treatment for a future platform, and we all know the hardware folks like to rename things for fun and profit).
agreed.
+ (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)) { + ret = intel_modeset_all_pipes(state); + } + } else if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq) ret = intel_modeset_all_pipes(state); if (ret < 0) @@ -15002,6 +15068,11 @@ static void intel_init_display(struct drm_device *dev) broxton_modeset_commit_cdclk; dev_priv->display.modeset_calc_cdclk = broxton_modeset_calc_cdclk; + } else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) { + dev_priv->display.modeset_commit_cdclk = + skl_modeset_commit_cdclk; + dev_priv->display.modeset_calc_cdclk = + skl_modeset_calc_cdclk; } switch (INTEL_INFO(dev)->gen) { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a073f04..fbc37fe 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1242,9 +1242,10 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector) } static void -skl_edp_set_pll_config(struct intel_crtc_state *pipe_config) +skl_edp_set_pll_config(struct drm_i915_private *dev_priv, struct intel_crtc_state *pipe_config)dev_priv could be dug out via the pipe_config too, but this way is fine too IMO.
dev_priv is not used anymore and I can remove this parameter.
{ u32 ctrl1; + u32 vco = 8100; memset(&pipe_config->dpll_hw_state, 0, sizeof(pipe_config->dpll_hw_state)); @@ -1277,13 +1278,16 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config) case 108000: ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080, SKL_DPLL0); + vco = 8640; break; case 216000: ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160, SKL_DPLL0); + vco = 8640; break; - } + + to_intel_atomic_state(pipe_config->base.state)->vco_target = vco; pipe_config->dpll_hw_state.ctrl1 = ctrl1; } @@ -1664,7 +1668,7 @@ found: } if ((IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) && is_edp(intel_dp)) - skl_edp_set_pll_config(pipe_config); + skl_edp_set_pll_config(dev_priv, pipe_config); else if (IS_BROXTON(dev)) /* handled in ddi */; else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 878172a..005e036 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -260,6 +260,9 @@ struct intel_atomic_state { struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS]; struct intel_wm_config wm_config; + + /* SKL/KBL Only */ + unsigned int vco_target; }; struct intel_plane_state { @@ -1191,6 +1194,7 @@ void bxt_disable_dc9(struct drm_i915_private *dev_priv); void skl_init_cdclk(struct drm_i915_private *dev_priv); int skl_sanitize_cdclk(struct drm_i915_private *dev_priv); void skl_uninit_cdclk(struct drm_i915_private *dev_priv); +unsigned int skl_cdclk_get_vco(unsigned int freq); void skl_enable_dc6(struct drm_i915_private *dev_priv); void skl_disable_dc6(struct drm_i915_private *dev_priv); void intel_dp_get_m_n(struct intel_crtc *crtc, -- 1.7.9.5
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
