On Mon, 17 Oct 2022 01:27:35 -0700, Jani Nikula wrote: Hi Jani, Thanks for reviewing, great suggestions overall. I have taken care of most of them in series version v6. Please see below. > On Fri, 14 Oct 2022, Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx> wrote: > > @@ -811,9 +809,23 @@ u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const i915_reg_t reg) > > return mul_u64_u32_div(time_hw, mul, div); > > } > > > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg) > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id) > > +{ > > + return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, id), 1000); > > +} > > + > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id) > > intel_rc6_print_rc5_res is unnecessary duplication. > > intel_rc6_print_residency() maybe? Done. > > > { > > - return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, reg), 1000); > > + struct intel_gt *gt = m->private; > > + i915_reg_t reg = gt->rc6.res_reg[id]; > > + intel_wakeref_t wakeref; > > + > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > > + seq_printf(m, "%s %u (%llu us)\n", title, > > + intel_uncore_read(gt->uncore, reg), > > + intel_rc6_residency_us(>->rc6, id)); > > } > > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.h b/drivers/gpu/drm/i915/gt/intel_rc6.h > > index b6fea71afc223..584d2d3b2ec3f 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.h > > @@ -6,7 +6,7 @@ > > #ifndef INTEL_RC6_H > > #define INTEL_RC6_H > > > > -#include "i915_reg_defs.h" > > +#include "intel_rc6_types.h" > > You can forward declare enums as a gcc extension. > > enum rc6_res_reg; Tried but was seeing compile errors so left as is. > > struct intel_engine_cs; > > struct intel_rc6; > > @@ -21,7 +21,10 @@ void intel_rc6_sanitize(struct intel_rc6 *rc6); > > void intel_rc6_enable(struct intel_rc6 *rc6); > > void intel_rc6_disable(struct intel_rc6 *rc6); > > > > -u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, i915_reg_t reg); > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg); > > +u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id); > > "const enum" makes no sense. Removed. Probably const for pass-by-value function arguments never makes sense, I had left the const thinking it would indicate that the function won't modify that argument, but is probably not worth it so removed all "const enum"s. > > > > > #endif /* INTEL_RC6_H */ > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6_types.h b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > index e747492b2f46e..0386a3f6e4dc6 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > @@ -13,7 +13,17 @@ > > > > struct drm_i915_gem_object; > > > > +enum rc6_res_reg { > > + RC6_RES_REG_RC6_LOCKED, > > + RC6_RES_REG_RC6, > > + RC6_RES_REG_RC6p, > > + RC6_RES_REG_RC6pp > > +}; > > Naming: intel_rc6_* for all. Done. > I think you need to take the abstraction further away from > registers. You don't need the *register* part here for anything. Stop > thinking in terms of registers in the interface. > > The callers care about things like "RC6+ residency since boot", and the > callers don't care about where or how this information originates > from. They just want the info, and the register is an implementation > detail hidden behind the interface. > > I.e. use the enum to identify the data you want, not which register it > comes from. Done, please take a look at the new patch. > > > + > > +#define VLV_RC6_RES_REG_MEDIA_RC6 RC6_RES_REG_RC6p > > Please handle this in the enum. Done. > > > + > > struct intel_rc6 { > > + i915_reg_t res_reg[4]; > > Maybe the id enum should have _MAX as last value, used for size here. Done. Thanks. -- Ashutosh > > > u64 prev_hw_residency[4]; > > u64 cur_residency[4]; > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > index 8c70b7e120749..a236e3f8f3183 100644 > > --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c > > +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > @@ -19,11 +19,11 @@ static u64 rc6_residency(struct intel_rc6 *rc6) > > > > /* XXX VLV_GT_MEDIA_RC6? */ > > > > - result = intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6); > > + result = intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6); > > if (HAS_RC6p(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6p); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6p); > > if (HAS_RC6pp(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6pp); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6pp); > > > > return result; > > } > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > > index 958b37123bf12..15d0f88136394 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -148,13 +148,13 @@ static u64 __get_rc6(struct intel_gt *gt) > > struct drm_i915_private *i915 = gt->i915; > > u64 val; > > > > - val = intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6); > > + val = intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6); > > > > if (HAS_RC6p(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6p); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6p); > > > > if (HAS_RC6pp(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6pp); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6pp); > > > > return val; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center