Re: [PATCH 61/89] drm/i915/skl: Determine enabled PLL and its linkrate/pixel clock

[Paulo Zanoni <przanoni@gmail.com>]

2014-10-01 7:51 GMT-03:00 M, Satheeshakrishna <satheeshakrishna.m@intel.com>:
> On 9/23/2014 1:42 AM, Paulo Zanoni wrote:
>>
>> 2014-09-04 8:27 GMT-03:00 Damien Lespiau<damien.lespiau@intel.com>:
>>>
>>> From: Satheeshakrishna M<satheeshakrishna.m@intel.com>
>>>
>>> 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@intel.com>  (v1)
>>> Signed-off-by: Damien Lespiau<damien.lespiau@intel.com>  (v3)
>>> Signed-off-by: Daniel Vetter<daniel.vetter@ffwll.ch>  (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.

What I meant is: since you created a macro that expands to
"(port*3+1)", you could use it in the definition of the other macros,
instead of writing "(port*3+1)" again. The downside is that you'd have
to type many more characters, so maybe this change is not worth it.

So we'd have something like:
#define DPLL_CTRL2_XX_SHIFT(port) (port*3+1)
#define DPLL_CTRL2_XX_SEL_MASK(port) (3 << DPLL_CTRL2_XX_SHIFT(port))
#define DPLL_CTRL2_XX_SEL(clk, port) ((clk) << DPLL_CTRL2_XX_SHIFT(port))

Etc.

>>>
>>>   /* 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.

But currently, he'll have to read a lot of code to know which value to
pass: this is what I did while reviewing the patch. If this were a
real function, all that would be needed would be to look at the
function declaration.


>>
>>
>>> +
>>>   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()

Yeah, but maybe in some gen the register address will change, then
we'll start reading garbage and silently do the wrong thing. Or maybe
we have some serious memory corruption, or IDK. I know this is a
little too paranoid, but we have tons of WARNs on our code and they do
help us catch bugs, especially on the automated IGT tests where QA is
not really looking at the screen to check if it is actually working,
but there's a bot looking at new dmesg WARNs.


>
>>
>>> +               }
>>> +               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
>>
>>
>



-- 
Paulo Zanoni