| Message ID | 1431020329-11414-14-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6348
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 302/302 302/302
SNB 316/316 316/316
IVB 342/342 342/342
BYT -1 228/228 227/228
BDW 321/321 321/321
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_userptr_blits@forked-sync-swapping-normal PASS(2) INIT(1)PASS(1)
Note: You need to pay more attention to line start with '*'
2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@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@intel.com>. > > Signed-off-by: Damien Lespiau <damien.lespiau@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@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@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@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
On Wed, May 27, 2015 at 07:08:38PM -0300, Paulo Zanoni wrote: > 2015-05-27 18:39 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > > 2015-05-07 14:38 GMT-03:00 Damien Lespiau <damien.lespiau@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@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. Right, too bad I read that last, but that's indeed what I think and well and answered a previous comment with that already. > 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. Yup, agreed. > 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. With v2, the break on deviation 0 would just be a "fast path" because we can't improve on that. Still worth doing as a follow up (I like having real diffs rather than v2/v3 with all the changes squashed).
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; } }
Currently, if an odd divider improves the deviation (minimizes it), we take that divider. The recommendation is to prefer even dividers. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++ 1 file changed, 7 insertions(+)