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.cindex 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.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.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Regards, Tvrtkostatic const char *tiling_flag(int tiling)@@ -345,7 +351,7 @@ static void print_error_buffers(struct drm_i915_error_state_buf *m,err_puts(m, purgeable_flag(err->purgeable)); err_puts(m, err->userptr ? " userptr" : ""); err_puts(m, err->engine != -1 ? " " : ""); - err_puts(m, engine_str(err->engine)); + err_puts(m, engine_str(m->i915, err->engine)); err_puts(m, i915_cache_level_str(m->i915, err->cache_level)); if (err->name)@@ -417,7 +423,8 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,{ int n; - err_printf(m, "%s command stream:\n", engine_str(ee->engine_id)); + err_printf(m, "%s command stream:\n", engine_str(m->i915, + ee->engine_id)); err_printf(m, " IDLE?: %s\n", yesno(ee->idle)); err_printf(m, " START: 0x%08x\n", ee->start); err_printf(m, " HEAD: 0x%08x [0x%08x]\n", ee->head, ee->rq_head);@@ -633,7 +640,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,if (error->engine[i].hangcheck_stalled && error->engine[i].context.pid) {err_printf(m, "Active process (on ring %s): %s [%d], score %d\n",- engine_str(i), + engine_str(m->i915, i), error->engine[i].context.comm, error->engine[i].context.pid, error->engine[i].context.ban_score);
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
