Re: [PATCH v2] drm/amdkfd: save/restore MQD across hibernation when MQD is in VRAM
"Kuehling, Felix" <[email protected]> Tue, 28 Jul 2026 14:57:53 -0400
| Newsgroups | org.freedesktop.lists.amd-gfx |
|---|---|
| Message-ID | <[email protected]> |
On 2026-07-27 23:10, Fan, Shikang wrote: > AMD General > > Hi @Kuehling, Felix <mailto:[email protected]>, could you help > take a look for other comments that you might have? I don't have other comments per se. Maybe another idea for saving and restoring MQD contents. If allocation during suspend is a problem, you could allocate the backup space in system memory during queue creation. But using the general eviction mechanism and dealing with changing MQD addresses is probably cleaner. Regards, Felix > > Regards, > Shikang > > > ------------------------------------------------------------------------ > *From:* Fan, Shikang <[email protected]> > *Sent:* Wednesday, July 22, 2026 11:22 AM > *To:* Limonciello, Mario <[email protected]>; Kuehling, Felix > <[email protected]>; Koenig, Christian > <[email protected]>; [email protected] > <[email protected]>; SHANMUGAM, SRINIVASAN > <[email protected]> > *Cc:* Yang, Philip <[email protected]>; Deucher, Alexander > <[email protected]>; Zhao, Victor <[email protected]>; > Zhang, Tiantian (Celine) <[email protected]> > *Subject:* Re: [PATCH v2] drm/amdkfd: save/restore MQD across > hibernation when MQD is in VRAM > > AMD General > > This is for virtual machine migration feature, especially under the > SR-IOV scenario. > > Regards, > Shikang > > ------------------------------------------------------------------------ > *From:* Limonciello, Mario <[email protected]> > *Sent:* Wednesday, July 22, 2026 5:21 AM > *To:* Kuehling, Felix <[email protected]>; Fan, Shikang > <[email protected]>; Koenig, Christian <[email protected]>; > [email protected] <[email protected]>; > SHANMUGAM, SRINIVASAN <[email protected]> > *Cc:* Yang, Philip <[email protected]>; Deucher, Alexander > <[email protected]>; Zhao, Victor <[email protected]>; > Zhang, Tiantian (Celine) <[email protected]> > *Subject:* Re: [PATCH v2] drm/amdkfd: save/restore MQD across > hibernation when MQD is in VRAM > > IIUC you need to go through these flows as part of guest migration > (which system goes into S4 to accomplish). > > > On 7/21/26 16:20, Felix Kuehling wrote: > > These are all server GPUs. Is system suspend really a requirement > here? Or is this for runtime suspend? But runtime suspend > shouldn't kick in while user mode queues exist. > > > Regards, > Felix > > > On 2026-07-21 03:20, Fan, Shikang wrote: > > AMD General > > +@Zhao, Victor <mailto:[email protected]>, @Zhang, Tiantian > (Celine) <mailto:[email protected]>. And also > @Limonciello, Mario <mailto:[email protected]> to help > with the review. > > Regards, > Shikang > > ------------------------------------------------------------------------ > *From:* Fan, Shikang <[email protected]> > <mailto:[email protected]> > *Sent:* Tuesday, July 21, 2026 11:25 AM > *To:* Koenig, Christian <[email protected]> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <[email protected]> > <mailto:[email protected]>; SHANMUGAM, SRINIVASAN > <[email protected]> > <mailto:[email protected]>; Kuehling, Felix > <[email protected]> <mailto:[email protected]> > *Cc:* Yang, Philip <[email protected]> > <mailto:[email protected]>; Deucher, Alexander > <[email protected]> <mailto:[email protected]> > *Subject:* Re: [PATCH v2] drm/amdkfd: save/restore MQD across > hibernation when MQD is in VRAM > > AMD General > > +@SHANMUGAM, SRINIVASAN > <mailto:[email protected]> and @Kuehling, Felix > <mailto:[email protected]> to the review discussion as > Christian is currently OOO. > > Regards, > Shikang > > ------------------------------------------------------------------------ > *From:* Fan, Shikang <[email protected]> > <mailto:[email protected]> > *Sent:* Friday, July 17, 2026 6:25 PM > *To:* Koenig, Christian <[email protected]> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <[email protected]> > <mailto:[email protected]> > *Cc:* Yang, Philip <[email protected]> > <mailto:[email protected]>; Deucher, Alexander > <[email protected]> > <mailto:[email protected]>; Kuehling, Felix > <[email protected]> <mailto:[email protected]> > *Subject:* Re: [PATCH v2] drm/amdkfd: save/restore MQD across > hibernation when MQD is in VRAM > > AMD General > > > I do have a few thoughts on the unpin-repin approach and > re-building the MQD from housekeeping information. > > For unpin-repin: the MQD's GPU address may change after it's > moved back from GTT, and several places hold the original > address, so we'd need to update all of them and be careful not > to miss any — but overall it's doable. > > For re-build: the one thing we can't reconstruct is the > control stack. It resides in the same BO as the MQD, and the > driver doesn't preserve its contents. > > Separately, I'd like to raise a potential pre-existing issue I > noticed while writing the patch. mqd_on_vram() covers gfx942, > gfx943, gfx944 and gfx950. gfx943/944/950 are all registered > with the _v9_4_3 implementations, which call flush_hdp() in > init_mqd, update_mqd and restore_mqd. gfx942, however, uses > the base MQD functions, which lack the flush_hdp() call. > > Regards, > Shikang > > ------------------------------------------------------------------------ > *From:* Koenig, Christian <[email protected]> > <mailto:[email protected]> > *Sent:* Friday, July 17, 2026 4:59 PM > *To:* Fan, Shikang <[email protected]> > <mailto:[email protected]>; [email protected] > <mailto:[email protected]> > <[email protected]> > <mailto:[email protected]> > *Cc:* Yang, Philip <[email protected]> > <mailto:[email protected]>; Deucher, Alexander > <[email protected]> > <mailto:[email protected]>; Kuehling, Felix > <[email protected]> <mailto:[email protected]> > *Subject:* Re: [PATCH v2] drm/amdkfd: save/restore MQD across > hibernation when MQD is in VRAM > > On 7/17/26 10:53, Shikang Fan wrote: > > On gfx942/944 and gfx9.5.0, KFD compute-queue MQDs live in a > pinned VRAM > > BO (mqd_on_vram()). Pinned VRAM is skipped by TTM eviction > and not saved > > across S4 hibernation, so the MQD is garbage on resume and > the first > > submission faults. Save it to a system-RAM shadow at suspend > and restore > > it on resume, reusing the CRIU checkpoint_mqd/restore_mqd > primitives. > > > > v2: > > - Also snapshot the MQD when a process is already > runtime-evicted at > > hibernation start. The qpd->evicted refcount early-return > previously > > skipped the save loop, losing the MQD across the VRAM > wipe (both the > > cpsch and nocpsch evict paths). > > > > Signed-off-by: Shikang Fan <[email protected]> > <mailto:[email protected]> > > --- > > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 120 > +++++++++++++++++- > > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 ++ > > drivers/gpu/drm/amd/amdkfd/kfd_queue.c | 2 + > > 3 files changed, 129 insertions(+), 2 deletions(-) > > > > 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 51ee9c39104b..d168686ee324 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > > @@ -65,6 +65,9 @@ static int map_queues_cpsch(struct > device_queue_manager *dqm); > > static void deallocate_sdma_queue(struct > device_queue_manager *dqm, > > struct queue *q); > > > > +static int dqm_alloc_mqd_backup(struct device_queue_manager > *dqm, struct queue *q); > > +static void dqm_save_mqd_backup(struct device_queue_manager > *dqm, struct queue *q); > > + > > static inline void deallocate_hqd(struct > device_queue_manager *dqm, > > struct queue *q); > > static int allocate_hqd(struct device_queue_manager *dqm, > struct queue *q); > > @@ -802,6 +805,10 @@ static int create_queue_nocpsch(struct > device_queue_manager *dqm, > > mqd_mgr->init_mqd(mqd_mgr, &q->mqd, > q->mqd_mem_obj, > > &q->gart_mqd_addr, &q->properties); > > > > + retval = dqm_alloc_mqd_backup(dqm, q); > > + if (retval) > > + goto out_free_mqd; > > + > > if (q->properties.is_active) { > > if (!dqm->sched_running) { > > WARN_ONCE(1, "Load non-HWS mqd while > stopped\n"); > > @@ -1263,8 +1270,16 @@ static int > evict_process_queues_nocpsch(struct device_queue_manager *dqm, > > int retval, ret = 0; > > > > dqm_lock(dqm); > > - if (qpd->evicted++ > 0) /* already evicted, do nothing */ > > + if (qpd->evicted++ > 0) { /* already evicted, do > nothing */ > > + /* A process already runtime-evicted when > hibernation starts > > + * skips the save loop below, so snapshot its > VRAM MQDs here > > + * before the S4 image is taken. > dqm_save_mqd_backup() is a > > + * no-op outside S4. > > + */ > > + list_for_each_entry(q, &qpd->queues_list, list) > > + dqm_save_mqd_backup(dqm, q); > > goto out; > > + } > > > > pdd = qpd_to_pdd(qpd); > > pr_debug_ratelimited("Evicting process pid %d queues\n", > > @@ -1297,6 +1312,8 @@ static int > evict_process_queues_nocpsch(struct device_queue_manager *dqm, > > * maintain a consistent eviction state > > */ > > ret = retval; > > + > > + dqm_save_mqd_backup(dqm, q); > > } > > > > out: > > @@ -1304,6 +1321,88 @@ static int > evict_process_queues_nocpsch(struct device_queue_manager *dqm, > > return ret; > > } > > > > +/* MQD software-shadow save/restore across S4 hibernation, > reusing the CRIU > > + * checkpoint_mqd/restore_mqd primitives. > > + */ > > +static int dqm_alloc_mqd_backup(struct device_queue_manager > *dqm, struct queue *q) > > +{ > > + struct mqd_manager *mqd_mgr; > > + uint32_t mqd_size, ctl_stack_size = 0; > > + > > + if (!mqd_on_vram(dqm->dev->adev)) > > + return 0; > > + if (q->properties.type != KFD_QUEUE_TYPE_COMPUTE) > > + return 0; > > + if (!q->mqd) > > + return 0; > > + > > + mqd_mgr = > dqm->mqd_mgrs[get_mqd_type_from_queue_type(q->properties.type)]; > > + if (!mqd_mgr->checkpoint_mqd || !mqd_mgr->restore_mqd) > > + return 0; > > + > > + mqd_size = AMDGPU_MQD_SIZE_ALIGN(mqd_mgr->mqd_size) * > > + NUM_XCC(dqm->dev->xcc_mask); > > + if (mqd_mgr->get_checkpoint_info) > > + mqd_mgr->get_checkpoint_info(mqd_mgr, q->mqd, > &ctl_stack_size); > > + > > + if (!q->mqd_backup) { > > + q->mqd_backup = kzalloc(mqd_size, GFP_KERNEL); > > Please note that you need to be super careful when allocating > memory during suspend. The amount available is really limited > because both swap and OOM killer are already stopped. > > Instead you should probably consider unpinning on suspend and > repinning on resume, for this we have memory put asside. > > Additional to that it could be that backing up the MQD is > overkill, we should be able to re-create the MQD after resume > from the housekeeping information we still have around. > > Regards, > Christian. > > > > + if (!q->mqd_backup) > > + return -ENOMEM; > > + q->mqd_backup_size = mqd_size; > > + } > > + if (ctl_stack_size && !q->ctl_stack_backup) { > > + q->ctl_stack_backup = kzalloc(ctl_stack_size, > GFP_KERNEL); > > + if (!q->ctl_stack_backup) { > > + kfree(q->mqd_backup); > > + q->mqd_backup = NULL; > > + q->mqd_backup_size = 0; > > + return -ENOMEM; > > + } > > + q->ctl_stack_backup_size = ctl_stack_size; > > + } > > + > > + return 0; > > +} > > + > > +static void dqm_save_mqd_backup(struct device_queue_manager > *dqm, struct queue *q) > > +{ > > + struct mqd_manager *mqd_mgr; > > + > > + /* in_s4 is reliably set at evict/suspend time; skip > normal runtime eviction > > + * (pinned VRAM MQD stays intact then). > > + */ > > + if (!dqm->dev->adev->in_s4) > > + return; > > + if (!q->mqd_backup) > > + return; > > + if (!q->mqd || !q->mqd_mem_obj || > !q->mqd_mem_obj->cpu_ptr) > > + return; > > + > > + mqd_mgr = > dqm->mqd_mgrs[get_mqd_type_from_queue_type(q->properties.type)]; > > + mqd_mgr->checkpoint_mqd(mqd_mgr, q->mqd, > q->mqd_backup, q->ctl_stack_backup); > > + q->mqd_backup_valid = true; > > +} > > + > > +static void dqm_restore_mqd_backup(struct > device_queue_manager *dqm, struct queue *q) > > +{ > > + struct mqd_manager *mqd_mgr; > > + > > + if (!q->mqd_backup_valid || !q->mqd_backup) > > + return; > > + if (!q->mqd_mem_obj) > > + return; > > + > > + mqd_mgr = > dqm->mqd_mgrs[get_mqd_type_from_queue_type(q->properties.type)]; > > + if (!mqd_mgr->restore_mqd) > > + return; > > + > > + mqd_mgr->restore_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj, > &q->gart_mqd_addr, > > + &q->properties, q->mqd_backup, > q->ctl_stack_backup, > > + q->ctl_stack_backup_size); > > + q->mqd_backup_valid = false; > > +} > > + > > static int evict_process_queues_cpsch(struct > device_queue_manager *dqm, > > struct > qcm_process_device *qpd) > > { > > @@ -1313,8 +1412,16 @@ static int > evict_process_queues_cpsch(struct device_queue_manager *dqm, > > int retval = 0; > > > > dqm_lock(dqm); > > - if (qpd->evicted++ > 0) /* already evicted, do nothing */ > > + if (qpd->evicted++ > 0) { /* already evicted, do > nothing */ > > + /* A process already runtime-evicted when > hibernation starts > > + * skips the save loop below, so snapshot its > VRAM MQDs here > > + * before the S4 image is taken. > dqm_save_mqd_backup() is a > > + * no-op outside S4. > > + */ > > + list_for_each_entry(q, &qpd->queues_list, list) > > + dqm_save_mqd_backup(dqm, q); > > goto out; > > + } > > > > pdd = qpd_to_pdd(qpd); > > > > @@ -1350,6 +1457,8 @@ static int > evict_process_queues_cpsch(struct device_queue_manager *dqm, > > goto out; > > } > > } > > + > > + dqm_save_mqd_backup(dqm, q); > > } > > > > if (!dqm->dev->kfd->shared_resources.enable_mes) { > > @@ -1423,6 +1532,7 @@ static int > restore_process_queues_nocpsch(struct device_queue_manager *dqm, > > > > mqd_mgr = > dqm->mqd_mgrs[get_mqd_type_from_queue_type( > > q->properties.type)]; > > + dqm_restore_mqd_backup(dqm, q); > > q->properties.is_active = true; > > increment_queue_count(dqm, qpd, q); > > > > @@ -1486,6 +1596,7 @@ static int > restore_process_queues_cpsch(struct device_queue_manager *dqm, > > if (!QUEUE_IS_ACTIVE(q->properties)) > > continue; > > > > + dqm_restore_mqd_backup(dqm, q); > > Those are the wrong point to call those functions if I'm not > completely mistaken. > > Regards, > Christian. > > > q->properties.is_active = true; > > increment_queue_count(dqm, &pdd->qpd, q); > > > > @@ -2161,6 +2272,10 @@ static int create_queue_cpsch(struct > device_queue_manager *dqm, struct queue *q, > > mqd_mgr->init_mqd(mqd_mgr, &q->mqd, > q->mqd_mem_obj, > > &q->gart_mqd_addr, &q->properties); > > > > + retval = dqm_alloc_mqd_backup(dqm, q); > > + if (retval) > > + goto out_free_mqd; > > + > > list_add(&q->list, &qpd->queues_list); > > qpd->queue_count++; > > > > @@ -2193,6 +2308,7 @@ static int create_queue_cpsch(struct > device_queue_manager *dqm, struct queue *q, > > list_del(&q->list); > > if (q->properties.is_active) > > decrement_queue_count(dqm, qpd, q); > > +out_free_mqd: > > mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj); > > dqm_unlock(dqm); > > out_deallocate_doorbell: > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > index 88191a4c1657..1f728f13813e 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > @@ -637,6 +637,15 @@ struct queue { > > void *gang_ctx_cpu_ptr; > > > > struct amdgpu_bo *wptr_bo_gart; > > + > > + /* system-RAM shadow of a VRAM-resident MQD (+ control > stack) > > + * for hibernation snapshot/restore > > + */ > > + void *mqd_backup; > > + void *ctl_stack_backup; > > + uint32_t mqd_backup_size; > > + uint32_t ctl_stack_backup_size; > > + bool mqd_backup_valid; > > }; > > > > enum KFD_MQD_TYPE { > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c > b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c > > index 98a5512b701b..cbc90bf87792 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c > > @@ -83,6 +83,8 @@ int init_queue(struct queue **q, const > struct queue_properties *properties) > > > > void uninit_queue(struct queue *q) > > { > > + kfree(q->mqd_backup); > > + kfree(q->ctl_stack_backup); > > kfree(q); > > } > > >