Re: [PATCH 12/13] drm/i915/skl: Replace the HDMI DPLL divider computation algorithm

[Damien Lespiau <damien.lespiau@intel.com>]

On Wed, May 27, 2015 at 06:28:06PM -0300, Paulo Zanoni wrote:
> > @@ -1185,90 +1278,56 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
> >         uint64_t dco_central_freq[3] = {8400000000ULL,
> >                                         9000000000ULL,
> >                                         9600000000ULL};
> > +       static const int even_dividers[] = {  4,  6,  8, 10, 12, 14, 16, 18, 20,
> > +                                            24, 28, 30, 32, 36, 40, 42, 44,
> > +                                            48, 52, 54, 56, 60, 64, 66, 68,
> > +                                            70, 72, 76, 78, 80, 84, 88, 90,
> > +                                            92, 96, 98 };
> > +       static const int odd_dividers[] = { 3, 5, 7, 9, 15, 21, 35 };
> > +       static const struct {
> > +               const int *list;
> > +               int n_dividers;
> > +       } dividers[] = {
> > +               { even_dividers, ARRAY_SIZE(even_dividers) },
> > +               { odd_dividers, ARRAY_SIZE(odd_dividers) },
> > +       };
> > +       struct skl_wrpll_context ctx;
> > +       unsigned int dco, d, i;
> > +       unsigned int p0, p1, p2;
> > +
> > +       skl_wrpll_context_init(&ctx);
> > +
> > +       for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> > +               for (d = 0; d < ARRAY_SIZE(dividers); d++) {
> 
> From what I can tell by reading the documentation, it really seems
> that the two lines above should be inverted. First we do everything
> for the even (including all DCOs), and only then for the odd. It's
> easier to realize this when you look at the part of the doc that says,
> in order: "next candidate divider \n next DCO central frequency \n
> next divider parity". Anyway, this is not exactly a problem right now
> since we never break the loop, but you change this on the next patch,
> so it's probably best to discuss this here.

You're absolutely right, those 2 lines need to be inverted.

> > +       /*
> > +        * gcc incorrectly analyses that these can be used without being
> > +        * initialized. To be fair, it's hard to guess.
> 
> Why didn't you just write the mathematical proof as a comment? I hope
> it's not too big to fit on the 80 column margin limit.
> 
> Jokes aside, the only "blocker" would be the DCO first vs odd/even
> first. I'd like to hear your interpretation on this.
> 
> > +        */
> > +       p0 = p1 = p2 = 0;
> > +       skl_wrpll_get_multipliers(ctx.p, &p0, &p1, &p2);
> > +       skl_wrpll_params_populate(wrpll_params, afe_clock, ctx.central_freq,
> > +                                 p0, p1, p2);

The "proof" is in tools/skl_compute_wrpll.c, in the test_multipliers()
function that unit-tests skl_wrpll_get_multipliers().

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx