Re: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions
[AMD Official Use Only - Internal Distribution Only] 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. Regards, Yong From: Kuehling, Felix Sent: Tuesday, February 25, 2020 12:06 PM To: Zhao, Yong ; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions 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 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 > --- > .../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; > +
Re: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions
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 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 --- .../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_
Re: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions
[AMD Public Use] Series is: Acked-by: Alex Deucher From: amd-gfx on behalf of Yong Zhao Sent: Monday, February 24, 2020 5:18 PM To: amd-gfx@lists.freedesktop.org Cc: Zhao, Yong Subject: [PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions 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 --- .../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 @@ stat
[PATCH 6/6] drm/amdkfd: Delete unnecessary unmap queue package submissions
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 --- .../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_