[Intel-gfx] [PATCH v2] drm/i915: Use the engine name directly in the error_state file

[Chris Wilson]

Quoting Michel Thierry (2018-01-16 18:33:32)
> 
> 
> On 1/15/2018 9:15 AM, Tvrtko Ursulin wrote:
> > 
> > On 10/01/2018 01:21, Michel Thierry wrote:
> >> Instead of using local string names that we will have to keep
> >> maintaining, use the engine->name directly.
> >>
> >> v2: Better invalid engine_id handling, capture_bo will not be able know
> >> the engine_id and end up with -1 (Michal).
> >>
> >> Suggested-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> ---
> >>   drivers/gpu/drm/i915/i915_gpu_error.c | 33 
> >> ++++++++++++++++++++-------------
> >>   1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
> >> b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> index 94499c24f279..422e302161e5 100644
> >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> @@ -34,16 +34,22 @@
> >>   #include "i915_drv.h"
> >> -static const char *engine_str(int engine)
> >> -{
> >> -    switch (engine) {
> >> -    case RCS: return "render";
> >> -    case VCS: return "bsd";
> >> -    case BCS: return "blt";
> >> -    case VECS: return "vebox";
> >> -    case VCS2: return "bsd2";
> >> -    default: return "";
> >> -    }
> >> +static inline const char *intel_engine_name(struct intel_engine_cs 
> >> *engine)
> >> +{
> >> +    return engine ? engine->name : "";
> >> +}
> >> +
> >> +static inline struct intel_engine_cs *
> >> +intel_engine_lookup(struct drm_i915_private *i915, int engine_id)
> >> +{
> >> +    if (engine_id < 0 || engine_id >= I915_NUM_ENGINES)
> >> +        return NULL;
> >> +    return i915->engine[engine_id];
> >> +}
> >> +
> >> +static const char *engine_str(struct drm_i915_private *i915, int 
> >> engine_id)
> >> +{
> >> +    return intel_engine_name(intel_engine_lookup(i915, engine_id));
> >>   }
> > 
> > Feels like a bit of an overkill to have three functions to this trivial 
> > thing but meh. Could also maybe cheat and have engine_id as unsigned int 
> > and so would only need to check for >= I915_NUM_ENGINES.

Overkill? Yes! If you are concerned about the back mapping, just put a
pointer back to the engine (ee->engine). We are not going to reallocate
it after module load, right? RIGHT?!!! :)

> > Anyway, I peeked in intel_error_decode source and couldn't spot anything 
> > that looked it would break. You checked it by running it? Assuming you did:
> > 
> 
> Thanks. Yes, I did and also compared the error state output 
> (intel_error_decode also still works). The only change is that the batch 
> / ring / HWSP sections now use the engine->name (e.g. they will use rcs0 
> instead of render, vcs0 instead of bsd, etc.).
> 
> I didn't see anything in IGT complaining (probably because IGT usually 
> reads dmesg and there we already use engine->name), but there must be 
> something else out there that will complain.

Hmm, we will need to fixup mesa/aubinator_error_decode to expect the new
name, as it is using them to map various registers from the error state.
-Chris