Re: [Intel-gfx] [PATCH] drm/i915/rps: Centralize computation of freq caps

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 21 Mar 2022 11:17:46 -0700, Lucas De Marchi wrote:
>
> On Mon, Mar 21, 2022 at 10:56:04AM -0700, Ashutosh Dixit wrote:
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps_types.h b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > index 3941d8551f52..5990df35b393 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps_types.h
> > @@ -37,6 +37,13 @@ enum {
> >	INTEL_RPS_TIMER,
> > };
> >
> > +/* Freq caps exposed by HW, in units of 16.67 MHz for recent gens */
>
> "recent gens" is probably too broad. Since we are exporting this struct
> to other parts of the driver and we are already abstracting the
> register location and bit position, maybe we should also already
> abstract the unit in the same place? i.e. just make it always be
> "units of 16.67 MHz", or even just make it a standard Hz unit.
>
> If this would rather make things more complicated for places that expect
> "hw units", then maybe just say in this comment the value is in "HW
> units" and intel_gpu_freq() should be used to convert to hz.

Fixed in v2. In v2 I've changed the comment to say values are in "hw units"
and intel_gpu_freq() should be used to convert to MHz.

I have also added a couple of hopefully clarifying comments to
intel_rps_get_freq_caps() in v2. Some of the history of this code was not
clear to me previously and I had to trace all the way back to cee991cb9323
to figure out what is happening.

Thanks.
--
Ashutosh

> > +struct intel_rps_freq_caps {
> > +	u8 rp0_freq;		/* Non-overclocked max frequency. */
> > +	u8 rp1_freq;		/* "less than" RP0 power/freqency */
> > +	u8 min_freq;		/* AKA RPn. Minimum frequency */
> > +};
> > +
> > struct intel_rps {
> >	struct mutex lock; /* protects enabling and the worker */
> >