On 17/01/18 07:15, Chris Wilson wrote:
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.
Thanks for taking care of aubinator, hopefully next time I won't forget
about mesa.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
