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

[Dave Gordon <david.s.gordon@intel.com>]

On 16/06/15 10:40, Chris Wilson wrote:
> 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.

Done.

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

My terminal doesn't include <> in word selections (but DOES include "/"
and ".") so selecting a pathname just works :) But I've removed the
<angle.brackets> anyway.

> If you can decode numeric status values, do so.

Done. I've added a _repr function for decoding the enum and used it
everywhere.

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

There's only one other use of HAS_GUC_UCODE (in intel_guc_ucode_init())
and that one doesn't and mustn't trigger an error if false. I don't see
why it should be an *error* here either; the caller hasn't done anything
wrong, and there's no h/w or s/w failure. An empty result (EOF) is a
nice way of saying that there's nothing to say, without making the user
think something broke.

In fact it may be perfectly meaningful to continue rather than
returning; consider the case of a future GuC that comes with firmware
preloaded, so HAS_GUC() is true but HAS_GUC_UCODE() is FALSE. We could
still read and decode the GUC_STATUS even though we haven't loaded any
firmware.

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

It's just a register dump; nothing secret there. You could read them
with IGT's register dumper anyway.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx