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))
