Re: [Intel-gfx] [PATCH 2/2] drm/i915/mtl: Add C6 residency support for MTL SAMedia

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

On Tue, 20 Sep 2022 01:06:52 -0700, Jani Nikula wrote:
>
> On Mon, 19 Sep 2022, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> wrote:
> > On Mon, 19 Sep 2022 05:13:18 -0700, Jani Nikula wrote:
> >>
> >> On Mon, 19 Sep 2022, Badal Nilawar <badal.nilawar@intel.com> wrote:
> >> > For MTL SAMedia updated relevant functions and places in the code to get
> >> > Media C6 residency.
> >> >
> >> > v2: Fixed review comments (Ashutosh)
> >> >
> >> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> > Cc: Chris Wilson <chris.p.wilson@intel.com>
> >> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 60 +++++++++++++++++++
> >> >  drivers/gpu/drm/i915/gt/intel_gt_regs.h       | 10 ++++
> >> >  drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c   |  9 ++-
> >> >  drivers/gpu/drm/i915/gt/intel_rc6.c           |  5 +-
> >> >  drivers/gpu/drm/i915/gt/selftest_rc6.c        |  9 ++-
> >> >  drivers/gpu/drm/i915/i915_pmu.c               |  8 ++-
> >> >  6 files changed, 97 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> >> > index 68310881a793..053167b506a9 100644
> >> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> >> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> >> > @@ -269,6 +269,64 @@ static int ilk_drpc(struct seq_file *m)
> >> >	return 0;
> >> >  }
> >> >
> >> > +static int mtl_drpc(struct seq_file *m)
> >> > +{
> >> > +	struct intel_gt *gt = m->private;
> >> > +	struct intel_uncore *uncore = gt->uncore;
> >> > +	u32 gt_core_status, rcctl1, global_forcewake;
> >> > +	u32 mtl_powergate_enable = 0, mtl_powergate_status = 0;
> >> > +	i915_reg_t reg;
> >> > +
> >> > +	gt_core_status = intel_uncore_read(uncore, MTL_MIRROR_TARGET_WP1);
> >> > +
> >> > +	global_forcewake = intel_uncore_read(uncore, FORCEWAKE_GT_GEN9);
> >> > +
> >> > +	rcctl1 = intel_uncore_read(uncore, GEN6_RC_CONTROL);
> >> > +	mtl_powergate_enable = intel_uncore_read(uncore, GEN9_PG_ENABLE);
> >> > +	mtl_powergate_status = intel_uncore_read(uncore,
> >> > +						 GEN9_PWRGT_DOMAIN_STATUS);
> >> > +
> >> > +	seq_printf(m, "RC6 Enabled: %s\n",
> >> > +		   str_yes_no(rcctl1 & GEN6_RC_CTL_RC6_ENABLE));
> >> > +	if (gt->type == GT_MEDIA) {
> >> > +		seq_printf(m, "Media Well Gating Enabled: %s\n",
> >> > +			   str_yes_no(mtl_powergate_enable & GEN9_MEDIA_PG_ENABLE));
> >> > +	} else {
> >> > +		seq_printf(m, "Render Well Gating Enabled: %s\n",
> >> > +			   str_yes_no(mtl_powergate_enable & GEN9_RENDER_PG_ENABLE));
> >> > +	}
> >> > +
> >> > +	seq_puts(m, "Current RC state: ");
> >> > +
> >> > +	switch ((gt_core_status & MTL_CC_MASK) >> MTL_CC_SHIFT) {
> >> > +	case MTL_CC0:
> >> > +		seq_puts(m, "on\n");
> >> > +		break;
> >> > +	case MTL_CC6:
> >> > +		seq_puts(m, "RC6\n");
> >> > +		break;
> >> > +	default:
> >> > +		seq_puts(m, "Unknown\n");
> >> > +		break;
> >> > +	}
> >> > +
> >> > +	if (gt->type == GT_MEDIA)
> >> > +		seq_printf(m, "Media Power Well: %s\n",
> >> > +			   (mtl_powergate_status &
> >> > +			    GEN9_PWRGT_MEDIA_STATUS_MASK) ? "Up" : "Down");
> >> > +	else
> >> > +		seq_printf(m, "Render Power Well: %s\n",
> >> > +			   (mtl_powergate_status &
> >> > +			    GEN9_PWRGT_RENDER_STATUS_MASK) ? "Up" : "Down");
> >> > +
> >> > +	reg = (gt->type == GT_MEDIA) ? MTL_MEDIA_MC6 : GEN6_GT_GFX_RC6;
> >> > +	print_rc6_res(m, "RC6 residency since boot:", reg);
> >>
> >> Cc: Tvrtko, Joonas, Rodrigo
> >>
> >
> > Hi Jani,
> >
> >> IMO the register is not a good abstraction to build interfaces on. I see
> >> that this is not where the idea is introduced, but it'll probably get
> >> you in trouble later on.
> >
> > By "this is not where the idea is introduced" are you referring to what we
> > did here:
> >
> > https://patchwork.freedesktop.org/patch/502372/?series=108091&rev=5
> >
> > in intel_gt_perf_limit_reasons_reg()?
> >
> > Or, should we follow the schema of centralizing the register selection
> > depending on gt type in a single function here too (since this register
> > selection is repeated throughout this patch)?
>
> I'm looking at print_rc6_res(), for example.
>
> It takes the register, reads it, and also passes the register around,
> eventually to intel_rc6_residency_ns(). That does magic on the register
> offset, so it assumes a certain multi-register layout, and relative from
> GEN6_GT_GFX_RC6_LOCKED. Then it assumes the register contents are
> different on different platforms.
>
> So why did we pass around the register to begin with? The knowledge
> about the register offsets and contents are spread around. What if
> another platform gets added with a different register contents or layout
> or offsets?
>
> Registers are a really low level of abstraction, and IMO usually should
> not be passed around like this.

Hi Jani, I've tried to fix this in v5 of this series based on some
discussion which I believe happened between Badal and Rodrigo:

https://patchwork.freedesktop.org/series/108156/

Please take a look at "drm/i915/gt: Change RC6 residency functions to
accept register ID's".

Thanks.
--
Ashutosh