Nice. Surprisingly this actually resulted in slightly fewer lines of
code. Some suggestions:
Let's also rename uninit_mqd to free_mqd, as it is the counterpart to
allocate_mqd, not init_mqd.
And unless I'm missing something, init_mqd and update_mqd cannot fail
now that allocate_mqd has been separated from them. That means you could
change them to void functions, and it may simplify some of the error
handling if you have fewer potential failure cases to consider.
Regards,
Felix
On 2019-06-05 12:06 p.m., Zeng, Oak wrote:
> Introduce a new mqd allocation interface and split the original
> init_mqd function into two functions: allocate_mqd and init_mqd.
> This is preparation work to fix a circular lock dependency.
>
> Change-Id: I26e53ee1abcdd688ad11d35b433da77e3fa1bee7
> Signed-off-by: Oak Zeng <Oak.Zeng@xxxxxxx>
> ---
> .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 16 ++++++++---
> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 8 +++++-
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h | 8 ++++--
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c | 31 ++++++++--------------
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 31 ++++++++--------------
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c | 31 ++++++++--------------
> 7 files changed, 59 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 3c042eb..787b5be 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -319,10 +319,13 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> if (retval)
> goto out_deallocate_hqd;
>
> - retval = mqd_mgr->init_mqd(mqd_mgr, &q->mqd, &q->mqd_mem_obj,
> + q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> + if (!q->mqd_mem_obj)
> + goto out_deallocate_doorbell;
> + retval = mqd_mgr->init_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj,
> &q->gart_mqd_addr, &q->properties);
> if (retval)
> - goto out_deallocate_doorbell;
> + goto out_uninit_mqd;
>
> if (q->properties.is_active) {
>
> @@ -1160,10 +1163,13 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q,
> dqm->asic_ops.init_sdma_vm(dqm, q, qpd);
> q->properties.tba_addr = qpd->tba_addr;
> q->properties.tma_addr = qpd->tma_addr;
> - retval = mqd_mgr->init_mqd(mqd_mgr, &q->mqd, &q->mqd_mem_obj,
> + q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> + if (!q->mqd_mem_obj)
> + goto out_deallocate_doorbell;
> + retval = mqd_mgr->init_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj,
> &q->gart_mqd_addr, &q->properties);
> if (retval)
> - goto out_deallocate_doorbell;
> + goto out_deallocate_mqd;
>
> dqm_lock(dqm);
>
> @@ -1191,6 +1197,8 @@ static int create_queue_cpsch(struct device_queue_manager *dqm, struct queue *q,
> dqm_unlock(dqm);
> return retval;
>
> +out_deallocate_mqd:
> + mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> out_deallocate_doorbell:
> deallocate_doorbell(qpd, q);
> out_deallocate_sdma_queue:
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> index 1cc03b3..64c8ffe 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> @@ -132,8 +132,12 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
> kq->queue->device = dev;
> kq->queue->process = kfd_get_process(current);
>
> + kq->queue->mqd_mem_obj = kq->mqd_mgr->allocate_mqd(kq->mqd_mgr->dev,
> + &kq->queue->properties);
> + if (!kq->queue->mqd_mem_obj)
> + goto err_allocate_mqd;
> retval = kq->mqd_mgr->init_mqd(kq->mqd_mgr, &kq->queue->mqd,
> - &kq->queue->mqd_mem_obj,
> + kq->queue->mqd_mem_obj,
> &kq->queue->gart_mqd_addr,
> &kq->queue->properties);
> if (retval != 0)
> @@ -165,6 +169,8 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
> return true;
> err_alloc_fence:
> err_init_mqd:
> + kq->mqd_mgr->uninit_mqd(kq->mqd_mgr, kq->queue->mqd, kq->queue->mqd_mem_obj);
> +err_allocate_mqd:
> uninit_queue(kq->queue);
> err_init_queue:
> kfd_gtt_sa_free(dev, kq->wptr_mem);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> index cc04b362..b4f305a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
> @@ -45,7 +45,7 @@ int pipe_priority_map[] = {
> KFD_PIPE_PRIORITY_CS_HIGH
> };
>
> -struct kfd_mem_obj *allocate_hiq_mqd(struct kfd_dev *dev)
> +struct kfd_mem_obj *allocate_hiq_mqd(struct kfd_dev *dev, struct queue_properties *q)
> {
> struct kfd_mem_obj *mqd_mem_obj = NULL;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> index 66b8c67..8667990 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> @@ -64,8 +64,11 @@
> */
> extern int pipe_priority_map[];
> struct mqd_manager {
> + struct kfd_mem_obj* (*allocate_mqd)(struct kfd_dev *kfd,
> + struct queue_properties *q);
> +
> int (*init_mqd)(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q);
>
> int (*load_mqd)(struct mqd_manager *mm, void *mqd,
> @@ -102,7 +105,8 @@ struct mqd_manager {
> uint32_t mqd_size;
> };
>
> -struct kfd_mem_obj *allocate_hiq_mqd(struct kfd_dev *dev);
> +struct kfd_mem_obj *allocate_hiq_mqd(struct kfd_dev *dev,
> + struct queue_properties *q);
>
> struct kfd_mem_obj *allocate_sdma_mqd(struct kfd_dev *dev,
> struct queue_properties *q);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> index e911438..bbe808d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c
> @@ -77,9 +77,6 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> {
> struct kfd_mem_obj *mqd_mem_obj;
>
> - if (q->type == KFD_QUEUE_TYPE_HIQ)
> - return allocate_hiq_mqd(kfd);
> -
> if (kfd_gtt_sa_allocate(kfd, sizeof(struct cik_mqd),
> &mqd_mem_obj))
> return NULL;
> @@ -88,20 +85,15 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> }
>
> static int init_mqd(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> uint64_t addr;
> struct cik_mqd *m;
> int retval;
> - struct kfd_dev *kfd = mm->dev;
> -
> - *mqd_mem_obj = allocate_mqd(kfd, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
>
> - m = (struct cik_mqd *) (*mqd_mem_obj)->cpu_ptr;
> - addr = (*mqd_mem_obj)->gpu_addr;
> + m = (struct cik_mqd *) mqd_mem_obj->cpu_ptr;
> + addr = mqd_mem_obj->gpu_addr;
>
> memset(m, 0, ALIGN(sizeof(struct cik_mqd), 256));
>
> @@ -150,24 +142,19 @@ static int init_mqd(struct mqd_manager *mm, void **mqd,
> }
>
> static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> int retval;
> struct cik_sdma_rlc_registers *m;
> - struct kfd_dev *dev = mm->dev;
> -
> - *mqd_mem_obj = allocate_sdma_mqd(dev, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
>
> - m = (struct cik_sdma_rlc_registers *) (*mqd_mem_obj)->cpu_ptr;
> + m = (struct cik_sdma_rlc_registers *) mqd_mem_obj->cpu_ptr;
>
> memset(m, 0, sizeof(struct cik_sdma_rlc_registers));
>
> *mqd = m;
> if (gart_addr)
> - *gart_addr = (*mqd_mem_obj)->gpu_addr;
> + *gart_addr = mqd_mem_obj->gpu_addr;
>
> retval = mm->update_mqd(mm, m, q);
>
> @@ -327,7 +314,7 @@ static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
> */
>
> static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> return init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
> @@ -399,6 +386,7 @@ struct mqd_manager *mqd_manager_init_cik(enum KFD_MQD_TYPE type,
> switch (type) {
> case KFD_MQD_TYPE_CP:
> case KFD_MQD_TYPE_COMPUTE:
> + mqd->allocate_mqd = allocate_mqd;
> mqd->init_mqd = init_mqd;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -411,6 +399,7 @@ struct mqd_manager *mqd_manager_init_cik(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_HIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> @@ -423,6 +412,7 @@ struct mqd_manager *mqd_manager_init_cik(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_DIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -435,6 +425,7 @@ struct mqd_manager *mqd_manager_init_cik(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_SDMA:
> + mqd->allocate_mqd = allocate_sdma_mqd;
> mqd->init_mqd = init_mqd_sdma;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> index 818944b..572cc6a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c
> @@ -79,9 +79,6 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> int retval;
> struct kfd_mem_obj *mqd_mem_obj = NULL;
>
> - if (q->type == KFD_QUEUE_TYPE_HIQ)
> - return allocate_hiq_mqd(kfd);
> -
> /* From V9, for CWSR, the control stack is located on the next page
> * boundary after the mqd, we will use the gtt allocation function
> * instead of sub-allocation function.
> @@ -111,20 +108,15 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> }
>
> static int init_mqd(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> int retval;
> uint64_t addr;
> struct v9_mqd *m;
> - struct kfd_dev *kfd = mm->dev;
>
> - *mqd_mem_obj = allocate_mqd(kfd, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
> -
> - m = (struct v9_mqd *) (*mqd_mem_obj)->cpu_ptr;
> - addr = (*mqd_mem_obj)->gpu_addr;
> + m = (struct v9_mqd *) mqd_mem_obj->cpu_ptr;
> + addr = mqd_mem_obj->gpu_addr;
>
> memset(m, 0, sizeof(struct v9_mqd));
>
> @@ -312,7 +304,7 @@ static int get_wave_state(struct mqd_manager *mm, void *mqd,
> }
>
> static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> struct v9_mqd *m;
> @@ -345,24 +337,19 @@ static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> }
>
> static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> int retval;
> struct v9_sdma_mqd *m;
> - struct kfd_dev *dev = mm->dev;
> -
> - *mqd_mem_obj = allocate_sdma_mqd(dev, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
>
> - m = (struct v9_sdma_mqd *) (*mqd_mem_obj)->cpu_ptr;
> + m = (struct v9_sdma_mqd *) mqd_mem_obj->cpu_ptr;
>
> memset(m, 0, sizeof(struct v9_sdma_mqd));
>
> *mqd = m;
> if (gart_addr)
> - *gart_addr = (*mqd_mem_obj)->gpu_addr;
> + *gart_addr = mqd_mem_obj->gpu_addr;
>
> retval = mm->update_mqd(mm, m, q);
>
> @@ -462,6 +449,7 @@ struct mqd_manager *mqd_manager_init_v9(enum KFD_MQD_TYPE type,
> switch (type) {
> case KFD_MQD_TYPE_CP:
> case KFD_MQD_TYPE_COMPUTE:
> + mqd->allocate_mqd = allocate_mqd;
> mqd->init_mqd = init_mqd;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -475,6 +463,7 @@ struct mqd_manager *mqd_manager_init_v9(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_HIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> @@ -487,6 +476,7 @@ struct mqd_manager *mqd_manager_init_v9(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_DIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -499,6 +489,7 @@ struct mqd_manager *mqd_manager_init_v9(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_SDMA:
> + mqd->allocate_mqd = allocate_sdma_mqd;
> mqd->init_mqd = init_mqd_sdma;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> index 00e6a59..431899f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c
> @@ -80,9 +80,6 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> {
> struct kfd_mem_obj *mqd_mem_obj;
>
> - if (q->type == KFD_QUEUE_TYPE_HIQ)
> - return allocate_hiq_mqd(kfd);
> -
> if (kfd_gtt_sa_allocate(kfd, sizeof(struct vi_mqd),
> &mqd_mem_obj))
> return NULL;
> @@ -91,20 +88,15 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> }
>
> static int init_mqd(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> int retval;
> uint64_t addr;
> struct vi_mqd *m;
> - struct kfd_dev *kfd = mm->dev;
> -
> - *mqd_mem_obj = allocate_mqd(kfd, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
>
> - m = (struct vi_mqd *) (*mqd_mem_obj)->cpu_ptr;
> - addr = (*mqd_mem_obj)->gpu_addr;
> + m = (struct vi_mqd *) mqd_mem_obj->cpu_ptr;
> + addr = mqd_mem_obj->gpu_addr;
>
> memset(m, 0, sizeof(struct vi_mqd));
>
> @@ -310,7 +302,7 @@ static int get_wave_state(struct mqd_manager *mm, void *mqd,
> }
>
> static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> struct vi_mqd *m;
> @@ -342,24 +334,19 @@ static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> }
>
> static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> int retval;
> struct vi_sdma_mqd *m;
> - struct kfd_dev *dev = mm->dev;
> -
> - *mqd_mem_obj = allocate_sdma_mqd(dev, q);
> - if (!*mqd_mem_obj)
> - return -ENOMEM;
>
> - m = (struct vi_sdma_mqd *) (*mqd_mem_obj)->cpu_ptr;
> + m = (struct vi_sdma_mqd *) mqd_mem_obj->cpu_ptr;
>
> memset(m, 0, sizeof(struct vi_sdma_mqd));
>
> *mqd = m;
> if (gart_addr)
> - *gart_addr = (*mqd_mem_obj)->gpu_addr;
> + *gart_addr = mqd_mem_obj->gpu_addr;
>
> retval = mm->update_mqd(mm, m, q);
>
> @@ -458,6 +445,7 @@ struct mqd_manager *mqd_manager_init_vi(enum KFD_MQD_TYPE type,
> switch (type) {
> case KFD_MQD_TYPE_CP:
> case KFD_MQD_TYPE_COMPUTE:
> + mqd->allocate_mqd = allocate_mqd;
> mqd->init_mqd = init_mqd;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -471,6 +459,7 @@ struct mqd_manager *mqd_manager_init_vi(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_HIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> @@ -483,6 +472,7 @@ struct mqd_manager *mqd_manager_init_vi(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_DIQ:
> + mqd->allocate_mqd = allocate_hiq_mqd;
> mqd->init_mqd = init_mqd_hiq;
> mqd->uninit_mqd = uninit_mqd;
> mqd->load_mqd = load_mqd;
> @@ -495,6 +485,7 @@ struct mqd_manager *mqd_manager_init_vi(enum KFD_MQD_TYPE type,
> #endif
> break;
> case KFD_MQD_TYPE_SDMA:
> + mqd->allocate_mqd = allocate_sdma_mqd;
> mqd->init_mqd = init_mqd_sdma;
> mqd->uninit_mqd = uninit_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
