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

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Mar 18, 2014 at 06:27:03PM -0700, 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
> > 
> 
> Okay, as stated before, you are correct - I need to bring back RPe/RPn
> distinction. I think using the mix of values (basically s/_delay/_freq)
> doesn't fully relize what I was hoping to achieve. I don't think there
> is ever a case, except when debugging where it's easier to refer to the
> RP mnemonic. How strongly do you feel about this one? I'd really prefer
> to just fix RPe/RPn.
> 
> Does anyone else have an opinion on:
> "max_freq_hardlimit" vs. "rp0"
> 
> Does anyone else want to review this one?
> 

Okay, I started on this, and I somewhat agree. How about:

        u8 cur_freq;            /* Current frequency (cached, may not == HW) */
        u8 min_freq_softlimit;  /* Minimum frequency permitted by the driver */
        u8 max_freq_softlimit;  /* Max frequency permitted by the driver */
        u8 max_freq;            /* Maximum frequency, RP0 if not overclocking */
        u8 min_freq;            /* AKA RPn. Minimum frequency */
        u8 efficient_freq;      /* AKA RPe. Pre-determined balanced frequency */
        u8 rp1_freq;            /* "less than" RP0 power/freqency */
        u8 rp0_freq;            /* Non-overclocked max frequency. */

Conveniently, this matches sysfs, minus the efficiency one, and I don't think
there's a point in explicitly storing RPn, since it's always == min_freq.

> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center