[Intel-gfx] [PATCH v3] drm/i915: Add Guc/HuC firmware details to error state

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-20 15:44:02)
> On Fri, 20 Oct 2017 11:41:57 +0200, Joonas Lahtinen  
> <joonas.lahtinen at linux.intel.com> wrote:
> 
> > On Thu, 2017-10-19 at 20:23 +0000, Michal Wajdeczko wrote:
> >> Include GuC and HuC firmware details in captured error state
> >> to provide additional debug information. To reuse existing
> >> uc firmware pretty printer, introduce new drm-printer variant
> >> that works with our i915_error_state_buf output. Also update
> >> uc firmware pretty printer to accept const input.
> >>
> >> v2: don't rely on current caps (Chris)
> >>     dump correct fw info (Michal)
> >> v3: simplify capture of custom paths (Chris)
> >>
> >> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >
> > A few comments below.
> >
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >
> > Regards, Joonas
> >
> >> @@ -774,6 +793,11 @@ int i915_error_state_to_str(struct  
> >> drm_i915_error_state_buf *m,
> >>      err_print_capabilities(m, &error->device_info);
> >>      err_print_params(m, &error->params);
> >>
> >> +    if (error->device_info.has_guc) {
> >> +            intel_uc_fw_dump(&error->guc_fw, &p);
> >> +            intel_uc_fw_dump(&error->huc_fw, &p);
> >> +    }
> >
> > I might use "error->{g,h}uc_fw.path" as the condition, for both
> 
> If we use "uc_fw->path" as condition then we have to assume that
> lack of uc firmware info is for this reason, and not from corrupted
> or incomplete or legacy format error dump ;)
> 
> I would prefer to stay with "has_guc" condition here, and trim
> output from uc fw printer if path is not set. This way we will
> cover all possible scenario (no guc = no fw, no fw = no fw info)

I still do not understand the insistence on has_guc. no fw is no fw, no
guc is no guc.
 
> > individually. We will have DMC here in the future, too.
> 
> Hmm, while not here, info about DMC fw is already included in
> error dump. But in their case reported DMC fw state (loaded flag,
> version) is taken directly from dev_priv instead of captured error
> state.

I know. You are not to copy that mistake! Also if you should happen to
feel inclined to rectify the heinous actions, preferably by removing
the dmc entirely, please do so.
-Chris