| Message ID | 20180110012151.28261-1-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/9/2018 5:21 PM, 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). > Hi, Fi.CI.IGT didn't catch any failure, apart from intel_error_decode (which didn't complain), I'm not sure which other tool could be using these names. And now that there are icl patches, this will help there too. > Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > 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)); > } > > static 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); > -- > 2.15.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > 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. 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: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > > static 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); >
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@intel.com> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> 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. > > 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@intel.com> > > Regards, > > Tvrtko > >> static 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); >>
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@intel.com> > >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> > >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> --- > >> 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
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@intel.com> >>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> --- >>>> 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.
Quoting Michel Thierry (2018-01-18 00:39:55) > On 17/01/18 07:15, Chris Wilson wrote: > > 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. Would be good if you could review my string handling there to make sure it does match. :) -Chris
Quoting Michel Thierry (2018-01-10 01:21:51) > 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@intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Massaged the patch just ever so slightly and applied. Thanks for the patch and review, -Chris
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)); } static 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);
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@intel.com> Signed-off-by: Michel Thierry <michel.thierry@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gpu_error.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)