From mboxrd@z Thu Jan 1 00:00:00 1970 From: "M, Satheeshakrishna" Subject: Re: [PATCH 61/89] drm/i915/skl: Determine enabled PLL and its linkrate/pixel clock Date: Wed, 01 Oct 2014 16:21:51 +0530 Message-ID: <542BDCC7.6030803@intel.com> References: <1409830075-11139-1-git-send-email-damien.lespiau@intel.com> <1409830075-11139-62-git-send-email-damien.lespiau@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id C8C616E2DB for ; Wed, 1 Oct 2014 03:52:01 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni , Damien Lespiau Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On 9/23/2014 1:42 AM, Paulo Zanoni wrote: > 2014-09-04 8:27 GMT-03:00 Damien Lespiau: >> From: Satheeshakrishna M >> >> v2: Fixup compilation due to the removal of the intel_ddi_dpll_id enum. >> And add a fixme about the abuse of pipe_config here. >> >> v3: Rebase on top of the hsw_ddi_clock_get() rename (Damien) >> >> Signed-off-by: Satheeshakrishna M (v1) >> Signed-off-by: Damien Lespiau (v3) >> Signed-off-by: Daniel Vetter (v2) >> --- >> drivers/gpu/drm/i915/i915_reg.h | 5 ++ >> drivers/gpu/drm/i915/intel_ddi.c | 114 ++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 118 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 2364ece..794d0ba 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6367,6 +6367,7 @@ enum punit_power_well { >> #define DPLL_CRTL1_LINK_RATE_1620 3 >> #define DPLL_CRTL1_LINK_RATE_1080 4 >> #define DPLL_CRTL1_LINK_RATE_2160 5 >> +#define DPLL_CRTL1_LINK_RATE_SHIFT(id) ((id)*6+1) > I'd move this to a few lines above, where the MASK and RATE > definitions are. Possibly reimplement the other macros using the new > one (if the lines don't look to big/ugly). I will move it next to MASK and RATE definitions. I didn't really get what you meant my reimplement here :( >> /* DPLL control2 */ >> #define DPLL_CTRL2 0x6C05C >> @@ -6374,6 +6375,7 @@ enum punit_power_well { >> #define DPLL_CTRL2_DDI_CLK_SEL_MASK(port) (3<<((port)*3+1)) >> #define DPLL_CTRL2_DDI_CLK_SEL(clk, port) (clk<<((port)*3+1)) >> #define DPLL_CTRL2_DDI_SEL_OVERRIDE(port) (1<<(port*3)) >> +#define DPLL_CTRL2_DDI_CLK_SEL_SHIFT(port) (port*3+1) > Same here: move a few lines above, and possibly reimplement the others > using the new one. Also, use "(port)" instead of "port", since we > don't want to risk really-hard-to-debug bugs due to operator > precedence on those "*" and "+" operations. Will move up CLK_SEL and add parenthesis. Again didn't get the what to reimplement here. >> /* DPLL Status */ >> #define DPLL_STATUS 0x6C060 >> @@ -6400,6 +6402,9 @@ enum punit_power_well { >> #define DPLL_CFGCR2_PDIV(x) (x<<2) >> #define DPLL_CFGCR2_CENTRAL_FREQ_MASK (3) >> >> +#define GET_CFG_CR1_REG(id) (DPLL1_CFGCR1 + (id - 1) * 8) >> +#define GET_CFG_CR2_REG(id) (DPLL1_CFGCR2 + (id - 1) * 8) > The macros above are not really trivial due to the fact that the "id" > is undefined and confusing. Please convert this to an inline function, > since what we're actually expecting here is "enum intel_dpll_id", > which has ID 0 for DPLL 1, which can be super confusing (imagine > someone passing ID 1 for DPLL 1, not realizing it should be using the > correct enum...). If we use a function we can specify the correct > expected enum for the ID type, which helps the programmer find out > what is the expected thing to pass to the function. Expectation is that user of this macro should know value passed :) Anyway, let me try to have a inline function here. > >> + >> enum central_freq { >> freq_9600 = 0, >> freq_9000 = 1, >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index e7a5428..b5cfb07 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -649,6 +649,114 @@ static int intel_ddi_calc_wrpll_link(struct drm_i915_private *dev_priv, >> return (refclk * n * 100) / (p * r); >> } >> >> +static int skl_calc_wrpll_link(struct drm_i915_private *dev_priv, >> + enum intel_dpll_id dpll) >> +{ >> + uint32_t cfgcr1_reg, cfgcr2_reg; >> + uint32_t cfgcr1, cfgcr2; >> + uint32_t p0, p1, p2, dco_freq; >> + >> + cfgcr1_reg = GET_CFG_CR1_REG(dpll); >> + cfgcr2_reg = GET_CFG_CR2_REG(dpll); >> + >> + cfgcr1 = I915_READ(cfgcr1_reg); >> + cfgcr2 = I915_READ(cfgcr2_reg); > Bikeshed: I'd probably call these cfgcr{1,2}_val to avoid confusion. ok >> + >> + p0 = (cfgcr2 & DPLL_CFGCR2_PDIV_MASK) >> 2; >> + p2 = (cfgcr2 & DPLL_CFGCR2_KDIV_MASK) >> 5; >> + >> + if (cfgcr2 & DPLL_CFGCR2_QDIV_MODE(1)) >> + p1 = (cfgcr2 & DPLL_CFGCR2_QDIV_RATIO_MASK) >> 8; >> + else >> + p1 = 1; >> + >> + >> + switch (p0) { >> + case pdiv_1: >> + p0 = 1; >> + break; >> + case pdiv_2: >> + p0 = 2; >> + break; >> + case pdiv_3: >> + p0 = 3; >> + break; >> + case pdiv_7: >> + p0 = 7; >> + break; >> + } >> + >> + switch (p2) { >> + case kdiv_5: >> + p2 = 5; >> + break; >> + case kdiv_2: >> + p2 = 2; >> + break; >> + case kdiv_3: >> + p2 = 3; >> + break; >> + case kdiv_1: >> + p2 = 1; >> + break; >> + } > I really think that if we had something like: > #define DPLL_CFGCR2_PDIV_7 (4 << 2) > we'd be able to avoid this "convert to enum and then get the value" > part, making the function much simpler... ok > >> + >> + dco_freq = (cfgcr1 & DPLL_CFGCR1_DCO_INTEGER_MASK) * 24 * 1000; >> + >> + dco_freq += (((cfgcr1 & DPLL_CFGCR1_DCO_FRACTION_MASK) >> 9) * 24 * >> + 1000) / 0x8000; >> + >> + return dco_freq / (p0 * p1 * p2 * 5); >> +} >> + >> + >> +static void skl_ddi_clock_get(struct intel_encoder *encoder, >> + struct intel_crtc_config *pipe_config) >> +{ >> + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> + enum port port = intel_ddi_get_encoder_port(encoder); >> + int link_clock = 0; >> + uint32_t dpll_ctl1, dpll; >> + >> + /* FIXME: This should be tracked in the pipe config. */ >> + dpll = I915_READ(DPLL_CTRL2); >> + dpll &= DPLL_CTRL2_DDI_CLK_SEL_MASK(port); >> + dpll >>= DPLL_CTRL2_DDI_CLK_SEL_SHIFT(port); >> + >> + dpll_ctl1 = I915_READ(DPLL_CTRL1); >> + >> + if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(dpll)) { >> + link_clock = skl_calc_wrpll_link(dev_priv, dpll); >> + } else { >> + link_clock = dpll_ctl1 & DPLL_CRTL1_LINK_RATE_MASK(dpll); >> + link_clock >>= DPLL_CRTL1_LINK_RATE_SHIFT(dpll); >> + >> + switch (link_clock) { >> + case DPLL_CRTL1_LINK_RATE_810: >> + link_clock = 81000; >> + break; >> + case DPLL_CRTL1_LINK_RATE_1350: >> + link_clock = 135000; >> + break; >> + case DPLL_CRTL1_LINK_RATE_2700: >> + link_clock = 270000; >> + break; > What about 1620 and 1080? > > >> + default: >> + break; > We're just silently failing here, which will probably result in later > WARNs on the HW state readout/check code. So we should probably give a > WARN() here to make debugging easier :) Since link_clock is read out from the HW, we'll never end up in default case. Anyway, I'll add a WARN() > >> + } >> + link_clock *= 2; >> + } >> + >> + pipe_config->port_clock = link_clock; >> + >> + if (pipe_config->has_dp_encoder) >> + pipe_config->adjusted_mode.crtc_clock = >> + intel_dotclock_calculate(pipe_config->port_clock, >> + &pipe_config->dp_m_n); >> + else >> + pipe_config->adjusted_mode.crtc_clock = pipe_config->port_clock; >> +} >> + >> static void hsw_ddi_clock_get(struct intel_encoder *encoder, >> struct intel_crtc_config *pipe_config) >> { >> @@ -1535,6 +1643,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >> enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder; >> u32 temp, flags = 0; >> + struct drm_device *dev = dev_priv->dev; >> >> temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); >> if (temp & TRANS_DDI_PHSYNC) >> @@ -1606,7 +1715,10 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >> dev_priv->vbt.edp_bpp = pipe_config->pipe_bpp; >> } >> >> - hsw_ddi_clock_get(encoder, pipe_config); >> + if (INTEL_INFO(dev)->gen < 9) > I'm sure Daniel would request a change to "<= 8" instead of "< 9" here :) Couldn't figure of what the convention is. Will fix this instance :) > I should probably also complain about the fact that clock calculation > is a very confusing thing, and I never know which value should be > assigned where, and I also never know when to multiply by 2 or 5 or > divide by 10... > > Note: not everything mentioned above is a hard requirement for a R-B > tag. Deciding what's a bikeshed and what's not is left as an exercise > to the reader. > >> + hsw_ddi_clock_get(encoder, pipe_config); >> + else >> + skl_ddi_clock_get(encoder, pipe_config); >> } >> >> static void intel_ddi_destroy(struct drm_encoder *encoder) >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >