Re: [Intel-gfx] [PATCH 5/5] drm/i915/mtl: C6 residency and C state type for MTL SAMedia

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Oct 24, 2022 at 12:16:28PM -0700, Dixit, Ashutosh wrote:
> On Fri, 21 Oct 2022 09:35:32 -0700, Rodrigo Vivi wrote:
> >
> 
> Hi Rodrigo,
> 
> > On Wed, Oct 19, 2022 at 04:37:21PM -0700, Ashutosh Dixit wrote:
> > > From: Badal Nilawar <badal.nilawar@intel.com>
> > >
> > > Add support for C6 residency and C state type for MTL SAMedia. Also add
> > > mtl_drpc.
> >
> > I believe this patch deserves a slip between the actual support and
> > the debugfs, but I'm late to the review, so feel free to ignore this
> > comment...
> 
> Sorry didn't understand what you mean by "slip", you mean the patch should
> be split in two?

yeap, I meant split.. sorry for the typo...

> 
> > but I do have more dummy doubts below:
> >
> > >
> > > v2: Fixed review comments (Ashutosh)
> > > v3: Sort registers and fix whitespace errors in intel_gt_regs.h (Matt R)
> > >     Remove MTL_CC_SHIFT (Ashutosh)
> > >     Adapt to RC6 residency register code refactor (Jani N)
> > > v4: Move MTL branch to top in drpc_show
> > > v5: Use FORCEWAKE_MT identical to gen6_drpc (Ashutosh)
> > >
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 58 ++++++++++++++++++-
> > >  drivers/gpu/drm/i915/gt/intel_gt_regs.h       |  5 ++
> > >  drivers/gpu/drm/i915/gt/intel_rc6.c           | 17 ++++--
> > >  3 files changed, 75 insertions(+), 5 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 5d6b346831393..f15a7486a9866 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> > > @@ -256,6 +256,60 @@ 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, mt_fwake_req;
> > > +	u32 mtl_powergate_enable = 0, mtl_powergate_status = 0;
> > > +
> > > +	mt_fwake_req = intel_uncore_read_fw(uncore, FORCEWAKE_MT);
> > > +	gt_core_status = intel_uncore_read(uncore, MTL_MIRROR_TARGET_WP1);
> > > +
> > > +	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: ");
> >
> > (Just a "loud" thought here in this chunck, but no actual action requested)
> >
> > should we really use "R" (Render) for this Media C state?
> 
> This function is called for both render and media gt's. But let's think
> about this. We can call easily call them e.g. RC6 for render and MC6 for
> media too if that is more accurate and descriptive. On the other hand, do
> we really need to introduce a new term like MC6? Maybe we just stick to
> RC/RC6 terminology for anything on the GPU?
> 
> > But well, MC6 seems to be a totally different thing and CC6
> 
> MC6 is not the same as RC6 for the media tile?

I saw people using this in some bug, but I thought this was for the package
since they were refering to the rc6 as well...

so you can see how  confusing this bad naming is by itself... not a single
document or bspec page that put a good name we can use... so let's not
invent it here and move with the RC6.

