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@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?
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@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?
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
> > >
