| Message ID | 1434393394-21002-7-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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); +} + +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)) + 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))); + + return 0; +} + static int i915_edp_psr_status(struct seq_file *m, void *data) { struct drm_info_node *node = m->private; @@ -5046,6 +5082,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_gem_hws_bsd", i915_hws_info, 0, (void *)VCS}, {"i915_gem_hws_vebox", i915_hws_info, 0, (void *)VECS}, {"i915_gem_batch_pool", i915_gem_batch_pool_info, 0}, + {"i915_guc_load_status", i915_guc_load_status_info, 0}, {"i915_frequency_info", i915_frequency_info, 0}, {"i915_hangcheck_info", i915_hangcheck_info, 0}, {"i915_drpc_info", i915_drpc_info, 0},