From: Clint Taylor <clinton.a.taylor@intel.com>
To: "Rodrigo Vivi" <rodrigo.vivi@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH V11] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
Date: Thu, 17 Mar 2016 14:38:15 -0700 [thread overview]
Message-ID: <56EB23C7.4070402@intel.com> (raw)
In-Reply-To: <CABVU7+sWfSF7_kS7VTjDdrUwCmVfVBp6NnvdeDvV94NeD6EfmA@mail.gmail.com>
On 03/17/2016 02:18 PM, Rodrigo Vivi wrote:
>
>
> On Wed, Mar 16, 2016 at 4:33 PM Clint Taylor <clinton.a.taylor@intel.com
> <mailto:clinton.a.taylor@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@intel.com <mailto:clinton.a.taylor@intel.com> wrote:
> >> From: Clint Taylor <clinton.a.taylor@intel.com
> <mailto:clinton.a.taylor@intel.com>>
> >>
> >> WARNING: Using ChromeOS with an eDP panel and a 4K@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@linux.intel.com
> <mailto:ville.syrjala@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@intel.com
> <mailto:clinton.a.taylor@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@lists.freedesktop.org
> <mailto:Intel-gfx@lists.freedesktop.org>
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org <mailto:Intel-gfx@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-03-17 21:42 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-09 0:15 [PATCH] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2015-12-09 20:53 ` Ville Syrjälä
2016-02-10 0:28 ` [PATCH V3] " clinton.a.taylor
2016-02-10 3:29 ` Thulasimani, Sivakumar
2016-02-10 22:58 ` Clint Taylor
2016-02-10 14:27 ` Ville Syrjälä
2016-02-11 1:37 ` Marc Herbert
2016-02-11 8:29 ` Daniel Vetter
2016-02-11 10:48 ` Ville Syrjälä
2016-02-10 9:43 ` ✗ Fi.CI.BAT: failure for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev2) Patchwork
2016-02-11 23:22 ` [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2016-02-12 1:11 ` Marc Herbert
2016-02-12 11:25 ` Ville Syrjälä
2016-02-12 11:18 ` Ville Syrjälä
2016-02-12 18:51 ` Clint Taylor
2016-02-13 2:06 ` [PATCH V5] " clinton.a.taylor
2016-02-15 13:16 ` Ville Syrjälä
2016-02-16 2:44 ` Thulasimani, Sivakumar
2016-02-16 9:45 ` ✗ Fi.CI.BAT: warning for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev4) Patchwork
2016-02-16 17:19 ` [PATCH V6] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2016-02-16 17:44 ` [PATCH V7] " clinton.a.taylor
2016-02-17 16:56 ` Ville Syrjälä
2016-02-25 13:49 ` Ville Syrjälä
2016-02-25 23:52 ` Clint Taylor
2016-02-16 17:48 ` ✓ Fi.CI.BAT: success for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev5) Patchwork
2016-03-09 21:58 ` [PATCH V8] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2016-03-10 8:08 ` Maarten Lankhorst
2016-03-11 17:04 ` Clint Taylor
2016-03-10 13:35 ` Ville Syrjälä
2016-03-10 7:00 ` ✗ Fi.CI.BAT: warning for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev7) Patchwork
2016-03-10 18:42 ` [PATCH V9] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2016-03-10 21:43 ` [PATCH V10] " clinton.a.taylor
2016-03-11 7:09 ` ✗ Fi.CI.BAT: failure for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev9) Patchwork
2016-03-15 21:34 ` [PATCH V11] drm/i915/skl: SKL CDCLK change on modeset tracking VCO clinton.a.taylor
2016-03-16 7:27 ` Daniel Vetter
2016-03-16 23:28 ` Clint Taylor
2016-03-17 21:18 ` Rodrigo Vivi
2016-03-17 21:38 ` Clint Taylor [this message]
2016-03-16 10:01 ` ✗ Fi.CI.BAT: failure for drm/i915/skl: SKL CDCLK change on modeset tracking VCO (rev10) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56EB23C7.4070402@intel.com \
--to=clinton.a.taylor@intel.com \
--cc=Intel-gfx@lists.freedesktop.org \
--cc=daniel@ffwll.ch \
--cc=rodrigo.vivi@gmail.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.