On 13/06/2014 04:48 μμ, Dirk Brandewie wrote: > On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote: >> On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote: >>> On 12/06/2014 12:15 πμ, Doug Smythies wrote: >>>> >>>> >>>> -----Original Message----- >>>> From: Stratos Karafotis [mailto:strat...@semaphore.gr] >>>> Sent: June-11-2014 13:20 >>>> To: Doug Smythies >>>> Cc: linux...@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> r...@rjwysocki.net; viresh.ku...@linaro.org; dirk.j.brande...@intel.com >>>> Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct >>>> >>>> On 2014.06.11 13:20 Stratos Karafotis wrote: >>>>> On 11/06/2014 06:02 μμ, Doug Smythies wrote: >>>>>> >>>>>> On 2104.06.11 07:08 Stratos Karafotis wrote: >>>>>>> On 11/06/2014 04:41 μμ, Doug Smythies wrote: >>>>>>> >>>>>>> No. >>>>>> >>>>>>> The intent was only ever to round properly the pseudo floating point >>>>>>> result of the divide. >>>>>>> It was much more important (ugh, well 4 times more) when FRACBITS was >>>>>>> still 6, which also got changed to 8 in a recent patch. >>>>>>> >>>>>> >>>>>> Are you sure? >>>>>> >>>>>> Yes. >>>>>> >>>>>>> This rounding was very recently added. >>>>>>> As far as I can understand, I don't see the meaning of this rounding, >>>>>>> as is. >>>>>>> Even if FRAC_BITS was 6, I think it would have almost no improvement in >>>>>>> calculations. >>>>>> >>>>>> Note: I had not seen this e-mail when I wrote a few minutes ago: >>>>>> >>>>>> You may be correct. >>>>>> If Dirk agrees, I will re-analyse the entire driver for rounding effects >>>>>> soon. >>>>>> When FRACBITS was 6 there were subtle cases where the driver would get >>>>>> stuck, and not make a final pstate change, with the default PID gains. >>>>>> Other things have changed, and the analysis needs to be re-done. >>>>>> >>>> >>>>> Could you please elaborate a little bit more what we need these 2 lines >>>>> below? >>>>> > > Sorry for being MIA on this thread I have been up to my eyeballs. > >>>>> if ((rem << 1) >= int_tofp(sample->mperf)) >>>>> core_pct += 1; > > The rounding should have been > core_pct += (1 << (FRAC_BITS-1)); > Since core_pct is is in fixeded point notation at this point. Adding .5 to > core_pct to round up. > > As Stratos pointed out the the current code only adds 1/256 to core_pct > > Since core_pct_busy stays in fixed point through out the rest of the > calculations ans we only do the rounding when the PID is returning an > int I think we can safely remove these two lines. >
Please let me know if you want me to send a new patch for this (after the merge window). Or will you or Doug handle this? >> Depending on the original reason, it may or may not be. >> >> In theory, it may help reduce numerical drift resulting from rounding always >> in >> one direction only, but I'm not really sure if that matters here. >> >> Doug seems to have carried out full analysis, though. >> >> Rafael >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > Thank you all, for your comments! Stratos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/