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

[Michal Wajdeczko]

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.

Michal