On 13/06/2014 04:48 μμ, Dirk Brandewie wrote:

On 06/12/2014 01:03 PM, Rafael J. Wysocki wrote:

quoted

On Thursday, June 12, 2014 05:35:59 PM Stratos Karafotis wrote:

quoted

On 12/06/2014 12:15 πμ, Doug Smythies wrote:

quoted

-----Original Message-----
From: Stratos Karafotis [mailto:stratosk@semaphore.gr]
Sent: June-11-2014 13:20
To: Doug Smythies
Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; rjw@rjwysocki.net; viresh.kumar@linaro.org; dirk.j.brandewie@intel.com
Subject: Re: [PATCH] cpufreq: intel_pstate: Fix rounding of core_pct

On 2014.06.11 13:20 Stratos Karafotis wrote:

quoted

On 11/06/2014 06:02 μμ, Doug Smythies wrote:

quoted

On 2104.06.11 07:08 Stratos Karafotis wrote:

quoted

On 11/06/2014 04:41 μμ, Doug Smythies wrote:

No.

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

        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?

quoted

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thank you all, for your comments!

Stratos