Received-SPF: pass (google.com: domain of linux-kernel+bounces-92424-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <85027e94-9741-4bf8-ba5c-2937fc27d2d3@amd.com>
Date: Tue, 5 Mar 2024 14:20:35 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] drm/amdgpu: add ring timeout information in
 devcoredump
Content-Language: en-US
To: "Khatri, Sunil" <sukhatri@amd.com>, Sunil Khatri <sunil.khatri@amd.com>,
 Alex Deucher <alexander.deucher@amd.com>,
 Shashank Sharma <shashank.sharma@amd.com>
Cc: amd-gfx@lists.freedesktop.org, Pan@rtg-sunil-navi33.amd.com,
 Xinhui <Xinhui.Pan@amd.com>, dri-devel@lists.freedesktop.org,
 linux-kernel@vger.kernel.org
References: <20240305115843.3119708-1-sunil.khatri@amd.com>
 <209b7e69-594d-4e40-8173-42643f94f2ba@amd.com>
 <d517ae14-4668-4782-9a96-d5482ca5c81d@amd.com>
From: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <d517ae14-4668-4782-9a96-d5482ca5c81d@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TTVFYmFsK2duaG1UUkQvdWphZ0N4QThrVlNscmVIWG1PbllVcndlVFdyY3Fu?=
 =?utf-8?B?cmVvaVlqWUxVbHMxY0tWZWtlMmNNTmFTMnFnNGI5c1lhWTJRSzFtbkUySm9H?=
 =?utf-8?B?U0Y1a1orcjkwM2ljaUtVMWhzSk9yWTIwVXMyRm55akdmemNPcUN3Qkwrc292?=
 =?utf-8?B?czR6Zkl3TnN0ekc5UHZnL0NGZzI5bXRHaS9hZGFuQ2NJb3Q2VTB1UThLWUMx?=
 =?utf-8?B?UHppUnNDVzJ2Q3dYU1V2MFVCUmRvZU1LcWpHT1VaSk96QjRtZ1YrVGhMb3pa?=
 =?utf-8?B?QllpRWp3RXphQUN2UXhJUG4ramFUY21pVDdHZEJ4QmhMRXowSXBEYjRaZWZK?=
 =?utf-8?B?R0JnOHdxV0ZNWEpQUVVPM3Z0TjdlcElCTFJ3VzliVHVYY2xlMzY2ak1ra2di?=
 =?utf-8?B?SnJzVXJJa2phbDJiWlg0Q3YxUmFEWDNFUEc0UlBXUzByZ0lmSHlPWkM1QlVO?=
 =?utf-8?B?Y0orS0dKdEloMXM2M3hXSk5yTWR5SEhMUlBHWFArTWM4dUIvWXhiVVlzT2ZM?=
 =?utf-8?B?TDAzWVROTmxWUTdVNG5QazhYMU9nb3RtamhyMWhMSU54ODIyNGkzZGpxUGtX?=
 =?utf-8?B?dnRPM3JRZkJ5ZzAwUVZSVWZPVmllWUhTQ0NtTFYrQnhlTE4yNHBodXYyR0F6?=
 =?utf-8?B?Mko0NjVmdi9Sc0NCRGNQeHdsZHc1Sk1YZU9ieWR6NnloTmdwS0FuYmlIOS9B?=
 =?utf-8?B?TzRvOW5GZmI3LzZjYVEzbTNacmJpczJHZUo1a3NXVVkzS3phRDdsSm9zck1i?=
 =?utf-8?B?eWdoZnhZQnhJQVNCZkx4WEFOSFJueVBJdDNreHBXVVRReXBNdkdZUFZlakRm?=
 =?utf-8?B?Rk1obFFQZUxXYjVuRUVFUVdjSFk1alIwbEhVWkhhMkhHdDVyRllqWkh1c21l?=
 =?utf-8?B?dGlINHpZSFh1VVExNEtWd3ZYRzAxWm9Uc0ZEVlB0VnpUeHptZU9vcGwwSXRz?=
 =?utf-8?B?dFl3WVNJNkNjcHkrczVwTEZ0bWh2cnFlR0NCQmV1bjRISzVjeDFMQ1FtOWd3?=
 =?utf-8?B?Vkh4dlM4VW9Na0xYUzJmOGQ2aHhUb1VNVExxeFJzbGpqY1NvTlVFdkZ6QXJM?=
 =?utf-8?B?MjN6TXV3UGpmU2lERXZiMlMyQ2tkWjE0ZWxzT0VqbVZvTlcvdmlMQTQ0V0Vz?=
 =?utf-8?B?elc2QUorSVNrZ3JGRmxCVUhvNys0cDdCU3ZXVkZRWTR6aG1VZlpPSWl3WmFS?=
 =?utf-8?B?d1IrTzhUaTFkZDJBMHVuSFlXNUNJNjNuVUZyeWpyQmNoSmROSkdUeEtBcGhF?=
 =?utf-8?B?SitpMEhGR3VUYXhvVTRBQ3VsMGdPNnF2QXl3a2N2SGhIL08rL1ZxbGhXbzlY?=
 =?utf-8?B?SXh2NEFYWG4ySVF1Ryt3VE9tNUY4TFhzSmFGU3hTdHUxaDNBWXd0QmpMSk5S?=
 =?utf-8?B?RTU1SXBUdlVQVW12Rmhrc3l2OHRMQzZ4cTVEYjFzbXZqZDJ2NW5XUUFhRG9V?=
 =?utf-8?B?V3JnbmQ0WVVmYUFuUHFRVklhYUlwYWM3UW9xQlF1SDVra0MxSGprTkJnOGR0?=
 =?utf-8?B?VXNGMjR6YUk2cDl6endiNmFiK3VEUUZORWZvNE9DL2NQVHdOQlB1SzB4WXVR?=
 =?utf-8?B?cVV3N3VxVDVZaVVOWjlIRUZIUU9UQnFUVWM1OERGeXQzWE9mRmJPOFJMUVhC?=
 =?utf-8?B?TFJjZGRQd0d0V0dFSlNTMTVoNWtLUzlMeDFvME5RWGptUDJZWDVLbmUyL043?=
 =?utf-8?B?ek5PclA1cGJrMkw3dUgwOU9VK0ZwVVJXL1ZySENhYzNuUUFEWWloTnFwdkU3?=
 =?utf-8?B?clg4R084U25FWVNWL0hxc3MrU3dFSC96ZURFQTY4VUJXbEpTMXlHaXBDMjRX?=
 =?utf-8?B?WnpMRThqSUJqd3JlL2U3UXYyQS9BbDN1am4ybm5HRnV6aFlXckFWMmNEcCsz?=
 =?utf-8?B?cHBueU1INFY4djFncGpmb3FwamFSdS85VFVDRWtTY1p0K1FhbDRoYnVmS0I4?=
 =?utf-8?B?NzRUN1Bsa2JWU1Q5UG9pZ2gvbHlrcDBFWTU1NHE5Z3JCN1NsbHA0bHVIWVRj?=
 =?utf-8?B?cVJpV3g3a2xJRndRWVVIZVZYTEh6ZTk5QmhtNkFmcXh0TzR4bFMrQ0ZpK21z?=
 =?utf-8?B?RFFFRjVPK2V6SlppdGp3Q2cxSmd5aUlWdS9qM3dCUWhpaWZoNnVQT3BjVGZW?=
 =?utf-8?Q?X5vJxZsRt9g491ompfLBF/cRR?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bcaf613-d285-49a4-ef8d-08dc3d170e1c
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5685.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2024 13:20:41.1185
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hkwZP36W0Cvh9m12u+QdPD74aWgHa19wcFplImn3CBmJBh1UlYLzSwOXTUVVRQdW
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7620

