On Thu, Dec 15, 2011 at 12:30:38PM -0800, Jesse Barnes wrote: > More i9xx mode set cleanups, further simplifying the mode set path and > making it easier to extend. > > Signed-off-by: Jesse Barnes <jbar...@virtuousgeek.org>
This is imo too much refactoring for just one patch. I'd like the extract intel_update_lvds in a separate patch. That one intermingled with the update_pll extraction makes things hard to follow. Patches 1&2 of this series are queued for -next. Also, can we _please_ have the same treatment for ironlake_crtc_mode_set? That monstrositiy is already around 450 lines and growing ... Yours, Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 402 > ++++++++++++++++++++-------------- > 1 files changed, 236 insertions(+), 166 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index b08089c..0eff163 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4848,7 +4848,7 @@ static void i9xx_update_pll_dividers(struct drm_crtc > *crtc, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int pipe = intel_crtc->pipe; > - u32 fp, fp2; > + u32 fp, fp2 = 0; > > if (IS_PINEVIEW(dev)) { > fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2; > @@ -4874,6 +4874,233 @@ static void i9xx_update_pll_dividers(struct drm_crtc > *crtc, > } > } > > +static void intel_update_lvds(struct drm_crtc *crtc, intel_clock_t *clock, > + struct drm_display_mode *adjusted_mode) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + int pipe = intel_crtc->pipe; > + u32 temp, lvds_sync = 0; > + > + temp = I915_READ(LVDS); > + temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP; > + if (pipe == 1) { > + temp |= LVDS_PIPEB_SELECT; > + } else { > + temp &= ~LVDS_PIPEB_SELECT; > + } > + /* set the corresponsding LVDS_BORDER bit */ > + temp |= dev_priv->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. > + */ > + if (clock->p2 == 7) > + temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP; > + else > + temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP); > + > + /* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP) > + * appropriately here, but we need to look more thoroughly into how > + * panels behave in the two modes. > + */ > + /* set the dithering flag on LVDS as needed */ > + if (INTEL_INFO(dev)->gen >= 4) { > + if (dev_priv->lvds_dither) > + temp |= LVDS_ENABLE_DITHER; > + else > + temp &= ~LVDS_ENABLE_DITHER; > + } > + if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) > + lvds_sync |= LVDS_HSYNC_POLARITY; > + if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) > + lvds_sync |= LVDS_VSYNC_POLARITY; > + if ((temp & (LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY)) > + != lvds_sync) { > + char flags[2] = "-+"; > + DRM_INFO("Changing LVDS panel from " > + "(%chsync, %cvsync) to (%chsync, %cvsync)\n", > + flags[!(temp & LVDS_HSYNC_POLARITY)], > + flags[!(temp & LVDS_VSYNC_POLARITY)], > + flags[!(lvds_sync & LVDS_HSYNC_POLARITY)], > + flags[!(lvds_sync & LVDS_VSYNC_POLARITY)]); > + temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY); > + temp |= lvds_sync; > + } > + I915_WRITE(LVDS, temp); > +} > + > +static void i9xx_update_pll(struct drm_crtc *crtc, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode, > + intel_clock_t *clock, intel_clock_t *reduced_clock, > + int num_connectors) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + int pipe = intel_crtc->pipe; > + u32 dpll; > + bool is_sdvo; > + > + is_sdvo = intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) || > + intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI); > + > + dpll = DPLL_VGA_MODE_DIS; > + > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) > + dpll |= DPLLB_MODE_LVDS; > + else > + dpll |= DPLLB_MODE_DAC_SERIAL; > + if (is_sdvo) { > + int pixel_multiplier = > intel_mode_get_pixel_multiplier(adjusted_mode); > + if (pixel_multiplier > 1) { > + if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) > + dpll |= (pixel_multiplier - 1) << > SDVO_MULTIPLIER_SHIFT_HIRES; > + } > + dpll |= DPLL_DVO_HIGH_SPEED; > + } > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) > + dpll |= DPLL_DVO_HIGH_SPEED; > + > + /* compute bitmask from p1 value */ > + if (IS_PINEVIEW(dev)) > + dpll |= (1 << (clock->p1 - 1)) << > DPLL_FPA01_P1_POST_DIV_SHIFT_PINEVIEW; > + else { > + dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT; > + if (IS_G4X(dev) && reduced_clock) > + dpll |= (1 << (reduced_clock->p1 - 1)) << > DPLL_FPA1_P1_POST_DIV_SHIFT; > + } > + switch (clock->p2) { > + case 5: > + dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5; > + break; > + case 7: > + dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7; > + break; > + case 10: > + dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10; > + break; > + case 14: > + dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14; > + break; > + } > + if (INTEL_INFO(dev)->gen >= 4) > + dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT); > + > + if (is_sdvo && intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT)) > + dpll |= PLL_REF_INPUT_TVCLKINBC; > + else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT)) > + /* XXX: just matching BIOS for now */ > + /* dpll |= PLL_REF_INPUT_TVCLKINBC; */ > + dpll |= 3; > + else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && > + intel_panel_use_ssc(dev_priv) && num_connectors < 2) > + dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > + else > + dpll |= PLL_REF_INPUT_DREFCLK; > + > + dpll |= DPLL_VCO_ENABLE; > + I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE); > + POSTING_READ(DPLL(pipe)); > + udelay(150); > + > + /* The LVDS pin pair needs to be on before the DPLLs are enabled. > + * This is an exception to the general rule that mode_set doesn't turn > + * things on. > + */ > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) > + intel_update_lvds(crtc, clock, adjusted_mode); > + > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) > + intel_dp_set_m_n(crtc, mode, adjusted_mode); > + > + I915_WRITE(DPLL(pipe), dpll); > + > + /* Wait for the clocks to stabilize. */ > + POSTING_READ(DPLL(pipe)); > + udelay(150); > + > + if (INTEL_INFO(dev)->gen >= 4) { > + u32 temp = 0; > + if (is_sdvo) { > + temp = intel_mode_get_pixel_multiplier(adjusted_mode); > + if (temp > 1) > + temp = (temp - 1) << > DPLL_MD_UDI_MULTIPLIER_SHIFT; > + else > + temp = 0; > + } > + I915_WRITE(DPLL_MD(pipe), temp); > + } else { > + /* The pixel multiplier can only be updated once the > + * DPLL is enabled and the clocks are stable. > + * > + * So write it again. > + */ > + I915_WRITE(DPLL(pipe), dpll); > + } > +} > + > +static void i8xx_update_pll(struct drm_crtc *crtc, > + struct drm_display_mode *adjusted_mode, > + intel_clock_t *clock, > + int num_connectors) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + int pipe = intel_crtc->pipe; > + u32 dpll; > + > + dpll = DPLL_VGA_MODE_DIS; > + > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) { > + dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT; > + } else { > + if (clock->p1 == 2) > + dpll |= PLL_P1_DIVIDE_BY_TWO; > + else > + dpll |= (clock->p1 - 2) << DPLL_FPA01_P1_POST_DIV_SHIFT; > + if (clock->p2 == 4) > + dpll |= PLL_P2_DIVIDE_BY_4; > + } > + > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT)) > + /* XXX: just matching BIOS for now */ > + /* dpll |= PLL_REF_INPUT_TVCLKINBC; */ > + dpll |= 3; > + else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) && > + intel_panel_use_ssc(dev_priv) && num_connectors < 2) > + dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > + else > + dpll |= PLL_REF_INPUT_DREFCLK; > + > + dpll |= DPLL_VCO_ENABLE; > + I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE); > + POSTING_READ(DPLL(pipe)); > + udelay(150); > + > + /* The LVDS pin pair needs to be on before the DPLLs are enabled. > + * This is an exception to the general rule that mode_set doesn't turn > + * things on. > + */ > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) > + intel_update_lvds(crtc, clock, adjusted_mode); > + > + I915_WRITE(DPLL(pipe), dpll); > + > + /* Wait for the clocks to stabilize. */ > + POSTING_READ(DPLL(pipe)); > + udelay(150); > + > + /* The pixel multiplier can only be updated once the > + * DPLL is enabled and the clocks are stable. > + * > + * So write it again. > + */ > + I915_WRITE(DPLL(pipe), dpll); > +} > + > static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > @@ -4887,15 +5114,13 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > int plane = intel_crtc->plane; > int refclk, num_connectors = 0; > intel_clock_t clock, reduced_clock; > - u32 dpll, dspcntr, pipeconf; > - bool ok, has_reduced_clock = false, is_sdvo = false, is_dvo = false; > - bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false; > + u32 dspcntr, pipeconf; > + bool ok, has_reduced_clock = false, is_sdvo = false; > + bool is_lvds = false, is_tv = false; > struct drm_mode_config *mode_config = &dev->mode_config; > struct intel_encoder *encoder; > const intel_limit_t *limit; > int ret; > - u32 temp; > - u32 lvds_sync = 0; > > list_for_each_entry(encoder, &mode_config->encoder_list, base.head) { > if (encoder->base.crtc != crtc) > @@ -4911,18 +5136,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > if (encoder->needs_tv_clock) > is_tv = true; > break; > - case INTEL_OUTPUT_DVO: > - is_dvo = true; > - break; > case INTEL_OUTPUT_TVOUT: > is_tv = true; > break; > - case INTEL_OUTPUT_ANALOG: > - is_crt = true; > - break; > - case INTEL_OUTPUT_DISPLAYPORT: > - is_dp = true; > - break; > } > > num_connectors++; > @@ -4969,71 +5185,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > i9xx_update_pll_dividers(crtc, &clock, has_reduced_clock ? > &reduced_clock : NULL); > > - dpll = DPLL_VGA_MODE_DIS; > - > - if (!IS_GEN2(dev)) { > - if (is_lvds) > - dpll |= DPLLB_MODE_LVDS; > - else > - dpll |= DPLLB_MODE_DAC_SERIAL; > - if (is_sdvo) { > - int pixel_multiplier = > intel_mode_get_pixel_multiplier(adjusted_mode); > - if (pixel_multiplier > 1) { > - if (IS_I945G(dev) || IS_I945GM(dev) || > IS_G33(dev)) > - dpll |= (pixel_multiplier - 1) << > SDVO_MULTIPLIER_SHIFT_HIRES; > - } > - dpll |= DPLL_DVO_HIGH_SPEED; > - } > - if (is_dp) > - dpll |= DPLL_DVO_HIGH_SPEED; > - > - /* compute bitmask from p1 value */ > - if (IS_PINEVIEW(dev)) > - dpll |= (1 << (clock.p1 - 1)) << > DPLL_FPA01_P1_POST_DIV_SHIFT_PINEVIEW; > - else { > - dpll |= (1 << (clock.p1 - 1)) << > DPLL_FPA01_P1_POST_DIV_SHIFT; > - if (IS_G4X(dev) && has_reduced_clock) > - dpll |= (1 << (reduced_clock.p1 - 1)) << > DPLL_FPA1_P1_POST_DIV_SHIFT; > - } > - switch (clock.p2) { > - case 5: > - dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5; > - break; > - case 7: > - dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7; > - break; > - case 10: > - dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10; > - break; > - case 14: > - dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14; > - break; > - } > - if (INTEL_INFO(dev)->gen >= 4) > - dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT); > - } else { > - if (is_lvds) { > - dpll |= (1 << (clock.p1 - 1)) << > DPLL_FPA01_P1_POST_DIV_SHIFT; > - } else { > - if (clock.p1 == 2) > - dpll |= PLL_P1_DIVIDE_BY_TWO; > - else > - dpll |= (clock.p1 - 2) << > DPLL_FPA01_P1_POST_DIV_SHIFT; > - if (clock.p2 == 4) > - dpll |= PLL_P2_DIVIDE_BY_4; > - } > - } > - > - if (is_sdvo && is_tv) > - dpll |= PLL_REF_INPUT_TVCLKINBC; > - else if (is_tv) > - /* XXX: just matching BIOS for now */ > - /* dpll |= PLL_REF_INPUT_TVCLKINBC; */ > - dpll |= 3; > - else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > - dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > + if (IS_GEN2(dev)) > + i8xx_update_pll(crtc, adjusted_mode, &clock, num_connectors); > else > - dpll |= PLL_REF_INPUT_DREFCLK; > + i9xx_update_pll(crtc, mode, adjusted_mode, &clock, > + has_reduced_clock ? &reduced_clock : NULL, > + num_connectors); > > /* setup pipeconf */ > pipeconf = I915_READ(PIPECONF(pipe)); > @@ -5062,97 +5219,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > pipeconf &= ~PIPECONF_DOUBLE_WIDE; > } > > - dpll |= DPLL_VCO_ENABLE; > > DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B'); > drm_mode_debug_printmodeline(mode); > > - I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE); > - > - POSTING_READ(DPLL(pipe)); > - udelay(150); > - > - /* The LVDS pin pair needs to be on before the DPLLs are enabled. > - * This is an exception to the general rule that mode_set doesn't turn > - * things on. > - */ > - if (is_lvds) { > - temp = I915_READ(LVDS); > - temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP; > - if (pipe == 1) { > - temp |= LVDS_PIPEB_SELECT; > - } else { > - temp &= ~LVDS_PIPEB_SELECT; > - } > - /* set the corresponsding LVDS_BORDER bit */ > - temp |= dev_priv->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. > - */ > - if (clock.p2 == 7) > - temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP; > - else > - temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP); > - > - /* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP) > - * appropriately here, but we need to look more thoroughly into > how > - * panels behave in the two modes. > - */ > - /* set the dithering flag on LVDS as needed */ > - if (INTEL_INFO(dev)->gen >= 4) { > - if (dev_priv->lvds_dither) > - temp |= LVDS_ENABLE_DITHER; > - else > - temp &= ~LVDS_ENABLE_DITHER; > - } > - if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC) > - lvds_sync |= LVDS_HSYNC_POLARITY; > - if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC) > - lvds_sync |= LVDS_VSYNC_POLARITY; > - if ((temp & (LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY)) > - != lvds_sync) { > - char flags[2] = "-+"; > - DRM_INFO("Changing LVDS panel from " > - "(%chsync, %cvsync) to (%chsync, %cvsync)\n", > - flags[!(temp & LVDS_HSYNC_POLARITY)], > - flags[!(temp & LVDS_VSYNC_POLARITY)], > - flags[!(lvds_sync & LVDS_HSYNC_POLARITY)], > - flags[!(lvds_sync & LVDS_VSYNC_POLARITY)]); > - temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY); > - temp |= lvds_sync; > - } > - I915_WRITE(LVDS, temp); > - } > - > - if (is_dp) { > - intel_dp_set_m_n(crtc, mode, adjusted_mode); > - } > - > - I915_WRITE(DPLL(pipe), dpll); > - > - /* Wait for the clocks to stabilize. */ > - POSTING_READ(DPLL(pipe)); > - udelay(150); > - > - if (INTEL_INFO(dev)->gen >= 4) { > - temp = 0; > - if (is_sdvo) { > - temp = intel_mode_get_pixel_multiplier(adjusted_mode); > - if (temp > 1) > - temp = (temp - 1) << > DPLL_MD_UDI_MULTIPLIER_SHIFT; > - else > - temp = 0; > - } > - I915_WRITE(DPLL_MD(pipe), temp); > - } else { > - /* The pixel multiplier can only be updated once the > - * DPLL is enabled and the clocks are stable. > - * > - * So write it again. > - */ > - I915_WRITE(DPLL(pipe), dpll); > - } > - > if (HAS_PIPE_CXSR(dev)) { > if (intel_crtc->lowfreq_avail) { > DRM_DEBUG_KMS("enabling CxSR downclocking\n"); > -- > 1.7.4.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx