| Message ID | 20230308053321.939906-4-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pmu: Use common freq functions with sysfs | expand |
On 3/7/2023 9:33 PM, Ashutosh Dixit wrote: > Using common freq functions with sysfs in PMU (but without taking > forcewake) solves the following issues (a) missing support for MTL (b) For the requested_freq, we read it only if actual_freq is zero below (meaning, GT is in C6). So then what is the point of reading it without a force wake? It will also be zero, correct? Thanks, Vinay. > missing support for older generation (prior to Gen6) (c) missing support > for slpc when freq sampling has to fall back to requested freq. It also > makes the PMU code future proof where sometimes code has been updated for > sysfs and PMU has been missed. > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_rps.c | 10 ---------- > drivers/gpu/drm/i915/gt/intel_rps.h | 1 - > drivers/gpu/drm/i915/i915_pmu.c | 10 ++++------ > 3 files changed, 4 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c > index 49df31927c0e..b03bfbe7ee23 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > @@ -2046,16 +2046,6 @@ void intel_rps_sanitize(struct intel_rps *rps) > rps_disable_interrupts(rps); > } > > -u32 intel_rps_read_rpstat_fw(struct intel_rps *rps) > -{ > - struct drm_i915_private *i915 = rps_to_i915(rps); > - i915_reg_t rpstat; > - > - rpstat = (GRAPHICS_VER(i915) >= 12) ? GEN12_RPSTAT1 : GEN6_RPSTAT1; > - > - return intel_uncore_read_fw(rps_to_gt(rps)->uncore, rpstat); > -} > - > u32 intel_rps_read_rpstat(struct intel_rps *rps) > { > struct drm_i915_private *i915 = rps_to_i915(rps); > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h > index a990f985ab23..60ae27679011 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.h > +++ b/drivers/gpu/drm/i915/gt/intel_rps.h > @@ -53,7 +53,6 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); > u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); > u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); > u32 intel_rps_read_rpstat(struct intel_rps *rps); > -u32 intel_rps_read_rpstat_fw(struct intel_rps *rps); > void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps); > void intel_rps_raise_unslice(struct intel_rps *rps); > void intel_rps_lower_unslice(struct intel_rps *rps); > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index a76c5ce9513d..1a4c9fed257c 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -392,14 +392,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) > * case we assume the system is running at the intended > * frequency. Fortunately, the read should rarely fail! > */ > - val = intel_rps_read_rpstat_fw(rps); > - if (val) > - val = intel_rps_get_cagf(rps, val); > - else > - val = rps->cur_freq; > + val = intel_rps_read_actual_frequency_fw(rps); > + if (!val) > + val = intel_rps_get_requested_frequency_fw(rps), > > add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT], > - intel_gpu_freq(rps, val), period_ns / 1000); > + val, period_ns / 1000); > } > > if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
On Tue, 07 Mar 2023 22:12:49 -0800, Belgaumkar, Vinay wrote: > Hi Vinay, > On 3/7/2023 9:33 PM, Ashutosh Dixit wrote: > > Using common freq functions with sysfs in PMU (but without taking > > forcewake) solves the following issues (a) missing support for MTL (b) > > For the requested_freq, we read it only if actual_freq is zero below > (meaning, GT is in C6). So then what is the point of reading it without a > force wake? It will also be zero, correct? Yes agreed. I had tested this and you do see values for requested freq which look correct even when actual freq is 0 even without taking forcewake. That is why I ended up writing Patch 2/3. However what I missed is what you pointed out that 0xa008 is a shadowed register which cannot be read without taking forcewake. It is probably returning the last value which was written to the shadowed write register. As a result I have dropped the "drm/i915/rps: Expose get_requested_frequency_fw for PMU" patch in v2 of this series. Thanks. -- Ashutosh
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 49df31927c0e..b03bfbe7ee23 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2046,16 +2046,6 @@ void intel_rps_sanitize(struct intel_rps *rps) rps_disable_interrupts(rps); } -u32 intel_rps_read_rpstat_fw(struct intel_rps *rps) -{ - struct drm_i915_private *i915 = rps_to_i915(rps); - i915_reg_t rpstat; - - rpstat = (GRAPHICS_VER(i915) >= 12) ? GEN12_RPSTAT1 : GEN6_RPSTAT1; - - return intel_uncore_read_fw(rps_to_gt(rps)->uncore, rpstat); -} - u32 intel_rps_read_rpstat(struct intel_rps *rps) { struct drm_i915_private *i915 = rps_to_i915(rps); diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index a990f985ab23..60ae27679011 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -53,7 +53,6 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); u32 intel_rps_read_rpstat(struct intel_rps *rps); -u32 intel_rps_read_rpstat_fw(struct intel_rps *rps); void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps); void intel_rps_raise_unslice(struct intel_rps *rps); void intel_rps_lower_unslice(struct intel_rps *rps); diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index a76c5ce9513d..1a4c9fed257c 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -392,14 +392,12 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * case we assume the system is running at the intended * frequency. Fortunately, the read should rarely fail! */ - val = intel_rps_read_rpstat_fw(rps); - if (val) - val = intel_rps_get_cagf(rps, val); - else - val = rps->cur_freq; + val = intel_rps_read_actual_frequency_fw(rps); + if (!val) + val = intel_rps_get_requested_frequency_fw(rps), add_sample_mult(&pmu->sample[__I915_SAMPLE_FREQ_ACT], - intel_gpu_freq(rps, val), period_ns / 1000); + val, period_ns / 1000); } if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
Using common freq functions with sysfs in PMU (but without taking forcewake) solves the following issues (a) missing support for MTL (b) missing support for older generation (prior to Gen6) (c) missing support for slpc when freq sampling has to fall back to requested freq. It also makes the PMU code future proof where sometimes code has been updated for sysfs and PMU has been missed. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/gt/intel_rps.c | 10 ---------- drivers/gpu/drm/i915/gt/intel_rps.h | 1 - drivers/gpu/drm/i915/i915_pmu.c | 10 ++++------ 3 files changed, 4 insertions(+), 17 deletions(-)