[Intel-gfx] [PATCH V11] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
Clint Taylor
clinton.a.taylor at intel.com
Thu Mar 17 21:38:15 UTC 2016
On 03/17/2016 02:18 PM, Rodrigo Vivi wrote:
>
>
> On Wed, Mar 16, 2016 at 4:33 PM Clint Taylor <clinton.a.taylor at intel.com
> <mailto:clinton.a.taylor at intel.com>> wrote:
>
> On 03/16/2016 12:27 AM, Daniel Vetter wrote:
> > On Tue, Mar 15, 2016 at 02:34:05PM -0700,
> clinton.a.taylor at intel.com <mailto:clinton.a.taylor at intel.com> wrote:
> >> From: Clint Taylor <clinton.a.taylor at intel.com
> <mailto:clinton.a.taylor at intel.com>>
> >>
> >> WARNING: Using ChromeOS with an eDP panel and a 4K at 60 DP monitor
> connected
> >> to DDI1 the system will hard hang during a cold boot. Occurs
> when DDI1
> >> is enabled when the cdclk is less then required. DP connected to
> DDI2
> >> and HPD on either port works correctly.
> >
> > So this patch hard-hangs machines?
>
> The hard-hang only occurs in developer mode ChromeOS with Coreboot (FSP
> version < 1.9) using DP on DDI1 using a USB-C->DP converter.
>
> We have not been able to reproduce this issue since updating to FSP 1.9
> and was never able to replicate the issue with UEFI and SKL RVP. The
> warning was added to the commit message during development to make
> everyone aware of the issue.
>
>
> What does this developer mode has special/different?
Developer mode initializes eDP and display a boot message, the boot
message stays enabled until the kernel takes over. Normal mode does not
initialize the display hardware.
> What are the risks of getting this in other platform out there non chrome?
FSP 1.8 and earlier are only used in coreboot for ChromeOS in
non-shipping platforms. Of course there is a risk since cdclk is so
important to the display subsystem and we haven't found exactly why
cdclk was not being computed correctly for DDI1.
-Clint
>
>
> >
> >>
> >> Set cdclk based on the max required pixel clock based on VCO
> >> selected. Track boot vco instead of boot cdclk.
> >>
> >> The vco is now tracked at the atomic level and all CRTCs updated if
> >> the required vco is changed. Not tested with eDP v1.4 panels that
> >> require 8640 vco due to availability.
> >>
> >> 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
> >> V5: rename atomic variable, cleaner if/else logic, use existing
> vco if
> >> encoder does not return a new vco value. check_patch.pl
> <http://check_patch.pl> cleanup
> >> V6: simplify logic in intel_modeset_checks.
> >> V7: reorder an IF for readability and whitespace fix.
> >> V8: use dev_cdclk for tracking new cdclk during atomic
> >> V9: correctly handle vco 8640 when crtcs==0
> >> V10: Clean up if else in crtcs==0
> >> V11: Rebase for new intel_dpll_mgr.c
> >>
> >> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com
> <mailto:ville.syrjala at linux.intel.com>>
> >
> > Is the r-b from Ville really for v11?
>
> r-b is from patch V9 and only his V8 comment change and a rebase has
> occured since. I will submit a V12 with the warning and r-b removed if
> necessary.
>
>
> Ville?
>
>
> > -Daniel
> >
> >> Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com
> <mailto:clinton.a.taylor at intel.com>>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> >> drivers/gpu/drm/i915/intel_display.c | 111
> ++++++++++++++++++++++++++++-----
> >> drivers/gpu/drm/i915/intel_dpll_mgr.c | 9 +--
> >> drivers/gpu/drm/i915/intel_drv.h | 5 ++
> >> 4 files changed, 106 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> >> index 80b14f1..bf87e62 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1759,7 +1759,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 rawclk_freq;
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> >> index ce55f0b..fc5268c 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5584,7 +5584,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;
> >>
> >> @@ -5742,17 +5742,21 @@ void skl_uninit_cdclk(struct
> drm_i915_private *dev_priv)
> >>
> >> void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >> {
> >> - unsigned int required_vco;
> >> + unsigned int cdclk;
> >>
> >> /* 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);
> >> + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500
> : 308570);
> >> + } else {
> >> + cdclk = dev_priv->cdclk_freq;
> >> }
> >>
> >> - /* set CDCLK to the frequency the BIOS chose */
> >> - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> >> + /* set CDCLK to the lowest frequency, Modeset follows */
> >> + skl_set_cdclk(dev_priv, cdclk);
> >>
> >> /* enable DBUF power */
> >> I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> >> @@ -5768,7 +5772,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
> >> @@ -5792,11 +5796,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 */
> >> @@ -9753,6 +9753,73 @@ 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 intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> >> + struct drm_i915_private *dev_priv = to_i915(state->dev);
> >> + const int max_pixclk = ilk_max_pixel_rate(state);
> >> + int cdclk;
> >> +
> >> + /*
> >> + * FIXME should also account for plane ratio
> >> + * once 64bpp pixel formats are supported.
> >> + */
> >> +
> >> + if (intel_state->cdclk_pll_vco == 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;
> >> + }
> >> +
> >> + intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> >> + if (!intel_state->active_crtcs)
> >> + intel_state->dev_cdclk =
> ((intel_state->cdclk_pll_vco == 8640) ?
> >> + 308570 : 337500);
> >> +
> >> +
> >> + 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)->dev_cdclk;
> >> +
> >> + /*
> >> + * FIXME disable/enable PLL should wrap set_cdclk()
> >> + */
> >> + skl_set_cdclk(dev_priv, req_cdclk);
> >> +
> >> + dev_priv->skl_vco_freq =
> to_intel_atomic_state(old_state)->cdclk_pll_vco;
> >> +}
> >> +
> >> static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> >> struct intel_crtc_state
> *crtc_state)
> >> {
> >> @@ -13214,9 +13281,15 @@ static int intel_modeset_checks(struct
> drm_atomic_state *state)
> >> * adjusted_mode bits in the crtc directly.
> >> */
> >> if (dev_priv->display.modeset_calc_cdclk) {
> >> + if (!intel_state->cdclk_pll_vco)
> >> + intel_state->cdclk_pll_vco =
> dev_priv->skl_vco_freq;
> >> +
> >> ret = dev_priv->display.modeset_calc_cdclk(state);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >> - if (!ret && intel_state->dev_cdclk !=
> dev_priv->cdclk_freq)
> >> + if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> >> + intel_state->cdclk_pll_vco !=
> dev_priv->skl_vco_freq)
> >> ret = intel_modeset_all_pipes(state);
> >>
> >> if (ret < 0)
> >> @@ -13588,7 +13661,8 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> >>
> drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> >>
> >> if (dev_priv->display.modeset_commit_cdclk &&
> >> - intel_state->dev_cdclk != dev_priv->cdclk_freq)
> >> + (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> >> + intel_state->cdclk_pll_vco !=
> dev_priv->skl_vco_freq))
> >> dev_priv->display.modeset_commit_cdclk(state);
> >> }
> >>
> >> @@ -14964,8 +15038,12 @@ 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) {
> >> case 2:
> >> dev_priv->display.queue_flip = intel_gen2_queue_flip;
> >> @@ -15672,7 +15750,8 @@ static void
> intel_modeset_readout_hw_state(struct drm_device *dev)
> >> if (crtc_state->base.active) {
> >> dev_priv->active_crtcs |= 1 << crtc->pipe;
> >>
> >> - if (IS_BROADWELL(dev_priv)) {
> >> + if (IS_BROADWELL(dev_priv) ||
> IS_SKYLAKE(dev_priv) ||
> >> + IS_KABYLAKE(dev_priv)) {
> >> pixclk =
> ilk_pipe_pixel_rate(crtc_state);
> >>
> >> /* pixel rate mustn't exceed 95% of
> cdclk with IPS on BDW */
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index 4b636c4..a5642b1 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -1183,6 +1183,7 @@ skl_get_dpll(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state,
> >> struct intel_shared_dpll *pll;
> >> uint32_t ctrl1, cfgcr1, cfgcr2;
> >> int clock = crtc_state->port_clock;
> >> + uint32_t vco = 8100;
> >>
> >> /*
> >> * See comment in intel_dpll_hw_state to understand why we
> always use 0
> >> @@ -1225,17 +1226,17 @@ skl_get_dpll(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state,
> >> case 162000:
> >> ctrl1 |=
> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1620, 0);
> >> break;
> >> - /* TBD: For DP link rates 2.16 GHz and 4.32 GHz,
> VCO is 8640 which
> >> - results in CDCLK change. Need to handle the change
> of CDCLK by
> >> - disabling pipes and re-enabling them */
> >> case 108000:
> >> ctrl1 |=
> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080, 0);
> >> + vco = 8640;
> >> break;
> >> case 216000:
> >> ctrl1 |=
> DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160, 0);
> >> + vco = 8640;
> >> break;
> >> }
> >>
> >> +
> to_intel_atomic_state(crtc_state->base.state)->cdclk_pll_vco = vco;
> >> cfgcr1 = cfgcr2 = 0;
> >> } else {
> >> return NULL;
> >> @@ -1628,7 +1629,7 @@ static 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_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 02b3d22..b23b129 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -301,6 +301,10 @@ struct intel_atomic_state {
> >> * don't bother calculating intermediate watermarks.
> >> */
> >> bool skip_intermediate_wm;
> >> +
> >> + /* SKL/KBL Only */
> >> + unsigned int cdclk_pll_vco;
> >> +
> >> };
> >>
> >> struct intel_plane_state {
> >> @@ -1239,6 +1243,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 at lists.freedesktop.org
> <mailto:Intel-gfx at lists.freedesktop.org>
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org <mailto:Intel-gfx at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list