| Message ID | 1370432073-27634-25-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <daniel.vetter@ffwll.ch> > --- > 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;
On Thu, Jun 13, 2013 at 10:26 PM, Imre Deak <imre.deak@intel.com> wrote: >> + 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? LVDS has a special clock selection setting and there's only eve one LVDS port on any given machine. Which means we won't ever be able to share the dpll with anything else. I'll add this to the commit message when I resend the patch to fix up the debug output. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, 2013-06-13 at 22:46 +0200, Daniel Vetter wrote: > On Thu, Jun 13, 2013 at 10:26 PM, Imre Deak <imre.deak@intel.com> wrote: > >> + 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? > > LVDS has a special clock selection setting and there's only eve one > LVDS port on any given machine. Which means we won't ever be able to > share the dpll with anything else. Ok, I should've looked closer and realize that dpll matching is not only about rate matching, but also matching the rest of dpll mode bits. But it's clear now we can't get here with a shared dpll, so the assert is ok. > I'll add this to the commit message when I resend the patch to fix up > the debug output. Ok. --Imre
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"); } 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)); + } 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;
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 <daniel.vetter@ffwll.ch> --- 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(-)