2014-09-04 8:27 GMT-03:00 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
> From: Satheeshakrishna M <satheeshakrishna.m@xxxxxxxxx>
>
> 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 <satheeshakrishna.m@xxxxxxxxx> (v1)
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx> (v3)
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> (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).
>
> /* 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.
>
> /* 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.
> +
> 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.
> +
> + 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...
> +
> + 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 :)
> + }
> + 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 :)
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
