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@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