Am 05.03.24 um 14:17 schrieb Khatri, Sunil:
>
> On 3/5/2024 6:40 PM, Christian König wrote:
>> Am 05.03.24 um 12:58 schrieb Sunil Khatri:
>>> Add ring timeout related information in the amdgpu
>>> devcoredump file for debugging purposes.
>>>
>>> During the gpu recovery process the registered call
>>> is triggered and add the debug information in data
>>> file created by devcoredump framework under the
>>> directory /sys/class/devcoredump/devcdx/
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 15 +++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h |  2 ++
>>>   2 files changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> index a59364e9b6ed..aa7fed59a0d5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
>>> @@ -196,6 +196,13 @@ amdgpu_devcoredump_read(char *buffer, loff_t 
>>> offset, size_t count,
>>>                  coredump->reset_task_info.process_name,
>>>                  coredump->reset_task_info.pid);
>>>   +    if (coredump->ring_timeout) {
>>> +        drm_printf(&p, "\nRing timed out details\n");
>>> +        drm_printf(&p, "IP Type: %d Ring Name: %s \n",
>>> +                coredump->ring->funcs->type,
>>> +                coredump->ring->name);
>>> +    }
>>> +
>>>       if (coredump->reset_vram_lost)
>>>           drm_printf(&p, "VRAM is lost due to GPU reset!\n");
>>>       if (coredump->adev->reset_info.num_regs) {
>>> @@ -220,6 +227,8 @@ void amdgpu_coredump(struct amdgpu_device *adev, 
>>> bool vram_lost,
>>>   {
>>>       struct amdgpu_coredump_info *coredump;
>>>       struct drm_device *dev = adev_to_drm(adev);
>>> +    struct amdgpu_job *job = reset_context->job;
>>> +    struct drm_sched_job *s_job;
>>>         coredump = kzalloc(sizeof(*coredump), GFP_NOWAIT);
>>>   @@ -228,6 +237,12 @@ void amdgpu_coredump(struct amdgpu_device 
>>> *adev, bool vram_lost,
>>>           return;
>>>       }
>>>   +    if (job) {
>>> +        s_job = &job->base;
>>> +        coredump->ring = to_amdgpu_ring(s_job->sched);
>>> +        coredump->ring_timeout = TRUE;
>>> +    }
>>> +
>>>       coredump->reset_vram_lost = vram_lost;
>>>         if (reset_context->job && reset_context->job->vm) {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
>>> index 19899f6b9b2b..6d67001a1057 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.h
>>> @@ -97,6 +97,8 @@ struct amdgpu_coredump_info {
>>>       struct amdgpu_task_info         reset_task_info;
>>>       struct timespec64               reset_time;
>>>       bool                            reset_vram_lost;
>>> +    struct amdgpu_ring          *ring;
>>> +    bool                            ring_timeout;
>>
>> I think you can drop ring_timeout, just having ring as optional 
>> information should be enough.
>>
>> Apart from that looks pretty good I think.
>>
> - GPU reset could happen due to two possibilities atleast: 1. via 
> sysfs cat /sys/kernel/debug/dri/0/amdgpu_gpu_recover there is no 
> timeout or page fault here. In this case we need information if 
> ringtimeout happened or not else it will try to print empty 
> information in devcoredump. Same goes for pagefault also in that case 
> also we need to see if recovery ran due to pagefault and then only add 
> that information.
>
> So to cover all use cases i added this parameter.

Yeah, but you don't need that parameter.

If a GPU reset is triggered by a ring timeout we note the ring causing 
this, otherwise the ring is simply NULL.

There is no need for a separate variable to note that a timeout happened.

Regards,
Christian.

>
>
> Thanks
> Sunil
>
>> Regards,
>> Christian.
>>
>>>   };
>>>   #endif
>>