<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - Internal Distribution Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Thanks! I will update the commit message before pushing. It should be the way how SDMA queue count were used to unmap SDMA engines according to the previous understanding was wrong.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Regards,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Yong</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Kuehling, Felix <Felix.Kuehling@amd.com> Sent: Tuesday, February 25, 2020 12:06 PM To: Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">As I understand it, the SDMA queue counting wasn't incorrect. The main change here is that you no longer send separate unmap packets for SDMA queues, and that makes SDMA queue counting unnecessary. That said, this patch series is a nice cleanup and improvement. The series is Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> On 2020-02-24 17:18, Yong Zhao wrote: > The previous SDMA queue counting was wrong. In addition, after confirming > with MEC firmware team, we understands that only one unmap queue package, > instead of one unmap queue package for CP and each SDMA engine, is needed, > which results in much simpler driver code. > > Change-Id: I84fd2f7e63d6b7f664580b425a78d3e995ce9abc > Signed-off-by: Yong Zhao <Yong.Zhao@amd.com> > --- > .../drm/amd/amdkfd/kfd_device_queue_manager.c | 79 ++++++------------- > .../drm/amd/amdkfd/kfd_device_queue_manager.h | 2 - > .../amd/amdkfd/kfd_process_queue_manager.c | 16 ++-- > 3 files changed, 29 insertions(+), 68 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 958275db3f55..692abfd2088a 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > @@ -109,6 +109,11 @@ static unsigned int get_num_xgmi_sdma_engines(struct device_queue_manager *dqm) > return dqm->dev->device_info->num_xgmi_sdma_engines; > } > > +static unsigned int get_num_all_sdma_engines(struct device_queue_manager *dqm) > +{ > + return get_num_sdma_engines(dqm) + get_num_xgmi_sdma_engines(dqm); > +} > + > unsigned int get_num_sdma_queues(struct device_queue_manager *dqm) > { > return dqm->dev->device_info->num_sdma_engines > @@ -375,11 +380,6 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm, > if (q->properties.is_active) > increment_queue_count(dqm, q->properties.type); > > - if (q->properties.type == KFD_QUEUE_TYPE_SDMA) > - dqm->sdma_queue_count++; > - else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) > - dqm->xgmi_sdma_queue_count++; > - > /* > * Unconditionally increment this counter, regardless of the queue's > * type or whether the queue is active. > @@ -460,15 +460,13 @@ static int destroy_queue_nocpsch_locked(struct device_queue_manager *dqm, > mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type( > q->properties.type)]; > > - if (q->properties.type == KFD_QUEUE_TYPE_COMPUTE) { > + if (q->properties.type == KFD_QUEUE_TYPE_COMPUTE) > deallocate_hqd(dqm, q); > - } else if (q->properties.type == KFD_QUEUE_TYPE_SDMA) { > - dqm->sdma_queue_count--; > + else if (q->properties.type == KFD_QUEUE_TYPE_SDMA) > deallocate_sdma_queue(dqm, q); > - } else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) { > - dqm->xgmi_sdma_queue_count--; > + else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) > deallocate_sdma_queue(dqm, q); > - } else { > + else { > pr_debug("q->properties.type %d is invalid\n", > q->properties.type); > return -EINVAL; > @@ -915,8 +913,6 @@ static int initialize_nocpsch(struct device_queue_manager *dqm) > INIT_LIST_HEAD(&dqm->queues); > dqm->active_queue_count = dqm->next_pipe_to_allocate = 0; > dqm->active_cp_queue_count = 0; > - dqm->sdma_queue_count = 0; > - dqm->xgmi_sdma_queue_count = 0; > > for (pipe = 0; pipe < get_pipes_per_mec(dqm); pipe++) { > int pipe_offset = pipe * get_queues_per_pipe(dqm); > @@ -981,8 +977,11 @@ static int allocate_sdma_queue(struct device_queue_manager *dqm, > int bit; > > if (q->properties.type == KFD_QUEUE_TYPE_SDMA) { > - if (dqm->sdma_bitmap == 0) > + if (dqm->sdma_bitmap == 0) { > + pr_err("No more SDMA queue to allocate\n"); > return -ENOMEM; > + } > + > bit = __ffs64(dqm->sdma_bitmap); > dqm->sdma_bitmap &= ~(1ULL << bit); > q->sdma_id = bit; > @@ -991,8 +990,10 @@ static int allocate_sdma_queue(struct device_queue_manager *dqm, > q->properties.sdma_queue_id = q->sdma_id / > get_num_sdma_engines(dqm); > } else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) { > - if (dqm->xgmi_sdma_bitmap == 0) > + if (dqm->xgmi_sdma_bitmap == 0) { > + pr_err("No more XGMI SDMA queue to allocate\n"); > return -ENOMEM; > + } > bit = __ffs64(dqm->xgmi_sdma_bitmap); > dqm->xgmi_sdma_bitmap &= ~(1ULL << bit); > q->sdma_id = bit; > @@ -1081,8 +1082,7 @@ static int initialize_cpsch(struct device_queue_manager *dqm) > INIT_LIST_HEAD(&dqm->queues); > dqm->active_queue_count = dqm->processes_count = 0; > dqm->active_cp_queue_count = 0; > - dqm->sdma_queue_count = 0; > - dqm->xgmi_sdma_queue_count = 0; > + > dqm->active_runlist = false; > dqm->sdma_bitmap = ~0ULL >> (64 - get_num_sdma_queues(dqm)); > dqm->xgmi_sdma_bitmap = ~0ULL >> (64 - get_num_xgmi_sdma_queues(dqm)); > @@ -1254,11 +1254,6 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q, > list_add(&q->list, &qpd->queues_list); > qpd->queue_count++; > > - if (q->properties.type == KFD_QUEUE_TYPE_SDMA) > - dqm->sdma_queue_count++; > - else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) > - dqm->xgmi_sdma_queue_count++; > - > if (q->properties.is_active) { > increment_queue_count(dqm, q->properties.type); > > @@ -1315,20 +1310,6 @@ int amdkfd_fence_wait_timeout(unsigned int *fence_addr, > return 0; > } > > -static int unmap_sdma_queues(struct device_queue_manager *dqm) > -{ > - int i, retval = 0; > - > - for (i = 0; i < dqm->dev->device_info->num_sdma_engines + > - dqm->dev->device_info->num_xgmi_sdma_engines; i++) { > - retval = pm_send_unmap_queue(&dqm->packets, KFD_QUEUE_TYPE_SDMA, > - KFD_UNMAP_QUEUES_FILTER_DYNAMIC_QUEUES, 0, false, i); > - if (retval) > - return retval; > - } > - return retval; > -} > - > /* dqm->lock mutex has to be locked before calling this function */ > static int map_queues_cpsch(struct device_queue_manager *dqm) > { > @@ -1366,12 +1347,6 @@ static int unmap_queues_cpsch(struct device_queue_manager *dqm, > if (!dqm->active_runlist) > return retval; > > - pr_debug("Before destroying queues, sdma queue count is : %u, xgmi sdma queue count is : %u\n", > - dqm->sdma_queue_count, dqm->xgmi_sdma_queue_count); > - > - if (dqm->sdma_queue_count > 0 || dqm->xgmi_sdma_queue_count) > - unmap_sdma_queues(dqm); > - > retval = pm_send_unmap_queue(&dqm->packets, KFD_QUEUE_TYPE_COMPUTE, > filter, filter_param, false, 0); > if (retval) > @@ -1444,13 +1419,10 @@ static int destroy_queue_cpsch(struct device_queue_manager *dqm, > > deallocate_doorbell(qpd, q); > > - if (q->properties.type == KFD_QUEUE_TYPE_SDMA) { > - dqm->sdma_queue_count--; > + if (q->properties.type == KFD_QUEUE_TYPE_SDMA) > deallocate_sdma_queue(dqm, q); > - } else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) { > - dqm->xgmi_sdma_queue_count--; > + else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) > deallocate_sdma_queue(dqm, q); > - } > > list_del(&q->list); > qpd->queue_count--; > @@ -1673,13 +1645,10 @@ static int process_termination_cpsch(struct device_queue_manager *dqm, > > /* Clear all user mode queues */ > list_for_each_entry(q, &qpd->queues_list, list) { > - if (q->properties.type == KFD_QUEUE_TYPE_SDMA) { > - dqm->sdma_queue_count--; > + if (q->properties.type == KFD_QUEUE_TYPE_SDMA) > deallocate_sdma_queue(dqm, q); > - } else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) { > - dqm->xgmi_sdma_queue_count--; > + else if (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) > deallocate_sdma_queue(dqm, q); > - } > > if (q->properties.is_active) > decrement_queue_count(dqm, q->properties.type); > @@ -1759,8 +1728,7 @@ static int allocate_hiq_sdma_mqd(struct device_queue_manager *dqm) > struct kfd_dev *dev = dqm->dev; > struct kfd_mem_obj *mem_obj = &dqm->hiq_sdma_mqd; > uint32_t size = dqm->mqd_mgrs[KFD_MQD_TYPE_SDMA]->mqd_size * > - (dev->device_info->num_sdma_engines + > - dev->device_info->num_xgmi_sdma_engines) * > + get_num_all_sdma_engines(dqm) * > dev->device_info->num_sdma_queues_per_engine + > dqm->mqd_mgrs[KFD_MQD_TYPE_HIQ]->mqd_size; > > @@ -2012,8 +1980,7 @@ int dqm_debugfs_hqds(struct seq_file *m, void *data) > } > } > > - for (pipe = 0; pipe < get_num_sdma_engines(dqm) + > - get_num_xgmi_sdma_engines(dqm); pipe++) { > + for (pipe = 0; pipe < get_num_all_sdma_engines(dqm); pipe++) { > for (queue = 0; > queue < dqm->dev->device_info->num_sdma_queues_per_engine; > queue++) { > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h > index 05e0afc04cd9..50d919f814e9 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h > @@ -182,8 +182,6 @@ struct device_queue_manager { > unsigned int processes_count; > unsigned int active_queue_count; > unsigned int active_cp_queue_count; > - unsigned int sdma_queue_count; > - unsigned int xgmi_sdma_queue_count; > unsigned int total_queue_count; > unsigned int next_pipe_to_allocate; > unsigned int *allocated_queues; > 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 3bfa5c8d9654..eb1635ac8988 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c > @@ -241,16 +241,12 @@ int pqm_create_queue(struct process_queue_manager *pqm, > switch (type) { > case KFD_QUEUE_TYPE_SDMA: > case KFD_QUEUE_TYPE_SDMA_XGMI: > - if ((type == KFD_QUEUE_TYPE_SDMA && dev->dqm->sdma_queue_count > - >= get_num_sdma_queues(dev->dqm)) || > - (type == KFD_QUEUE_TYPE_SDMA_XGMI && > - dev->dqm->xgmi_sdma_queue_count > - >= get_num_xgmi_sdma_queues(dev->dqm))) { > - pr_debug("Over-subscription is not allowed for SDMA.\n"); > - retval = -EPERM; > - goto err_create_queue; > - } > - > + /* SDMA queues are always allocated statically no matter > + * which scheduler mode is used. We also do not need to > + * check whether a SDMA queue can be allocated here, because > + * allocate_sdma_queue() in create_queue() has the > + * corresponding check logic. > + */ > retval = init_user_queue(pqm, dev, &q, properties, f, *qid); > if (retval != 0) > goto err_create_queue; </div> </div> </div> </body> </html>