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

[Michel Thierry]

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