> 
> > and CC6 is really strange because the C stands for Core and this can get
> > very confusing with the SoC or CPU C states...  :(
> 
> Yes Bspec 66300 refers to these as core C states but refers to GT and
> IA. So it's confusing.
> 
> > At least with the Render we know which level of the IP we
> > are looking at when looking at media...
> 
> Yup that's why I've left this as RC/RC6 in Patch v6.

ack

> 
> >
> > > +	switch (REG_FIELD_GET(MTL_CC_MASK, gt_core_status)) {
> > > +	case MTL_CC0:
> > > +		seq_puts(m, "on\n");
> >
> > maybe "*C0" instead of "on"?
> 
> Done in v6. Though this string is "on" also in the previous function
> gen6_drpc. Also, if we are calling this C0 we could call the C6 state as
> just C6 (which would mean RC6 for render and MC6 for media). But I thought
> RC6 is better for both render and media.

oh, now that we agreed on staying with the RC I believe RC0 is better.


> 
> >
> > > +		break;
> > > +	case MTL_CC6:
> > > +		seq_puts(m, "RC6\n");
> > > +		break;
> > > +	default:
> > > +		seq_puts(m, "Unknown\n");
> >
> > maybe use a MISSING_CASE() here?
> > or raise a WARN?
> 
> Done in v6.
> 
> >
> > > +		break;
> > > +	}
> > > +
> > > +	seq_printf(m, "Multi-threaded Forcewake Request: 0x%x\n", mt_fwake_req);
> > > +	if (gt->type == GT_MEDIA)
> > > +		seq_printf(m, "Media Power Well: %s\n",
> > > +			   (mtl_powergate_status &
> > > +			    GEN9_PWRGT_MEDIA_STATUS_MASK) ? "Up" : "Down");
> >
> > gate is up and power is down or gate is down and power is up?
> 
> Yes name is confusing but is the same as Bspec and also gen6_drpc. So the
> prints "Media Power Well: Up" or "Media Power Well: Down" are correct (0 is
> down, 1 is up). Similarly for Render below.

:( oh, indeed!

Let's stay with it.

> 
> Thanks.
> --
> Ashutosh
> 
> >
> > > +	else
> > > +		seq_printf(m, "Render Power Well: %s\n",
> > > +			   (mtl_powergate_status &
> > > +			    GEN9_PWRGT_RENDER_STATUS_MASK) ? "Up" : "Down");
> > > +
> > > +	/* Works for both render and media gt's */
> > > +	intel_rc6_print_residency(m, "RC6 residency since boot:", INTEL_RC6_RES_RC6);
> > > +
> > > +	return fw_domains_show(m, NULL);
> > > +}
> > > +
> > >  static int drpc_show(struct seq_file *m, void *unused)
> > >  {
> > >	struct intel_gt *gt = m->private;
> > > @@ -264,7 +318,9 @@ static int drpc_show(struct seq_file *m, void *unused)
> > >	int err = -ENODEV;
> > >
> > >	with_intel_runtime_pm(gt->uncore->rpm, wakeref) {
> > > -		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > > +		if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
> > > +			err = mtl_drpc(m);
> > > +		else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > >			err = vlv_drpc(m);
> > >		else if (GRAPHICS_VER(i915) >= 6)
> > >			err = gen6_drpc(m);
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index d8dbd0ac3b064..a0ddaf243593c 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -24,6 +24,9 @@
> > >  /* MTL workpoint reg to get core C state and actual freq of 3D, SAMedia */
> > >  #define MTL_MIRROR_TARGET_WP1			_MMIO(0xc60)
> > >  #define   MTL_CAGF_MASK				REG_GENMASK(8, 0)
> > > +#define   MTL_CC0				0x0
> > > +#define   MTL_CC6				0x3
> > > +#define   MTL_CC_MASK				REG_GENMASK(12, 9)
> > >
> > >  /* RPM unit config (Gen8+) */
> > >  #define RPM_CONFIG0				_MMIO(0xd00)
> > > @@ -1512,6 +1515,8 @@
> > >  #define FORCEWAKE_MEDIA_VLV			_MMIO(0x1300b8)
> > >  #define FORCEWAKE_ACK_MEDIA_VLV			_MMIO(0x1300bc)
> > >
> > > +#define MTL_MEDIA_MC6				_MMIO(0x138048)
> > > +
> > >  #define GEN6_GT_THREAD_STATUS_REG		_MMIO(0x13805c)
> > >  #define   GEN6_GT_THREAD_STATUS_CORE_MASK	0x7
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > index 6db4e60d5fba5..2ee4051e4d961 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > @@ -553,10 +553,19 @@ static void __intel_rc6_disable(struct intel_rc6 *rc6)
> > >
> > >  static void rc6_res_reg_init(struct intel_rc6 *rc6)
> > >  {
> > > -	rc6->res_reg[INTEL_RC6_RES_RC6_LOCKED] = GEN6_GT_GFX_RC6_LOCKED;
> > > -	rc6->res_reg[INTEL_RC6_RES_RC6] = GEN6_GT_GFX_RC6;
> > > -	rc6->res_reg[INTEL_RC6_RES_RC6p] = GEN6_GT_GFX_RC6p;
> > > -	rc6->res_reg[INTEL_RC6_RES_RC6pp] = GEN6_GT_GFX_RC6pp;
> > > +	memset(rc6->res_reg, INVALID_MMIO_REG.reg, sizeof(rc6->res_reg));
> > > +
> > > +	switch (rc6_to_gt(rc6)->type) {
> > > +	case GT_MEDIA:
> > > +		rc6->res_reg[INTEL_RC6_RES_RC6] = MTL_MEDIA_MC6;
> > > +		break;
> > > +	default:
> > > +		rc6->res_reg[INTEL_RC6_RES_RC6_LOCKED] = GEN6_GT_GFX_RC6_LOCKED;
> > > +		rc6->res_reg[INTEL_RC6_RES_RC6] = GEN6_GT_GFX_RC6;
> > > +		rc6->res_reg[INTEL_RC6_RES_RC6p] = GEN6_GT_GFX_RC6p;
> > > +		rc6->res_reg[INTEL_RC6_RES_RC6pp] = GEN6_GT_GFX_RC6pp;
> > > +		break;
> > > +	}
> > >  }
> > >
> > >  void intel_rc6_init(struct intel_rc6 *rc6)
> > > --
> > > 2.38.0
> > >