Re: [Intel-gfx] [PATCH 1/3] drm/i915/gt: Change RC6 residency functions to accept register ID's

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

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@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(&gt->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(&gt->rc6, GEN6_GT_GFX_RC6);
> > +	val = intel_rc6_residency_ns(&gt->rc6, RC6_RES_REG_RC6);
> >
> >	if (HAS_RC6p(i915))
> > -		val += intel_rc6_residency_ns(&gt->rc6, GEN6_GT_GFX_RC6p);
> > +		val += intel_rc6_residency_ns(&gt->rc6, RC6_RES_REG_RC6p);
> >
> >	if (HAS_RC6pp(i915))
> > -		val += intel_rc6_residency_ns(&gt->rc6, GEN6_GT_GFX_RC6pp);
> > +		val += intel_rc6_residency_ns(&gt->rc6, RC6_RES_REG_RC6pp);
> >
> >	return val;
> >  }
>
> --
> Jani Nikula, Intel Open Source Graphics Center