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@xxxxxxxxx>
> >
> > 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?
> 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@xxxxxxxxx>
> > Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
> > ---
> > 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?
> 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.
>
> > + 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.
>
> > + 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.
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
> >
