> -----Original Message----- > From: Paul Menzel <pmenzel@xxxxxxxxxxxxx> > Sent: Monday, April 18, 2022 4:23 PM > To: Yat Sin, David <David.YatSin@xxxxxxx> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Kuehling, Felix > <Felix.Kuehling@xxxxxxx> > Subject: Re: [PATCH v2 2/2] drm/amdkfd: CRIU add support for GWS queues > > Dear David, > > > Thank you for your patch. > > Am 18.04.22 um 18:44 schrieb David Yat Sin: > > In the commit message summary, you could reorder some words: > > Add CRIU support for GWS queues > > > Adding support to checkpoint/restore GWS(Global Wave Sync) queues. > > s/Adding/Add/ > > Please add a space before the (. ACK > > How can this be tested? We have some internal tests that can we be used to specifically test this feature. > > > Signed-off-by: David Yat Sin <david.yatsin@xxxxxxx> > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 +- > > drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 10 > +++++++--- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > index f36062be9ca8..192dbef04c43 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > > @@ -1102,7 +1102,7 @@ struct kfd_criu_queue_priv_data { > > uint32_t priority; > > uint32_t q_percent; > > uint32_t doorbell_id; > > - uint32_t is_gws; > > + uint32_t gws; > > Why is the new name better? The old variable (is_gws) was obtained from the queue_properties structure during checkpoint and is only used temporarily during queue creation, so this variable cannot be used to determine whether a queue as gws enabled. The new variable (gws) is obtained from the queue structure. The name is changed to better reflect this. > > > uint32_t sdma_id; > > uint32_t eop_ring_buffer_size; > > uint32_t ctx_save_restore_area_size; diff --git > > a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > index 6eca9509f2e3..4f58e671d39b 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > > @@ -636,6 +636,8 @@ static int criu_checkpoint_queue(struct > kfd_process_device *pdd, > > q_data->ctx_save_restore_area_size = > > q->properties.ctx_save_restore_area_size; > > > > + q_data->gws = !!q->gws; > > + > > ret = pqm_checkpoint_mqd(&pdd->process->pqm, q- > >properties.queue_id, mqd, ctl_stack); > > if (ret) { > > pr_err("Failed checkpoint queue_mqd (%d)\n", ret); @@ - > 743,7 > > +745,6 @@ static void set_queue_properties_from_criu(struct > queue_properties *qp, > > struct kfd_criu_queue_priv_data > *q_data) > > { > > qp->is_interop = false; > > - qp->is_gws = q_data->is_gws; > > qp->queue_percent = q_data->q_percent; > > qp->priority = q_data->priority; > > qp->queue_address = q_data->q_address; @@ -826,12 +827,15 @@ > int > > kfd_criu_restore_queue(struct kfd_process *p, > > NULL); > > if (ret) { > > pr_err("Failed to create new queue err:%d\n", ret); > > - ret = -EINVAL; > > + goto exit; > > } > > > > + if (q_data->gws) > > + ret = pqm_set_gws(&p->pqm, q_data->q_id, pdd->dev->gws); > > + > > exit: > > if (ret) > > - pr_err("Failed to create queue (%d)\n", ret); > > + pr_err("Failed to restore queue (%d)\n", ret); > > Maybe separate this out, so it can be applied to stable series. > > > else > > pr_debug("Queue id %d was restored successfully\n", > queue_id); > > > > > Kind regards, > > Paul