Re: [PATCH 06/15] drm/i915: Debugfs interface to read GuC load status

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 06/15] drm/i915: Debugfs interface to read GuC load status
Date: Tue, 16 Jun 2015 10:40:56 +0100	[thread overview]
Message-ID: <20150616094056.GO28462@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1434393394-21002-7-git-send-email-david.s.gordon@intel.com>

On Mon, Jun 15, 2015 at 07:36:24PM +0100, Dave Gordon wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> The new node provides access to the status of the common uC loader
> code and the GuC-specific loader; also the scratch registers used
> for communicatio between the i915 driver and the GuC firmware.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 47636f3..c52a745 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2352,6 +2352,42 @@ static int i915_llc(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static void i915_uc_load_status_info(struct seq_file *m, struct intel_uc_fw *uc_fw)
> +{
> +	seq_printf(m, "%s firmware status:\n\tpath: <%s>\n\tfetch: %d\n\tload: %d\n",
> +			uc_fw->uc_name,
> +			uc_fw->uc_fw_path,
> +			uc_fw->uc_fw_fetch_status,
> +			uc_fw->uc_fw_load_status);

If you made this one seq_printf() per line visualing the resulting
format would have been easier - and easier to modify.

Don't use <%s>, that's just visual noise to make cutting and pasting
harder.

If you can decode numeric status values, do so.

> +}
> +
> +static int i915_guc_load_status_info(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_i915_private *dev_priv = node->minor->dev->dev_private;
> +	u32 tmp, i;
> +
> +	if (!HAS_GUC_UCODE(dev_priv->dev))

Here and elsewhere it should be return -ENODEV;

> +		return 0;
> +
> +	i915_uc_load_status_info(m, &dev_priv->guc.guc_fw);
> +
> +	tmp = I915_READ(GUC_STATUS);
> +
> +	seq_printf(m, "\nGuC status 0x%08x:\n", tmp);
> +	seq_printf(m, "\tBootrom status = 0x%x\n",
> +		(tmp & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT);
> +	seq_printf(m, "\tuKernel status = 0x%x\n",
> +		(tmp & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT);
> +	seq_printf(m, "\tMIA Core status = 0x%x\n",
> +		(tmp & GS_MIA_MASK) >> GS_MIA_SHIFT);
> +	seq_puts(m, "\nScratch registers value:\n");
> +	for (i = 0; i < 16; i++)
> +		seq_printf(m, "\t%2d: \t0x%x\n", i, I915_READ(SOFT_SCRATCH(i)));

I have a feeling these probably don't want to be upstreamed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx