| Message ID | 20180109193355.35538-1-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry <michel.thierry@intel.com> wrote: > Instead of using local string names that we will have to keep > maintaining, use the engine->name directly. > > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++------------- > 1 file changed, 10 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..db95ecacdace 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -34,16 +34,12 @@ > #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 const char *engine_str(struct drm_i915_private *i915, int > engine_id) > +{ > + if (!i915->engine[engine_id]) > + return ""; While unlikely, empty string may be misleading, so maybe better we return "<invalid>" or at least "?" here. Also maybe we can do as part of more global helper function like static inline const char* intel_engine_name(struct intel_engine_cs *engine) { return engine ? engine->name : "<invalid>"; } static inline struct intel_engine_cs * intel_engine_lookup(struct drm_i915_private *i915, int engine_id) { if (engine_id < 0 || engine_id > I915_MAX_ENGINES) return NULL; return i915->engine[engine_id]; } and then static const char *engine_str(struct drm_i915_private *i915, int engine_id) { return intel_engine_name(intel_engine_lookup(i915, engine_id)); } > + else > + return i915->engine[engine_id]->name; > } > static const char *tiling_flag(int tiling) > @@ -345,7 +341,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 ? " " : ""); Hmm, it looks that -1 is allowed/expected as engine_id so proposed above check in lookup function is mandatory > - 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 +413,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 +630,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);
Quoting Michal Wajdeczko (2018-01-09 20:39:09) > On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry > <michel.thierry@intel.com> wrote: > > > Instead of using local string names that we will have to keep > > maintaining, use the engine->name directly. > > > > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> I'd a patch to do this, I just had to make sure the tooling was ready for a name change. At the time, changing this string broke a few igt and intel_error_decode. > > --- > > drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++------------- > > 1 file changed, 10 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..db95ecacdace 100644 > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > @@ -34,16 +34,12 @@ > > #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 const char *engine_str(struct drm_i915_private *i915, int > > engine_id) > > +{ > > + if (!i915->engine[engine_id]) > > + return ""; > > While unlikely, empty string may be misleading, so maybe better we return > "<invalid>" or at least "?" here. Also maybe we can do as part of more > global helper function like > > static inline const char* intel_engine_name(struct intel_engine_cs *engine) > { > return engine ? engine->name : "<invalid>"; > } > > static inline struct intel_engine_cs * > intel_engine_lookup(struct drm_i915_private *i915, int engine_id) > { > if (engine_id < 0 || engine_id > I915_MAX_ENGINES) > return NULL; > return i915->engine[engine_id]; > } > > and then > > static const char *engine_str(struct drm_i915_private *i915, int engine_id) > { > return intel_engine_name(intel_engine_lookup(i915, engine_id)); > } No need, this is all internal, perma-allocated structs. -Chris
On 09/01/18 12:46, Chris Wilson wrote: > Quoting Michal Wajdeczko (2018-01-09 20:39:09) >> On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry >> <michel.thierry@intel.com> wrote: >> >>> Instead of using local string names that we will have to keep >>> maintaining, use the engine->name directly. >>> >>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > I'd a patch to do this, I just had to make sure the tooling was ready > for a name change. At the time, changing this string broke a few igt and > intel_error_decode. > I think your igt patch "lib: Fix up internal engine names (again)" covered it. Not sure if there are more tools looking at the engines' registers in the error_state outside igt. >>> --- >>> drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++------------- >>> 1 file changed, 10 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..db95ecacdace 100644 >>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >>> @@ -34,16 +34,12 @@ >>> #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 const char *engine_str(struct drm_i915_private *i915, int >>> engine_id) >>> +{ >>> + if (!i915->engine[engine_id]) >>> + return ""; >> >> While unlikely, empty string may be misleading, so maybe better we return >> "<invalid>" or at least "?" here. Also maybe we can do as part of more >> global helper function like >> >> static inline const char* intel_engine_name(struct intel_engine_cs *engine) >> { >> return engine ? engine->name : "<invalid>"; >> } >> >> static inline struct intel_engine_cs * >> intel_engine_lookup(struct drm_i915_private *i915, int engine_id) >> { >> if (engine_id < 0 || engine_id > I915_MAX_ENGINES) >> return NULL; >> return i915->engine[engine_id]; >> } >> >> and then >> >> static const char *engine_str(struct drm_i915_private *i915, int engine_id) >> { >> return intel_engine_name(intel_engine_lookup(i915, engine_id)); >> } > > No need, this is all internal, perma-allocated structs. > -Chris >
On 09/01/18 13:37, Michel Thierry wrote: > On 09/01/18 12:46, Chris Wilson wrote: >> Quoting Michal Wajdeczko (2018-01-09 20:39:09) >>> On Tue, 09 Jan 2018 20:33:55 +0100, Michel Thierry >>> <michel.thierry@intel.com> wrote: >>> >>>> Instead of using local string names that we will have to keep >>>> maintaining, use the engine->name directly. >>>> >>>> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> I'd a patch to do this, I just had to make sure the tooling was ready >> for a name change. At the time, changing this string broke a few igt and >> intel_error_decode. >> > > I think your igt patch "lib: Fix up internal engine names (again)" > covered it. > > Not sure if there are more tools looking at the engines' registers in > the error_state outside igt. > >>>> --- >>>> drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++------------- >>>> 1 file changed, 10 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..db95ecacdace 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c >>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c >>>> @@ -34,16 +34,12 @@ >>>> #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 const char *engine_str(struct drm_i915_private *i915, int >>>> engine_id) >>>> +{ >>>> + if (!i915->engine[engine_id]) >>>> + return ""; >>> >>> While unlikely, empty string may be misleading, so maybe better we >>> return >>> "<invalid>" or at least "?" here. Also maybe we can do as part of more >>> global helper function like >>> >>> static inline const char* intel_engine_name(struct intel_engine_cs >>> *engine) >>> { >>> return engine ? engine->name : "<invalid>"; >>> } >>> >>> static inline struct intel_engine_cs * >>> intel_engine_lookup(struct drm_i915_private *i915, int engine_id) >>> { >>> if (engine_id < 0 || engine_id > I915_MAX_ENGINES) >>> return NULL; >>> return i915->engine[engine_id]; >>> } >>> >>> and then >>> >>> static const char *engine_str(struct drm_i915_private *i915, int >>> engine_id) >>> { >>> return intel_engine_name(intel_engine_lookup(i915, engine_id)); >>> } >> >> No need, this is all internal, perma-allocated structs. i915_gpu_info_open can pass engine_id=-1, better have some protection. E.g. cat /sys/kernel/debug/dri/0/i915_gpu_info (while idle) [ 84.448504] GEM_BUG_ON(engine_id == -1) [ 84.448591] ------------[ cut here ]------------ [ 84.448630] kernel BUG at drivers/gpu/drm/i915/i915_gpu_error.c:39! [ 84.448673] invalid opcode: 0000 [#1] SMP [ 84.448696] Modules linked in: rfcomm bnep nls_iso8859_1 ... [ 84.449425] Call Trace: [ 84.449463] print_error_buffers+0x166/0x210 [i915] [ 84.449510] i915_error_state_to_str+0x923/0x1210 [i915] [ 84.449541] ? create_object+0x23f/0x2e0 [ 84.449584] ? i915_error_state_buf_init+0x59/0xc0 [i915] [ 84.449613] ? rcu_read_lock_sched_held+0x49/0x90 [ 84.449644] ? kmalloc_order_trace+0x10d/0x170 [ 84.449685] gpu_state_read+0x56/0xb0 [i915] [ 84.449712] full_proxy_read+0x4d/0x70 [ 84.449736] __vfs_read+0x23/0x120 [ 84.449759] vfs_read+0xa0/0x140 [ 84.449779] SyS_read+0x45/0xa0 [ 84.449801] entry_SYSCALL_64_fastpath+0x23/0x9a [ 84.449834] RIP: 0033:0x7f2a2d3d4230 [ 84.449856] RSP: 002b:00007ffda36960e8 EFLAGS: 00000246 [ 84.449861] Code: 15 83 fe 04 7f 27 48 63 f6 48 8b 84 f7 08 46 00 00 48 83 c0 08 c3 48 c7 c6 e3 03 59 c0 48 c7 c7 f3 03 59 c0 31 c0 e8 66 75 b9 c6 <0f> 0b 48 c7 c6 05 04 59 c0 48 c7 c7 f3 03 59 c0 31 c0 e8 4f 75 [ 84.450028] RIP: engine_str+0x34/0x50 [i915] RSP: ffffb7e142b53c98 [ 89.410577] ---[ end trace 5eacc9ca2b6d1e5d ]---
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 94499c24f279..db95ecacdace 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -34,16 +34,12 @@ #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 const char *engine_str(struct drm_i915_private *i915, int engine_id) +{ + if (!i915->engine[engine_id]) + return ""; + else + return i915->engine[engine_id]->name; } static const char *tiling_flag(int tiling) @@ -345,7 +341,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 +413,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 +630,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);
Instead of using local string names that we will have to keep maintaining, use the engine->name directly. Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Michel Thierry <michel.thierry@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/i915_gpu_error.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)