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@xxxxxxxxx>
> >> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> >> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
