[PATCH 1/3] drm/i915/gt: Change RC6 residency functions to accept register ID's
Dixit, Ashutosh
ashutosh.dixit at intel.com
Wed Oct 19 05:22:59 UTC 2022
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 at intel.com> 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
More information about the dri-devel
mailing list