AMD General
Hi Christian, Felix,
Thanks, both points taken. I've dropped this approach and sent a v4 that does
what you suggested
("[PATCH v4] drm/amdkfd: discard interrupts during a GPU reset"):
- Christian: no queue unmap once the reset has started. v4 doesn't touch
queues, DQM or the reset ordering, and sends nothing to the GPU.
- Felix: ignore interrupts during reset. kgd2kfd_interrupt() returns early
while amdgpu_in_reset().
- Felix: drain or discard pending IRQs? Discard. The KFD IH worker drops
entries while in reset, and kgd2kfd_pre_reset() flushes the KFD IH workqueue
after suspend, so nothing queued before the reset is processed after it.
- Felix: the process-close drain fence I raised earlier is not an issue. Only a
debugger trap disable or detach sends it, and during a reset that wait ends
after HZ/2 with a ratelimited warning.
Tested on MI300X, four processes under load plus amd-smi reset, stock and v4 on
the same box:
- Stock: 2/5 resets hit the original Oops (munmap -> free_swap_cache+0x5) right
after SDMA1 VM faults during the reset.
- v4: 0/18 Oops and no KFD ih_fifo overflow, including 4 runs with the same
SDMA1 faults. In ftrace, KFD handled 0 IH entries inside the reset window.
- No regressions: unload/reload 10/10, rocgdb check and detach clean, kfdtest
Evict/QM/Event 40/40. Reset latency is unchanged (8542 vs 8540 ms median).
Regards,
William
----------------------------------------------------------------------------------------------------------------------------------
-----Original Message-----
From: Koenig, Christian <[email protected]>
Sent: Monday, October 12, 2026 3:43 AM
To: Palacek, William <[email protected]>; Kuehling, Felix
<[email protected]>; [email protected]
Cc: Russell, Kent <[email protected]>; Kasiviswanathan, Harish
<[email protected]>
Subject: Re: [PATCH v2] drm/amdgpu: preempt KFD queues before taking the reset
domain lock
Hi William,
yeah Felix pointed it out already.
When we device to reset the ASIC this harsch the GPU is already completely
gone, partial recovery by unmapping the KFD queues is not possible any more at
that point.
So clear NAK to the whole approach.
Regards,
Christian.
On 10/10/26 18:27, Palacek, William wrote:
> AMD General
>
> Hi Felix,
>
> Thanks for the review. v3 addresses these:
>
> - Interrupts during reset: with v3, KFD pre-reset runs before the reset
> domain is locked, so the unmap is sent and waited for while the GPU can
> still respond.
> Filtering interrupts for a half-broken GPU is a separate change; KFD also
> needs the fence interrupt for process-close draining, so I'd rather do that
> as a follow-up than fold it in here.
> - amdgpu_amdkfd_pre_reset() now takes the device list; the extra wrapper is
> gone.
> - pci_channel_state: you're right, it is only set in
> amdgpu_pci_error_detected(). kfd_dqm_can_reach_hw() is dropped, so no new
> "is the GPU usable" condition is added.
> - unmap_queues_cpsch() now returns -EIO when the HWS is known to be hung.
> The MES check is removed.
>
> Regards,
> William
>
> ----------------------------------------------------------------------
> ------------------------------------------------------------
> 1 Commerce Valley Drive, Markham, ON L3T 7X6 LinkedIn | Instagram |
> X | amd.com
>
>
> -----Original Message-----
> From: Kuehling, Felix <[email protected]>
> Sent: Thursday, October 8, 2026 11:58 AM
> To: Palacek, William <[email protected]>;
> [email protected]; Koenig, Christian
> <[email protected]>
> Cc: Russell, Kent <[email protected]>; Kasiviswanathan, Harish
> <[email protected]>
> Subject: Re: [PATCH v2] drm/amdgpu: preempt KFD queues before taking
> the reset domain lock
>
> [+Christian for comments on reset architecture]
>
> On 2026-10-08 11:46, Kuehling, Felix wrote:
>>
>> On 2026-10-08 10:00, William Palacek wrote:
>>> A GPU reset issued while processes are using the device can Oops the
>>> host in munmap() (free_swap_cache) after SDMA write faults from a
>>> user queue whose process has already exited.
>>>
>>> amdgpu_device_gpu_recover() takes reset_domain->sem for write before
>>> amdgpu_amdkfd_pre_reset(), so the down_read_trylock() in
>>> unmap_queues_cpsch() and execute_queues_cpsch() fails with -EIO and
>>> no unmap is sent. stop_cpsch() still clears sched_running, so user
>>> queues keep running while their memory is freed.
>>
>> There is a general assumption that the GPU is stuck when we're in a
>> GPU reset. It seems in your scenario, the GPU is not entirely stuck.
>> So you can still get interrupts from the GPU. At the same time, you
>> can't rely on MES or HWS to unmap queues when the GPU is in a
>> half-broken state. So while your patch may help in this special case,
>> there could be other cases where you can't reliably stop the GPU from
>> sending spurious interrupts while a reset is in progress.
>>
>> Maybe we need something in the interrupt handler to ignore interrupts
>> while a GPU reset is in progress.
>
> It's also possible that there are still interrupts pending in the IH ring
> when a reset starts that won't get processed until later. Do we drain or
> discard pending IRQs before starting a reset?
>
> Regards,
> Felix
>
>
>>
>>
>>>
>>> Call amdgpu_amdkfd_pre_reset() before the lock, for gpu_recover and
>>> PCI error handling. Skip the dequeue when the HWS is hung or the PCI
>>> link is down, so recovery does not wait out the preemption timeout.
>>>
>>> Signed-off-by: William Palacek <[email protected]>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - Drop the SRC_USER gate and the helper; run for every reset
>>> source
>>> - Cover amdgpu_pci_error_detected() as well as gpu_recover()
>>> - Skip the dequeue when the HWS is hung or the PCI link is down
>>>
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16
>>> ++++++++++++++--
>>> .../drm/amd/amdkfd/kfd_device_queue_manager.c | 14
>>> ++++++++++++++
>>> 2 files changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index e0014611f039..8ae6228dbf31 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -5479,6 +5479,16 @@ static void
>>> amdgpu_device_recovery_prepare(struct amdgpu_device *adev,
>>> }
>>> }
>>> +/* Dequeueing needs reset_domain->sem for read, so run before it
>>> is write-locked */
>>> +static void amdgpu_device_kfd_pre_reset(struct amdgpu_reset_context
>>> *reset_context,
>>> + struct list_head *device_list)
>>
>> Instead of adding a new function with a confusingly similar name,
>> should we change the signature of amdgpu_amdkfd_pre_reset to accept a
>> device-list instead?
>>
>>
>>> +{
>>> + struct amdgpu_device *tmp_adev;
>>> +
>>> + list_for_each_entry(tmp_adev, device_list, reset_list)
>>> + amdgpu_amdkfd_pre_reset(tmp_adev, reset_context); }
>>> +
>>> static void amdgpu_device_recovery_get_reset_lock(struct
>>> amdgpu_device *adev,
>>> struct list_head *device_list)
>>> {
>>> @@ -5534,8 +5544,6 @@ static void
>>> amdgpu_device_halt_activities(struct amdgpu_device *adev,
>>> cancel_delayed_work_sync(&tmp_adev->delayed_init_work);
>>> - amdgpu_amdkfd_pre_reset(tmp_adev, reset_context);
>>> -
>>> /*
>>> * Mark these ASICs to be reset as untracked first
>>> * And add them back after reset completed @@ -5784,6
>>> +5792,9 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
>>> /* Cannot be called after locking reset domain */
>>> amdgpu_ras_pre_reset(adev, &device_list);
>>> + /* Cannot be called after locking reset domain either */
>>> + amdgpu_device_kfd_pre_reset(reset_context, &device_list);
>>> +
>>> /* We need to lock reset domain only once both for XGMI and
>>> single device */
>>> amdgpu_device_recovery_get_reset_lock(adev, &device_list);
>>> @@ -6232,6 +6243,7 @@ pci_ers_result_t
>>> amdgpu_pci_error_detected(struct pci_dev *pdev, pci_channel_sta
>>> INIT_LIST_HEAD(&device_list);
>>> amdgpu_device_recovery_prepare(adev, &device_list,
>>> hive);
>>> + amdgpu_device_kfd_pre_reset(&reset_context, &device_list);
>>> amdgpu_device_recovery_get_reset_lock(adev, &device_list);
>>> amdgpu_device_halt_activities(adev, NULL, &reset_context,
>>> &device_list,
>>> hive, false); diff --git
>>> a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> index 5e57576fba21..988fce11a4fb 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>> @@ -81,6 +81,16 @@ static void set_queue_as_reset(struct
>>> device_queue_manager *dqm, struct queue *q
>>> struct qcm_process_device *qpd, bool verbose);
>>> static int reset_queues_mes(struct device_queue_manager *dqm,
>>> struct queue *q);
>>> +/*
>>> + * A dead HWS or an offline PCI link cannot service a dequeue
>>> request. Skip it
>>> + * rather than wait out queue_preemption_timeout_ms per node.
>>> + */
>>> +static inline bool kfd_dqm_can_reach_hw(struct device_queue_manager
>>> *dqm)
>>> +{
>>> + return !dqm->is_hws_hang &&
>>> + dqm->dev->adev->pci_channel_state ==
>>> +pci_channel_io_normal;
>>
>> Is pci_channel_state always valid? I notice that
>> pci_channel_io_normal is 1. pci_channel_state is only set in
>> amdgpu_pci_error_detected. If that never gets called,
>> pci_channel_state is probably 0-initialized and this condition will never be
>> true.
>>
>> I'm a bit cautious about spreading different versions of "is the GPU
>> usable" throughout the code. There are lots of things you could look
>> at. I've seen different conditions related to GPU hot-unplug in
>> another patch I'm reviewing. These kinds of conditions should be
>> managed carefully, otherwise we end up with subtly different or
>> broken conditions all over the place and it will be a mess to fix
>> later. Or the the code becomes unmaintainable if the conditions need
>> to change, because now you have to change it in 15 different places.
>>
>>
>>> +}
>>> +
>>> static inline
>>> enum KFD_MQD_TYPE get_mqd_type_from_queue_type(enum kfd_queue_type
>>> type)
>>> {
>>> @@ -314,6 +324,8 @@ static int
>>> remove_queue_mes_on_reset_option(struct device_queue_manager *dqm, st
>>> if (!dqm->sched_running || dqm->sched_halt)
>>> return 0;
>>> + if (!kfd_dqm_can_reach_hw(dqm))
>>> + return 0;
>>
>> Why does this return 0 and not -EIO? Isn't this logically the same as
>> the next condition? We don't talk to MES because the GPU is hanging
>> and a reset in progress.
>>
>>
>>> if (!down_read_trylock(&adev->reset_domain->sem))
>>> return -EIO;
>>> @@ -2832,6 +2844,8 @@ static int unmap_queues_cpsch(struct
>>> device_queue_manager *dqm,
>>> if (!dqm->sched_running)
>>> return 0;
>>> + if (!kfd_dqm_can_reach_hw(dqm))
>>> + return 0;
>>
>> Same question as above.
>>
>> Regards,
>> Felix
>>
>>
>>> if (!dqm->active_runlist)
>>> return 0;
>>> if (!down_read_trylock(&dqm->dev->adev->reset_domain->sem))
