From: "Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
To: "Zeng, Oak" <Oak.Zeng-5C7GfCeVMHo@public.gmane.org>,
"amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Cc: "Zhao, Yong" <Yong.Zhao-5C7GfCeVMHo@public.gmane.org>,
"Freehill, Chris" <Chris.Freehill-5C7GfCeVMHo@public.gmane.org>,
"Liu, Alex" <Alex.Liu-5C7GfCeVMHo@public.gmane.org>
Subject: Re: [PATCH 4/6] drm/amdkfd: Separate mqd allocation and initialization
Date: Thu, 6 Jun 2019 20:13:46 +0000 [thread overview]
Message-ID: <ed6ffa65-a53b-0ad5-0f57-e7813a36d710@amd.com> (raw)
In-Reply-To: <1559845507-3052-1-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
On 2019-06-06 2:25 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.
> Also renamed uninit_mqd to free_mqd. This is preparation work to
> fix a circular lock dependency.
>
> Change-Id: I26e53ee1abcdd688ad11d35b433da77e3fa1bee7
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
> .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 34 ++++-----
> drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 16 ++--
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c | 4 +-
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h | 18 +++--
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_cik.c | 78 +++++++------------
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 78 +++++++------------
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_vi.c | 88 ++++++++--------------
> 7 files changed, 124 insertions(+), 192 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..10d4f4f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -319,11 +319,11 @@ 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->gart_mqd_addr, &q->properties);
> - if (retval)
> + q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> + if (!q->mqd_mem_obj)
> goto out_deallocate_doorbell;
You need to set retval = -ENOMEM here.
> -
> + mqd_mgr->init_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj,
> + &q->gart_mqd_addr, &q->properties);
> if (q->properties.is_active) {
>
> if (WARN(q->process->mm != current->mm,
> @@ -333,7 +333,7 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> retval = mqd_mgr->load_mqd(mqd_mgr, q->mqd, q->pipe,
> q->queue, &q->properties, current->mm);
> if (retval)
> - goto out_uninit_mqd;
> + goto out_free_mqd;
> }
>
> list_add(&q->list, &qpd->queues_list);
> @@ -355,8 +355,8 @@ static int create_queue_nocpsch(struct device_queue_manager *dqm,
> dqm->total_queue_count);
> goto out_unlock;
>
> -out_uninit_mqd:
> - mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> +out_free_mqd:
> + mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> out_deallocate_doorbell:
> deallocate_doorbell(qpd, q);
> out_deallocate_hqd:
> @@ -450,7 +450,7 @@ static int destroy_queue_nocpsch_locked(struct device_queue_manager *dqm,
> if (retval == -ETIME)
> qpd->reset_wavefronts = true;
>
> - mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> + mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
>
> list_del(&q->list);
> if (list_empty(&qpd->queues_list)) {
> @@ -527,7 +527,7 @@ static int update_queue(struct device_queue_manager *dqm, struct queue *q)
> }
> }
>
> - retval = mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties);
> + mqd_mgr->update_mqd(mqd_mgr, q->mqd, &q->properties);
I think there is a chance now that retval will remain uninitialized in
this function (in the non-HWS case if a queue is inactive). You should
initialized it to 0 at the start of the function.
Check the other functions as well. Check for compiler warnings. Most of
these types of problems should be caught by the compiler.
>
> /*
> * check active state vs. the previous state and modify
> @@ -1160,11 +1160,11 @@ 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->gart_mqd_addr, &q->properties);
> - if (retval)
> + q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, &q->properties);
> + if (!q->mqd_mem_obj)
> goto out_deallocate_doorbell;
retval = -ENOMEM;
> -
> + mqd_mgr->init_mqd(mqd_mgr, &q->mqd, q->mqd_mem_obj,
> + &q->gart_mqd_addr, &q->properties);
> dqm_lock(dqm);
>
> list_add(&q->list, &qpd->queues_list);
> @@ -1373,8 +1373,8 @@ static int destroy_queue_cpsch(struct device_queue_manager *dqm,
>
> dqm_unlock(dqm);
>
> - /* Do uninit_mqd after dqm_unlock(dqm) to avoid circular locking */
> - mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> + /* Do free_mqd after dqm_unlock(dqm) to avoid circular locking */
> + mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
>
> return retval;
>
> @@ -1615,14 +1615,14 @@ static int process_termination_cpsch(struct device_queue_manager *dqm,
> kfd_dec_compute_active(dqm->dev);
>
> /* Lastly, free mqd resources.
> - * Do uninit_mqd() after dqm_unlock to avoid circular locking.
> + * Do free_mqd() after dqm_unlock to avoid circular locking.
> */
> list_for_each_entry_safe(q, next, &qpd->queues_list, list) {
> mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
> q->properties.type)];
> list_del(&q->list);
> qpd->queue_count--;
> - mqd_mgr->uninit_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> + mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> }
>
> return retval;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> index 1cc03b3..229500c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> @@ -132,13 +132,14 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
> kq->queue->device = dev;
> kq->queue->process = kfd_get_process(current);
>
> - retval = kq->mqd_mgr->init_mqd(kq->mqd_mgr, &kq->queue->mqd,
> - &kq->queue->mqd_mem_obj,
> + 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 = -ENOMEM;
Regards,
Felix
> + kq->mqd_mgr->init_mqd(kq->mqd_mgr, &kq->queue->mqd,
> + kq->queue->mqd_mem_obj,
> &kq->queue->gart_mqd_addr,
> &kq->queue->properties);
> - if (retval != 0)
> - goto err_init_mqd;
> -
> /* assign HIQ to HQD */
> if (type == KFD_QUEUE_TYPE_HIQ) {
> pr_debug("Assigning hiq to hqd\n");
> @@ -164,7 +165,8 @@ static bool initialize(struct kernel_queue *kq, struct kfd_dev *dev,
>
> return true;
> err_alloc_fence:
> -err_init_mqd:
> + kq->mqd_mgr->free_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);
> @@ -193,7 +195,7 @@ static void uninitialize(struct kernel_queue *kq)
> else if (kq->queue->properties.type == KFD_QUEUE_TYPE_DIQ)
> kfd_gtt_sa_free(kq->dev, kq->fence_mem_obj);
>
> - kq->mqd_mgr->uninit_mqd(kq->mqd_mgr, kq->queue->mqd,
> + kq->mqd_mgr->free_mqd(kq->mqd_mgr, kq->queue->mqd,
> kq->queue->mqd_mem_obj);
>
> kfd_gtt_sa_free(kq->dev, kq->rptr_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..d6cf391 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;
>
> @@ -86,7 +86,7 @@ struct kfd_mem_obj *allocate_sdma_mqd(struct kfd_dev *dev,
> return mqd_mem_obj;
> }
>
> -void uninit_mqd_hiq_sdma(struct mqd_manager *mm, void *mqd,
> +void free_mqd_hiq_sdma(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj)
> {
> WARN_ON(!mqd_mem_obj->gtt_mem);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> index 66b8c67..550b61e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.h
> @@ -39,7 +39,7 @@
> * @destroy_mqd: Destroys the HQD slot and by that preempt the relevant queue.
> * Used only for no cp scheduling.
> *
> - * @uninit_mqd: Releases the mqd buffer from local gpu memory.
> + * @free_mqd: Releases the mqd buffer from local gpu memory.
> *
> * @is_occupied: Checks if the relevant HQD slot is occupied.
> *
> @@ -64,8 +64,11 @@
> */
> extern int pipe_priority_map[];
> struct mqd_manager {
> - int (*init_mqd)(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> + struct kfd_mem_obj* (*allocate_mqd)(struct kfd_dev *kfd,
> + struct queue_properties *q);
> +
> + void (*init_mqd)(struct mqd_manager *mm, void **mqd,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q);
>
> int (*load_mqd)(struct mqd_manager *mm, void *mqd,
> @@ -73,7 +76,7 @@ struct mqd_manager {
> struct queue_properties *p,
> struct mm_struct *mms);
>
> - int (*update_mqd)(struct mqd_manager *mm, void *mqd,
> + void (*update_mqd)(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q);
>
> int (*destroy_mqd)(struct mqd_manager *mm, void *mqd,
> @@ -81,7 +84,7 @@ struct mqd_manager {
> unsigned int timeout, uint32_t pipe_id,
> uint32_t queue_id);
>
> - void (*uninit_mqd)(struct mqd_manager *mm, void *mqd,
> + void (*free_mqd)(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj);
>
> bool (*is_occupied)(struct mqd_manager *mm, void *mqd,
> @@ -102,11 +105,12 @@ 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);
> -void uninit_mqd_hiq_sdma(struct mqd_manager *mm, void *mqd,
> +void free_mqd_hiq_sdma(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj);
>
> void mqd_symmetrically_map_cu_mask(struct mqd_manager *mm,
> 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..28876ac 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;
> @@ -87,21 +84,15 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> return mqd_mem_obj;
> }
>
> -static int init_mqd(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd(struct mqd_manager *mm, void **mqd,
> + 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));
>
> @@ -144,37 +135,27 @@ static int init_mqd(struct mqd_manager *mm, void **mqd,
> *mqd = m;
> if (gart_addr)
> *gart_addr = addr;
> - retval = mm->update_mqd(mm, m, q);
> -
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> -static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> + 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;
> -
> - retval = mm->update_mqd(mm, m, q);
> + *gart_addr = mqd_mem_obj->gpu_addr;
>
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> -static void uninit_mqd(struct mqd_manager *mm, void *mqd,
> +static void free_mqd(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj)
> {
> kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
> @@ -203,7 +184,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> mms);
> }
>
> -static int __update_mqd(struct mqd_manager *mm, void *mqd,
> +static void __update_mqd(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q, unsigned int atc_bit)
> {
> struct cik_mqd *m;
> @@ -237,23 +218,21 @@ static int __update_mqd(struct mqd_manager *mm, void *mqd,
> set_priority(m, q);
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
> -static int update_mqd(struct mqd_manager *mm, void *mqd,
> +static void update_mqd(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> - return __update_mqd(mm, mqd, q, 1);
> + __update_mqd(mm, mqd, q, 1);
> }
>
> -static int update_mqd_hawaii(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_hawaii(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> - return __update_mqd(mm, mqd, q, 0);
> + __update_mqd(mm, mqd, q, 0);
> }
>
> -static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct cik_sdma_rlc_registers *m;
> @@ -278,8 +257,6 @@ static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> m->sdma_queue_id = q->sdma_queue_id;
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
> static int destroy_mqd(struct mqd_manager *mm, void *mqd,
> @@ -326,14 +303,14 @@ static bool is_occupied_sdma(struct mqd_manager *mm, void *mqd,
> * queues but with different initial values.
> */
>
> -static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> + 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);
> + init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
> }
>
> -static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct cik_mqd *m;
> @@ -360,7 +337,6 @@ static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> q->is_active = QUEUE_IS_ACTIVE(*q);
>
> set_priority(m, q);
> - return 0;
> }
>
> #if defined(CONFIG_DEBUG_FS)
> @@ -399,8 +375,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd;
> mqd->destroy_mqd = destroy_mqd;
> @@ -411,8 +388,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -423,8 +401,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -435,8 +414,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
> mqd->update_mqd = update_mqd_sdma;
> mqd->destroy_mqd = destroy_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..0c58f91 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.
> @@ -110,21 +107,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,
> +static void init_mqd(struct mqd_manager *mm, void **mqd,
> + 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));
>
> @@ -173,9 +164,7 @@ static int init_mqd(struct mqd_manager *mm, void **mqd,
> *mqd = m;
> if (gart_addr)
> *gart_addr = addr;
> - retval = mm->update_mqd(mm, m, q);
> -
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> static int load_mqd(struct mqd_manager *mm, void *mqd,
> @@ -190,7 +179,7 @@ static int load_mqd(struct mqd_manager *mm, void *mqd,
> wptr_shift, 0, mms);
> }
>
> -static int update_mqd(struct mqd_manager *mm, void *mqd,
> +static void update_mqd(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct v9_mqd *m;
> @@ -252,8 +241,6 @@ static int update_mqd(struct mqd_manager *mm, void *mqd,
> set_priority(m, q);
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
>
> @@ -267,7 +254,7 @@ static int destroy_mqd(struct mqd_manager *mm, void *mqd,
> pipe_id, queue_id);
> }
>
> -static void uninit_mqd(struct mqd_manager *mm, void *mqd,
> +static void free_mqd(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj)
> {
> struct kfd_dev *kfd = mm->dev;
> @@ -311,62 +298,47 @@ static int get_wave_state(struct mqd_manager *mm, void *mqd,
> return 0;
> }
>
> -static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> struct v9_mqd *m;
> - int retval = init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
>
> - if (retval != 0)
> - return retval;
> + init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
>
> m = get_mqd(*mqd);
>
> m->cp_hqd_pq_control |= 1 << CP_HQD_PQ_CONTROL__PRIV_STATE__SHIFT |
> 1 << CP_HQD_PQ_CONTROL__KMD_QUEUE__SHIFT;
> -
> - return retval;
> }
>
> -static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct v9_mqd *m;
> - int retval = update_mqd(mm, mqd, q);
>
> - if (retval != 0)
> - return retval;
> + update_mqd(mm, mqd, q);
>
> /* TODO: what's the point? update_mqd already does this. */
> m = get_mqd(mqd);
> m->cp_hqd_vmid = q->vmid;
> - return retval;
> }
>
> -static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> + 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);
> -
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> @@ -380,7 +352,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
>
> #define SDMA_RLC_DUMMY_DEFAULT 0xf
>
> -static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct v9_sdma_mqd *m;
> @@ -404,8 +376,6 @@ static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> m->sdmax_rlcx_dummy_reg = SDMA_RLC_DUMMY_DEFAULT;
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
> /*
> @@ -462,8 +432,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd;
> mqd->destroy_mqd = destroy_mqd;
> @@ -475,8 +446,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -487,8 +459,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -499,8 +472,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
> mqd->update_mqd = update_mqd_sdma;
> mqd->destroy_mqd = destroy_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..7d144f5 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;
> @@ -90,21 +87,15 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd,
> return mqd_mem_obj;
> }
>
> -static int init_mqd(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd(struct mqd_manager *mm, void **mqd,
> + 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));
>
> @@ -159,9 +150,7 @@ static int init_mqd(struct mqd_manager *mm, void **mqd,
> *mqd = m;
> if (gart_addr)
> *gart_addr = addr;
> - retval = mm->update_mqd(mm, m, q);
> -
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> static int load_mqd(struct mqd_manager *mm, void *mqd,
> @@ -177,7 +166,7 @@ static int load_mqd(struct mqd_manager *mm, void *mqd,
> wptr_shift, wptr_mask, mms);
> }
>
> -static int __update_mqd(struct mqd_manager *mm, void *mqd,
> +static void __update_mqd(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q, unsigned int mtype,
> unsigned int atc_bit)
> {
> @@ -245,21 +234,19 @@ static int __update_mqd(struct mqd_manager *mm, void *mqd,
> set_priority(m, q);
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
>
> -static int update_mqd(struct mqd_manager *mm, void *mqd,
> +static void update_mqd(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> - return __update_mqd(mm, mqd, q, MTYPE_CC, 1);
> + __update_mqd(mm, mqd, q, MTYPE_CC, 1);
> }
>
> -static int update_mqd_tonga(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_tonga(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> - return __update_mqd(mm, mqd, q, MTYPE_UC, 0);
> + __update_mqd(mm, mqd, q, MTYPE_UC, 0);
> }
>
> static int destroy_mqd(struct mqd_manager *mm, void *mqd,
> @@ -272,7 +259,7 @@ static int destroy_mqd(struct mqd_manager *mm, void *mqd,
> pipe_id, queue_id);
> }
>
> -static void uninit_mqd(struct mqd_manager *mm, void *mqd,
> +static void free_mqd(struct mqd_manager *mm, void *mqd,
> struct kfd_mem_obj *mqd_mem_obj)
> {
> kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
> @@ -309,61 +296,44 @@ static int get_wave_state(struct mqd_manager *mm, void *mqd,
> return 0;
> }
>
> -static int init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_hiq(struct mqd_manager *mm, void **mqd,
> + struct kfd_mem_obj *mqd_mem_obj, uint64_t *gart_addr,
> struct queue_properties *q)
> {
> struct vi_mqd *m;
> - int retval = init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
> -
> - if (retval != 0)
> - return retval;
> + init_mqd(mm, mqd, mqd_mem_obj, gart_addr, q);
>
> m = get_mqd(*mqd);
>
> m->cp_hqd_pq_control |= 1 << CP_HQD_PQ_CONTROL__PRIV_STATE__SHIFT |
> 1 << CP_HQD_PQ_CONTROL__KMD_QUEUE__SHIFT;
> -
> - return retval;
> }
>
> -static int update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_hiq(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct vi_mqd *m;
> - int retval = __update_mqd(mm, mqd, q, MTYPE_UC, 0);
> -
> - if (retval != 0)
> - return retval;
> + __update_mqd(mm, mqd, q, MTYPE_UC, 0);
>
> m = get_mqd(mqd);
> m->cp_hqd_vmid = q->vmid;
> - return retval;
> }
>
> -static int init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> - struct kfd_mem_obj **mqd_mem_obj, uint64_t *gart_addr,
> +static void init_mqd_sdma(struct mqd_manager *mm, void **mqd,
> + 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);
> -
> - return retval;
> + mm->update_mqd(mm, m, q);
> }
>
> static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> @@ -375,7 +345,7 @@ static int load_mqd_sdma(struct mqd_manager *mm, void *mqd,
> mms);
> }
>
> -static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> +static void update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> struct queue_properties *q)
> {
> struct vi_sdma_mqd *m;
> @@ -400,8 +370,6 @@ static int update_mqd_sdma(struct mqd_manager *mm, void *mqd,
> m->sdma_queue_id = q->sdma_queue_id;
>
> q->is_active = QUEUE_IS_ACTIVE(*q);
> -
> - return 0;
> }
>
> /*
> @@ -458,8 +426,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd;
> mqd->destroy_mqd = destroy_mqd;
> @@ -471,8 +440,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -483,8 +453,9 @@ 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->free_mqd = free_mqd;
> mqd->load_mqd = load_mqd;
> mqd->update_mqd = update_mqd_hiq;
> mqd->destroy_mqd = destroy_mqd;
> @@ -495,8 +466,9 @@ 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->free_mqd = free_mqd_hiq_sdma;
> mqd->load_mqd = load_mqd_sdma;
> mqd->update_mqd = update_mqd_sdma;
> mqd->destroy_mqd = destroy_mqd_sdma;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2019-06-06 20:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-06 18:25 [PATCH 4/6] drm/amdkfd: Separate mqd allocation and initialization Zeng, Oak
[not found] ` <1559845507-3052-1-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-06 18:25 ` [PATCH 5/6] drm/amdkfd: Fix a circular lock dependency Zeng, Oak
2019-06-06 18:25 ` [PATCH 6/6] drm/amdkfd: Fix sdma queue allocate race condition Zeng, Oak
[not found] ` <1559845507-3052-3-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-06 20:17 ` Kuehling, Felix
2019-06-06 20:13 ` Kuehling, Felix [this message]
-- strict thread matches above, loose matches on Subject: below --
2019-06-06 21:40 [PATCH 4/6] drm/amdkfd: Separate mqd allocation and initialization Zeng, Oak
[not found] ` <1559857240-6854-1-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-06 21:56 ` Kuehling, Felix
2019-06-05 16:06 [PATCH 1/6] drm/amdkfd: Only initialize sdma vm for sdma queues Zeng, Oak
[not found] ` <1559750793-16608-1-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-05 16:06 ` [PATCH 4/6] drm/amdkfd: Separate mqd allocation and initialization Zeng, Oak
[not found] ` <1559750793-16608-4-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-05 22:18 ` Kuehling, Felix
2019-06-04 2:52 [PATCH 1/6] drm/amdkfd: Only initialize sdma vm for sdma queues Zeng, Oak
[not found] ` <1559616755-13116-1-git-send-email-Oak.Zeng-5C7GfCeVMHo@public.gmane.org>
2019-06-04 2:52 ` [PATCH 4/6] drm/amdkfd: Separate mqd allocation and initialization Zeng, Oak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ed6ffa65-a53b-0ad5-0f57-e7813a36d710@amd.com \
--to=felix.kuehling-5c7gfcevmho@public.gmane.org \
--cc=Alex.Liu-5C7GfCeVMHo@public.gmane.org \
--cc=Chris.Freehill-5C7GfCeVMHo@public.gmane.org \
--cc=Oak.Zeng-5C7GfCeVMHo@public.gmane.org \
--cc=Yong.Zhao-5C7GfCeVMHo@public.gmane.org \
--cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.