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

But well, MC6 seems to be a totally different thing and CC6 is
really strange because the C stands for Core and this can get
very confusing with the SoC or CPU C states...  :(

At least with the Render we know which level of the IP we
are looking at when looking at media...

> +	switch (REG_FIELD_GET(MTL_CC_MASK, gt_core_status)) {
> +	case MTL_CC0:
> +		seq_puts(m, "on\n");

maybe "*C0" instead of "on"?

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

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

> +	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
>