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

[Damien Lespiau]

On Wed, May 27, 2015 at 06:51:13PM -0300, Paulo Zanoni wrote:
> >> +static void skl_wrpll_try_divider(struct skl_wrpll_context *ctx,
> >> +                                 uint64_t central_freq,
> >> +                                 uint64_t dco_freq,
> >> +                                 unsigned int divider)
> >> +{
> >> +       uint64_t deviation;
> >> +
> >> +       deviation = div64_u64(10000 * abs_diff(dco_freq, central_freq),
> >> +                             central_freq);
> >> +
> >> +       /* positive deviation */
> >> +       if (dco_freq >= central_freq) {
> >> +               if (deviation < ctx->min_pdeviation) {
> >> +                       ctx->min_pdeviation = deviation;
> >> +                       ctx->central_freq = central_freq;
> >> +                       ctx->dco_freq = dco_freq;
> >> +                       ctx->p = divider;
> >> +               }
> >> +       /* negative deviation */
> >> +       } else if (deviation < ctx->min_ndeviation) {
> >> +               ctx->min_ndeviation = deviation;
> >> +               ctx->central_freq = central_freq;
> >> +               ctx->dco_freq = dco_freq;
> >> +               ctx->p = divider;
> >> +       }
> 
> Some additional comments:
> 
> Don't we want to break the loop in case we reach a point where any of
> the deviations is zero?

We could, haven't done it in the v2 of the patch, could be done as a
follow up.

> Also notice that, due to the way we loop over the variables at the
> main func, we will always pick the last deviation that was "improved"
> (positive or negative), as opposed to the very minimal deviation. In
> other words, if the last thing our algorithm did was move the
> ndeviation from 600 to 599, we will pick this, even if, in previous
> iterations, we moved the pdeviation from 100 to 1. Is this really what
> we want? Maybe we want to compare min_pdeviation against
> min_ndeviation before picking central_freq, dco_freq and p?

That seems to be a (pretty big!) deficiency of the documented algorithm,
that's definitely something improving the average deviation in the i-g-t
test (average deviation 206.52 Vs 194.47)

> Also, if we loop over the *odd* dividers before looping over the
> *even* divers, and change the comparisons to "<=" instead of just "<",
> and don't "break the loop in case we reach 0 variation" for the odd
> dividers, then our algorithm will give preference to even dividers in
> case we find the same deviation on both odd and even dividers (in
> other words, this will give you the implementation of the alternative
> interpretation of the spec, which we're discussing on patch 13).

But then it would not use an even divider that has a slightly worse
deviation. My reading of the specs is:

  - If there's an even divider with the deviation fitting in the +1%/-6%
    constraint, choose it. (if there are several such even dividers,
    choose the one minimizing the deviation)
  - If we can't find an even divider within +1%/-6%, settle for an odd
    divider that satisfies those constraints.

> I know you probably don't have any of these answers, so I won't block
> the patch review on the problems on this email. But I'd at least like
> to know your opinions here.Maybe we could send some additional
> questions to the spec writers.

I do have some answers :) at least assuming the interpreation above is
the correct one. The two big suggestions you have make quite a
difference in the 2 metrics I gather in the i-g-t test:

v1 of this series:
  even/odd dividers: 338/35
  average deviation: 206.52

v2 of this series:
  even/odd dividers: 363/10
  average deviation: 194.47

It's also easier to follow the changes as diffs in the corresponding
i-g-t series than the v2 of the kernel patches.

testdisplay still cycles through the modes on the HDMI displays I have.

-- 
Damien