Re: [PATCH 03/11] drm/i915: Rename and comment all the RPS *stuff*

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sat, Feb 22, 2014 at 11:38:55AM -0800, Ben Widawsky wrote:
> On Sat, Feb 22, 2014 at 11:34:16AM -0800, Ben Widawsky wrote:
> > On Sat, Feb 22, 2014 at 01:37:16PM +0000, Chris Wilson wrote:
> > > On Mon, Feb 17, 2014 at 07:01:44PM -0800, Ben Widawsky wrote:
> > > > The names of the struct members for RPS are stupid. Every time I need to
> > > > do anything in this code I have to spend a significant amount of time to
> > > > remember what it all means. By renaming the variables (and adding the
> > > > comments) I hope to clear up the situation. Indeed doing this make some
> > > > upcoming patches more readable.
> > > > 
> > > > I've avoided ILK because it's possible that the naming used for Ironlake
> > > > matches what is in the docs. I believe the ILK power docs were never
> > > > published, and I am too lazy to dig them up.
> > > > 
> > > > While there may be mistakes, this patch was mostly done via sed. The
> > > > renaming of "hw_max" required a bit of interactivity.
> > > 
> > > It lost the distinction between RPe and RPn. I am in favour of keeping
> > > RP0, RP1, RPe, RPn for the hardware/spec values and adding the set of
> > > soft values used for actual interaction.
> > > -Chris
> > > 
> > 
> > And what is the difference between RPe, and RPn? I honestly have no clue
> > what RPe is.
> > 
> 
> I sent too soon. One other thing I want to make clear with a patch [like
> this] is what the min and max values always are. Names RP0, and RP1,
> while matching the spec do not do this. They have no meaning to anyone
> not well versed in the spec.
> 
> When one wants to find out min/max, or rp0/rp1, they'd always have to do
> a conversion from one to the other by looking at the code (unless we
> added a #define or something to alias it). My point really is, it's
> almost always more useful to know MIN and MAX, and if you must figure
> out if it's RP0, or RP1, then go back to the code to find it.

Right, I think we can have both. We can have RPx for the values we read
out of registers since we often have the spec open at the same time. And
from those we can store a second set of derived values that make sense
for implementing algorithms

That way, it should be easy enough to match what we see in the spec to
the limits we find and how they are transformed into human readable
values, both for exposing to userspace and for driving the gpufreq
algorithms.

It also means that we can place sanity checks at the end that we are
still within the original hw limits.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre