Quoting Tvrtko Ursulin (2020-11-05 09:31:07)
>
> On 04/11/2020 23:30, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-11-04 13:47:43)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>
> >> Instead of printing out the internal engine mask, which can change between
> >> kernel versions making it difficult to map to actual engines, present a
> >> bitmask of hanging engines ABI classes. For example:
> >>
> >> [drm] GPU HANG: ecode 9:24dffffd:8, in gem_exec_schedu [1334]
> >>
> >> Notice the swapped the order of ecode and bitmask which makes the new
> >> versus old bug reports are obvious.
> >>
> >> Engine ABI class is useful to quickly categorize render vs media etc hangs
> >> in bug reports. Considering virtual engine even more so than the current
> >> scheme.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++++++------
> >> 1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> index 857db66cc4a3..e7d9af184d58 100644
> >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> @@ -1659,17 +1659,16 @@ static u32 generate_ecode(const struct intel_engine_coredump *ee)
> >> static const char *error_msg(struct i915_gpu_coredump *error)
> >> {
> >> struct intel_engine_coredump *first = NULL;
> >> + unsigned int hung_classes = 0;
> >> struct intel_gt_coredump *gt;
> >> - intel_engine_mask_t engines;
> >> int len;
> >>
> >> - engines = 0;
> >> for (gt = error->gt; gt; gt = gt->next) {
> >> struct intel_engine_coredump *cs;
> >>
> >> for (cs = gt->engine; cs; cs = cs->next) {
> >> if (cs->hung) {
> >> - engines |= cs->engine->mask;
> >> + hung_classes |= BIT(cs->engine->uabi_class);
> >
> > Your argument makes sense.
> >
> >> if (!first)
> >> first = cs;
> >> }
> >> @@ -1677,9 +1676,11 @@ static const char *error_msg(struct i915_gpu_coredump *error)
> >> }
> >>
> >> len = scnprintf(error->error_msg, sizeof(error->error_msg),
> >> - "GPU HANG: ecode %d:%x:%08x",
> >> - INTEL_GEN(error->i915), engines,
> >> - generate_ecode(first));
> >> + "GPU HANG: ecode %d:%08x:%x",
> >> + INTEL_GEN(error->i915),
> >> + generate_ecode(first),
> >> + hung_classes);
> >
> > I vote for keeping gen:engines:ecode order, for me that is biggest to
> > smallest.
>
> It would not be obvious what kind of bits are in the mask then, say
> looking from the ecode in maybe bugzilla titles and two different bugs
> may be incorrectly marked as duplicate.
No one should be marking bugs as duplicate based on this string. It
really is that useless.
> Maybe instead of the order we
> could change the separator(s)? Or add prefix/suffix to the mask?
I don't see the point; we've changed the construction several times.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
