[Intel-gfx] [PATCH] drm/i915/gt: Reduce log severity on reset prepare.

[Tvrtko Ursulin]

On 05/12/2023 10:44, Nirmoy Das wrote:
> Hi Tvrtko,
> 
> On 12/5/2023 11:05 AM, Tvrtko Ursulin wrote:
>>
>> On 05/12/2023 08:50, Nirmoy Das wrote:
>>> Hi Tvrtko,
>>>
>>> On 12/5/2023 9:34 AM, Tvrtko Ursulin wrote:
>>>>
>>>> On 01/12/2023 15:44, Nirmoy Das wrote:
>>>>> gen8_engine_reset_prepare() can fail when HW fails to set
>>>>> RESET_CTL_READY_TO_RESET bit. In some cases this is not fatal
>>>>> error as driver will retry.
>>>>>
>>>>> Let the caller of gen8_engine_reset_prepare() decide if a
>>>>> failure in gen8_engine_reset_prepare is an error or not.
>>>>
>>>> No complaints per se but I don't see the caller deciding and it is 
>>>> not really reducing log level but converting to trace. So commit 
>>>> message and patch do not align for me which I think should be improved.
>>>
>>>
>>> I meant the return value is checked by the caller, 
>>> gen8_reset_engines(). I will resend with a improved commit message.
>>
>> Ah okay, maybe my bad for not figuring out that possibility. I guess 
>> it might be passable as is, but yes, clearer commit text would be better.
> 
> I sent a v2 already :)
> 
> 
>>
>> Trace is good enough - we are not usually interested in seeing those 
>> as dbg/info/notice?
> 
> 
> Idea is that all the GT related events are recorded in trace and dmesg 
> could be noisy some times.

Although trace does not help on production deployments so we need to be 
sure the fact this timeout is hit is totally un-interesting. I see John 
has some concerns that it may not be so. And I don't have currently a 
view into how frequent they are (timeouts) or which platforms are affected.

Regards,

Tvrtko

> 
> 
> Regards,
> 
> Nirmoy
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Thanks,
>>>
>>> Nirmoy
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>> Cc: John Harrison <John.C.Harrison at Intel.com>
>>>>> Cc: Andi Shyti <andi.shyti at linux.intel.com>
>>>>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
>>>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5591
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/gt/intel_reset.c | 8 ++++----
>>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c 
>>>>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> index d5ed904f355d..e6fbc6202c80 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>> @@ -593,10 +593,10 @@ static int gen8_engine_reset_prepare(struct 
>>>>> intel_engine_cs *engine)
>>>>>       ret = __intel_wait_for_register_fw(uncore, reg, mask, ack,
>>>>>                          700, 0, NULL);
>>>>>       if (ret)
>>>>> -        gt_err(engine->gt,
>>>>> -               "%s reset request timed out: {request: %08x, 
>>>>> RESET_CTL: %08x}\n",
>>>>> -               engine->name, request,
>>>>> -               intel_uncore_read_fw(uncore, reg));
>>>>> +        GT_TRACE(engine->gt,
>>>>> +             "%s reset request timed out: {request: %08x, 
>>>>> RESET_CTL: %08x}\n",
>>>>> +             engine->name, request,
>>>>> +             intel_uncore_read_fw(uncore, reg));
>>>>>         return ret;
>>>>>   }