drm/i915: Use the engine name directly in the error_state file

Submitted by Michel Thierry on Jan. 9, 2018, 7:33 p.m.

Details

Message ID 20180109193355.35538-1-michel.thierry@intel.com
State New
Headers show
Series "drm/i915: Use the engine name directly in the error_state file" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Michel Thierry Jan. 9, 2018, 7:33 p.m.
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(-)

Patch hide | download patch | download mbox

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);

Comments

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 ]---