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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
