Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/slpc: Update the frequency debugfs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 06, 2022 at 05:24:34PM -0400, Rodrigo Vivi wrote:
> On Wed, Oct 05, 2022 at 08:59:43AM -0700, Vinay Belgaumkar wrote:
> > Read the values stored in the SLPC structures. Remove the
> > fields that are no longer valid (like RPS interrupts) as
> > well.
> > 
> > v2: Move all functionality changes to this patch (Jani)
> > v3: Fix compile warning and if condition (Jani)
> > 
> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_rps.c | 46 ++++++++++++++++++++++++++++-
> >  1 file changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> > index 737db780db00..fc23c562d9b2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > @@ -2219,7 +2219,7 @@ u32 intel_rps_get_rpn_frequency(struct intel_rps *rps)
> >  		return intel_gpu_freq(rps, rps->min_freq);
> >  }
> >  
> > -void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
> > +static void rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
> >  {
> >  	struct intel_gt *gt = rps_to_gt(rps);
> >  	struct drm_i915_private *i915 = gt->i915;
> > @@ -2382,6 +2382,50 @@ void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
> >  		   intel_gpu_freq(rps, rps->efficient_freq));
> >  }
> >  
> > +static void slpc_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
> > +{
> > +	struct intel_gt *gt = rps_to_gt(rps);
> > +	struct intel_uncore *uncore = gt->uncore;
> > +	struct intel_rps_freq_caps caps;
> > +	u32 pm_mask;
> > +
> > +	gen6_rps_get_freq_caps(rps, &caps);
> > +	pm_mask = intel_uncore_read(uncore, GEN6_PMINTRMSK);
> > +
> > +	drm_printf(p, "PM MASK=0x%08x\n", pm_mask);
> > +	drm_printf(p, "pm_intrmsk_mbz: 0x%08x\n",
> > +		   rps->pm_intrmsk_mbz);
> > +	drm_printf(p, "RPSTAT1: 0x%08x\n", intel_uncore_read(uncore, GEN6_RPSTAT1));
> > +	drm_printf(p, "RPNSWREQ: %dMHz\n", intel_rps_get_requested_frequency(rps));
> > +	drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
> > +		   intel_gpu_freq(rps, caps.min_freq));
> > +	drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
> > +		   intel_gpu_freq(rps, caps.rp1_freq));
> > +	drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
> > +		   intel_gpu_freq(rps, caps.rp0_freq));
> > +	drm_printf(p, "Current freq: %d MHz\n",
> > +		   intel_rps_get_requested_frequency(rps));
> > +	drm_printf(p, "Actual freq: %d MHz\n",
> > +		   intel_rps_read_actual_frequency(rps));
> > +	drm_printf(p, "Min freq: %d MHz\n",
> > +		   intel_rps_get_min_frequency(rps));
> > +	drm_printf(p, "Boost freq: %d MHz\n",
> > +		   intel_rps_get_boost_frequency(rps));
> > +	drm_printf(p, "Max freq: %d MHz\n",
> > +		   intel_rps_get_max_frequency(rps));
> > +	drm_printf(p,
> > +		   "efficient (RPe) frequency: %d MHz\n",
> > +		   intel_gpu_freq(rps, caps.rp1_freq));
> 
> Well, my feelings with these are:
> 
> 1. We have these already in sysfs and we don't need to duplicated here.
> But we have this already duplicated for years
> 
> 2. We should probably simply remove this file when using SLPC and force
> folks to look to the sysfs files?
> 
> 3. Maybe we should take the simple lazy approach to just fix the values
> that are wrong?
> 
> But well, we might end up bikeshedding this for years... At least the
> new version for SLPC is clean, so I won't block.
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> (but I will hold the merge until tomorrow to see if anyone disagrees)

pushed now. Any clean-up can be done on top if we see a need.
Thanks for the patch.

> 
> 
> > +}
> > +
> > +void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
> > +{
> > +	if (rps_uses_slpc(rps))
> > +		return slpc_frequency_dump(rps, p);
> > +	else
> > +		return rps_frequency_dump(rps, p);
> > +}
> > +
> >  static int set_max_freq(struct intel_rps *rps, u32 val)
> >  {
> >  	struct drm_i915_private *i915 = rps_to_i915(rps);
> > -- 
> > 2.35.1
> >