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

Chris Wilson
2015-06-16T09:41:00Z

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