[Intel-gfx] [PATCH 13/13] drm/i915/skl: Prefer even dividers for SKL DPLLs

[Paulo Zanoni]

2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni at gmail.com>:
> 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau at intel.com>:
>> Currently, if an odd divider improves the deviation (minimizes it), we
>> take that divider. The recommendation is to prefer even dividers.
>
> The doc says "It is preferred to get as close to the DCO central
> frequency as possible, but using an even divider value takes
> precedence.", but I'm wondering if they meant "prefer even over odd in
> case they have the same deviation" or just "even divider is preferred
> as long as it's on the deviation threshold, even if there's an odd
> divider with minimal/no deviation". I see you implement the last
> option - if you don't count the possible bug mentioned on my review of
> patch 12.
>
> Assuming the loop order will be fixed on patch 12, and assuming you
> are correctly interpreting the spec, then your patch does what it
> says, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>.

I kept looking at the spec, and the section that describes the 4 steps
for the algorithm totally clarifies what's the correct interpretation.
Please see step 4. An "even" divider with any acceptable deviation is
preferred over any possible "odd" divider, it doesn't matter what is
the best deviation of the "odd" dividers.

Considering this, I think we really should invert the loops as I
suggested on patch 12, and then we should modify this patch so that it
breaks the loop only after we've also iterated over all DCOs. I guess
these changes are a requirement for the R-B tags on patches 12 and 13
as long as you don't have counter arguments.

We should still consider breaking the loop earlier in case we reach 0
deviation, and we should still consider comparing the pdeviation with
the ndeviation before picking central_freq, dco_freq and divider.
These things are not requirements for the R-B tags, but, as I said,
i'd like to see your opinion.

>
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 381a8c9..54344c3 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1308,6 +1308,13 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
>>                                                       dco_freq,
>>                                                       p);
>>                         }
>> +
>> +                       /*
>> +                        * If a solution is found with an even divider, prefer
>> +                        * this one.
>> +                        */
>> +                       if (d == 0 && ctx.p)
>> +                               break;
>>                 }
>>         }
>>
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni