| Message ID | 1455232928-2806-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[I'm cheating and doing this code review with the author watching over my shoulder] On 11/02/16 15:22, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@intel.com> > > 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. Nit: the main point shouldn't come second. > 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. should be -> is > When eDP v1.4 panel that require the 8640 vco are available this may need > to be investigated. Just say that 8640 can't be tested yet. > 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@intel.com> > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com> > --- > 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); - skl_cdclk_get_vco() and skl_cdclk_frequencies[] should probably be renamed to: + skl_get_bios_cdclk_vco() and skl_bios_cdclk_frequencies[] to avoid confusion with the (different) mapping used in the new skl_modeset_calc_cdclk() function below. > 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 */ This comment needs to be updated. Maybe something like: "initialize to the lowest/most economical freq for now; will modeset very soon anyway". > - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); > + skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 ); > > /* 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); const ? > + int cdclk; > + > + /* > + * FIXME should also account for plane ratio > + * once 64bpp pixel formats are supported. > + */ > + > + 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. > + */ This copied typos will help future grep and search/replace. > + 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; > + } > +} > + > 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); If you change this to: if ( ( ret = dev_priv->display.modeset_calc_cdclk(state) ) < 0 ) return ret; ... then you can simplify the boolean expressions below > + 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))) { Extra parens > + if (((target_vco) && (dev_priv->skl_vco_freq != target_vco)) || > + (!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); Suggestion (assuming ret change above): const bool cdclk_change = intel_state->dev_cdclk != dev_priv->cdclk_freq; const bool vco_change = ( IS_SKYLAKE(dev) || IS_KABYLAKE(dev) ) && (target_vco) && (dev_priv->skl_vco_freq != target_vco)) ; if ( cdclk_change || vco_change ) 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) > { > 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); Clint says: I will remove the dev_priv param. > 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, >
On Thu, Feb 11, 2016 at 03:22:08PM -0800, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@intel.com> > > 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@intel.com> > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com> > --- > 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. > +} > + > 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(). 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). > + (!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. > { > 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
On Thu, Feb 11, 2016 at 05:11:52PM -0800, Marc Herbert wrote: > [I'm cheating and doing this code review with the author watching over my shoulder] > > On 11/02/16 15:22, clinton.a.taylor@intel.com wrote: > > From: Clint Taylor <clinton.a.taylor@intel.com> > > > > 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. > > Nit: the main point shouldn't come second. > > > 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. > > should be -> is > > > > When eDP v1.4 panel that require the 8640 vco are available this may need > > to be investigated. > > Just say that 8640 can't be tested yet. > > > 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@intel.com> > > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com> > > --- > > 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); > > - skl_cdclk_get_vco() and skl_cdclk_frequencies[] should probably be renamed to: > + skl_get_bios_cdclk_vco() and skl_bios_cdclk_frequencies[] > > to avoid confusion with the (different) mapping used in the new skl_modeset_calc_cdclk() > function below. Let's not. This stuff doesn't really have anything to do with the BIOS. We just want to read out the current hardware state, nothing more.
On 02/12/2016 03:18 AM, Ville Syrjälä wrote: > On Thu, Feb 11, 2016 at 03:22:08PM -0800, clinton.a.taylor@intel.com wrote: >> From: Clint Taylor <clinton.a.taylor@intel.com> >> >> 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@intel.com> >> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com> >> --- >> 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 >
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); 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 ); /* 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. + */ + + 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; + } +} + 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)) || + (!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) { 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,