2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni@xxxxxxxxx>: > 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@xxxxxxxxx>: >> 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@xxxxxxxxx>. 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
