From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 24/31] drm/i915: asserts for lvds pre_enable Date: Thu, 13 Jun 2013 23:26:04 +0300 Message-ID: <1371155164.24509.20.camel@ideak-mobl> References: <1370432073-27634-1-git-send-email-daniel.vetter@ffwll.ch> <1370432073-27634-25-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id D8AD4E5C24 for ; Thu, 13 Jun 2013 13:26:11 -0700 (PDT) In-Reply-To: <1370432073-27634-25-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, 2013-06-05 at 13:34 +0200, Daniel Vetter wrote: > Lots of bangin my head against the wall^UExperiments have shown that > we really need to enable the lvds port before we enable plls. Strangely > that seems to include the fdi rx pll on the pch. > > Anyway, encode this new evidence with a few nice WARNs. > > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++------------- > drivers/gpu/drm/i915/intel_drv.h | 16 ++++++++++++++++ > drivers/gpu/drm/i915/intel_lvds.c | 17 ++++++++++++----- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5d84fea..7b34a92 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -874,8 +874,8 @@ static const char *state_string(bool enabled) > } > > /* Only for pre-ILK configs */ > -static void assert_pll(struct drm_i915_private *dev_priv, > - enum pipe pipe, bool state) > +void assert_pll(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state) > { > int reg; > u32 val; > @@ -888,10 +888,8 @@ static void assert_pll(struct drm_i915_private *dev_priv, > "PLL state assertion failure (expected %s, current %s)\n", > state_string(state), state_string(cur_state)); > } > -#define assert_pll_enabled(d, p) assert_pll(d, p, true) > -#define assert_pll_disabled(d, p) assert_pll(d, p, false) > > -static struct intel_shared_dpll * > +struct intel_shared_dpll * > intel_crtc_to_shared_dpll(struct intel_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > @@ -903,9 +901,9 @@ intel_crtc_to_shared_dpll(struct intel_crtc *crtc) > } > > /* For ILK+ */ > -static void assert_shared_dpll(struct drm_i915_private *dev_priv, > - struct intel_shared_dpll *pll, > - bool state) > +void assert_shared_dpll(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll, > + bool state) > { > bool cur_state; > struct intel_dpll_hw_state hw_state; > @@ -924,8 +922,6 @@ static void assert_shared_dpll(struct drm_i915_private *dev_priv, > "%s assertion failure (expected %s, current %s)\n", > pll->name, state_string(state), state_string(cur_state)); > } > -#define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) > -#define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > > static void assert_fdi_tx(struct drm_i915_private *dev_priv, > enum pipe pipe, bool state) > @@ -989,15 +985,16 @@ static void assert_fdi_tx_pll_enabled(struct drm_i915_private *dev_priv, > WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n"); > } > > -static void assert_fdi_rx_pll_enabled(struct drm_i915_private *dev_priv, > - enum pipe pipe) > +void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state) > { > int reg; > u32 val; > > reg = FDI_RX_CTL(pipe); > val = I915_READ(reg); > - WARN(!(val & FDI_RX_PLL_ENABLE), "FDI RX PLL assertion failure, should be active but is disabled\n"); > + WARN(!!(val & FDI_RX_PLL_ENABLE) != state, > + "FDI RX PLL assertion failure, should be active but is disabled\n"); The message should be updated too. > } > > static void assert_panel_unlocked(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 6f28375..ea8aa5e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -732,6 +732,22 @@ extern int intel_overlay_attrs(struct drm_device *dev, void *data, > extern void intel_fb_output_poll_changed(struct drm_device *dev); > extern void intel_fb_restore_mode(struct drm_device *dev); > > +struct intel_shared_dpll * > +intel_crtc_to_shared_dpll(struct intel_crtc *crtc); > + > +void assert_shared_dpll(struct drm_i915_private *dev_priv, > + struct intel_shared_dpll *pll, > + bool state); > +#define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true) > +#define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false) > +void assert_pll(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state); > +#define assert_pll_enabled(d, p) assert_pll(d, p, true) > +#define assert_pll_disabled(d, p) assert_pll(d, p, false) > +void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, > + enum pipe pipe, bool state); > +#define assert_fdi_rx_pll_enabled(d, p) assert_fdi_rx_pll(d, p, true) > +#define assert_fdi_rx_pll_disabled(d, p) assert_fdi_rx_pll(d, p, false) > extern void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, > bool state); > #define assert_pipe_enabled(d, p) assert_pipe(d, p, true) > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index 159aa9f..36f8901 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -120,12 +120,20 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); > struct drm_device *dev = encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > struct drm_display_mode *fixed_mode = > lvds_encoder->attached_connector->base.panel.fixed_mode; > - int pipe = intel_crtc->pipe; > + int pipe = crtc->pipe; > u32 temp; > > + if (HAS_PCH_SPLIT(dev)) { > + assert_fdi_rx_pll_disabled(dev_priv, pipe); > + assert_shared_dpll_disabled(dev_priv, > + intel_crtc_to_shared_dpll(crtc)); I think if we pick a shared PLL that is currently used by another port this will trigger. Should the PLL selection be limited to non-shared PLLs for LVDS? > + } else { > + assert_pll_disabled(dev_priv, pipe); > + } > + > temp = I915_READ(lvds_encoder->reg); > temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP; > > @@ -142,7 +150,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > > /* set the corresponsding LVDS_BORDER bit */ > temp &= ~LVDS_BORDER_ENABLE; > - temp |= intel_crtc->config.gmch_pfit.lvds_border_bits; > + temp |= crtc->config.gmch_pfit.lvds_border_bits; > /* Set the B0-B3 data pairs corresponding to whether we're going to > * set the DPLLs for dual-channel mode or not. > */ > @@ -162,8 +170,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder) > if (INTEL_INFO(dev)->gen == 4) { > /* Bspec wording suggests that LVDS port dithering only exists > * for 18bpp panels. */ > - if (intel_crtc->config.dither && > - intel_crtc->config.pipe_bpp == 18) > + if (crtc->config.dither && crtc->config.pipe_bpp == 18) > temp |= LVDS_ENABLE_DITHER; > else > temp &= ~LVDS_ENABLE_DITHER;