From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 09/26] drm/i915: split PLL update code out of i9xx_crtc_mode_set Date: Sat, 24 Mar 2012 00:00:11 +0100 Message-ID: <20120323230011.GC29390@phenom.ffwll.local> References: <1332452348-8814-1-git-send-email-jbarnes@virtuousgeek.org> <1332452348-8814-10-git-send-email-jbarnes@virtuousgeek.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id A2B869EEF1 for ; Fri, 23 Mar 2012 15:59:27 -0700 (PDT) Received: by wgbdr12 with SMTP id dr12so1984305wgb.12 for ; Fri, 23 Mar 2012 15:59:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1332452348-8814-10-git-send-email-jbarnes@virtuousgeek.org> 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: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 22, 2012 at 02:38:51PM -0700, Jesse Barnes wrote: > Makes it more readable and maintainable. ValleyView will add its own > PLL update function in a later patch. > > v2: split LVDS bits out of this patch (Daniel) > > Signed-off-by: Jesse Barnes In-depth review ftw, i.e. two things below. Otherwise this looks good, so keep everything else exactly as-is (I hate doing these kind of split things up reviews). -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 291 ++++++++++++++++++++-------------- > 1 files changed, 172 insertions(+), 119 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f27728c..84480da 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5230,6 +5230,170 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc, > } > } > > +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); > + > + 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; Just curious: Does gen2 really have ssc on lvds? > + 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, > @@ -5243,9 +5407,9 @@ 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, vsyncshift; > - 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, vsyncshift; > + 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; > @@ -5267,18 +5431,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++; > @@ -5322,71 +5477,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)); > @@ -5415,24 +5511,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > > /* default to 8bpc */ > pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN); > - if (is_dp) { > - if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) { > - pipeconf |= PIPECONF_BPP_6 | > - PIPECONF_DITHER_EN | > - PIPECONF_DITHER_TYPE_SP; > - } > - } This hunk seems to have gotten lost. Why? > - > - 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. > @@ -5485,35 +5567,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > 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.5.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48