[Intel-gfx] [PATCH] drm/i915: Use the engine name directly in the error_state file

[Michel Thierry]

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 at 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 at intel.com>
>>>> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko at 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 ]---