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

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-20 15:59:41)
> On Fri, 20 Oct 2017 16:50:10 +0200, Chris Wilson  
> <chris at chris-wilson.co.uk> wrote:
> 
> > 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.
> 
> I just wanted to cover the case where we have guc but without fw
> (explicitly expressed as null path) while skipping fw info for
> platforms without guc. No more guesses. No unwanted noise.

The first makes sense, but the second? We either used the struct uc_fw
or we didn't. If we did and didn't have guc, that raises a question or
two -- as it should never be the case, but for the error capture it
doesn't have to know, it just prints what it has captured.
-Chris