Re: [PATCH 5/7] drm/amdkfd: Add function to set queue gws

["Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>]

On 2019-05-22 11:51 a.m., Zeng, Oak wrote:
> Add functions in process queue manager to
> set/get queue gws. Also set process's number
> of gws used. Currently only one queue in
> process can use and use all gws.
>
> Change-Id: I03e480c8692db3eabfc3a188cce8904d5d962ab7
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h              |  6 +++
>   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 58 ++++++++++++++++++++++
>   2 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 5969e37..40a5c67 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -454,6 +454,9 @@ struct queue_properties {
>    *
>    * @device: The kfd device that created this queue.
>    *
> + * @gws: Pointing to gws kgd_mem if this is a gws control queue; NULL
> + * otherwise.
> + *
>    * This structure represents user mode compute queues.
>    * It contains all the necessary data to handle such queues.
>    *
> @@ -475,6 +478,7 @@ struct queue {
>   
>   	struct kfd_process	*process;
>   	struct kfd_dev		*device;
> +	void *gws;
>   };
>   
>   /*
> @@ -868,6 +872,8 @@ int pqm_update_queue(struct process_queue_manager *pqm, unsigned int qid,
>   			struct queue_properties *p);
>   int pqm_set_cu_mask(struct process_queue_manager *pqm, unsigned int qid,
>   			struct queue_properties *p);
> +int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
> +			void *gws);
>   struct kernel_queue *pqm_get_kernel_queue(struct process_queue_manager *pqm,
>   						unsigned int qid);
>   int pqm_get_wave_state(struct process_queue_manager *pqm,
> 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 e652e25..a78494d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -26,6 +26,7 @@
>   #include "kfd_device_queue_manager.h"
>   #include "kfd_priv.h"
>   #include "kfd_kernel_queue.h"
> +#include "amdgpu_amdkfd.h"
>   
>   static inline struct process_queue_node *get_queue_by_qid(
>   			struct process_queue_manager *pqm, unsigned int qid)
> @@ -74,6 +75,57 @@ void kfd_process_dequeue_from_device(struct kfd_process_device *pdd)
>   	pdd->already_dequeued = true;
>   }
>   
> +int pqm_set_gws(struct process_queue_manager *pqm, unsigned int qid,
> +			void *gws)
> +{
> +	struct kfd_dev *dev = NULL;
> +	struct process_queue_node *pqn;
> +	struct kfd_process_device *pdd;
> +
> +	pqn = get_queue_by_qid(pqm, qid);
> +	if (!pqn) {
> +		pr_err("Queue id does not match any known queue\n");
> +		return -EINVAL;
> +	}
> +
> +	if (pqn->q)
> +		dev = pqn->q->device;
> +	if (WARN_ON(!dev))
> +		return -ENODEV;
> +
> +	pdd = kfd_get_process_device_data(dev, pqm->process);
> +	if (!pdd) {
> +		pr_err("Process device data doesn't exist\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Only allow one queue per process can have GWS assigned */
> +	list_for_each_entry(pqn, &pqm->queues, process_queue_list) {
> +		if (pqn->q && pqn->q->gws)
> +			return -EINVAL;
> +	}
> +
> +	pqn->q->gws = gws;
> +	pdd->qpd.num_gws = gws ? amdgpu_amdkfd_get_num_gws(dev->kgd) : 0;
> +
> +	return pqn->q->device->dqm->ops.update_queue(pqn->q->device->dqm,
> +							pqn->q);

When pqm_set_gws is called during pqm_destroy_queue, this is 
unnecessary. It would result in two preemptions and runlist updates 
every time a queue is destroyed. See below.


> +}
> +
> +static void *pqm_get_gws(struct process_queue_manager *pqm, unsigned int qid)
> +{
> +	struct process_queue_node *pqn;
> +
> +	pqn = get_queue_by_qid(pqm, qid);
> +	if (!pqn) {
> +		pr_debug("No queue %d exists for get gws operation\n", qid);
> +		return NULL;
> +	}
> +
> +	return pqn->q->gws;
> +}
> +
> +
>   void kfd_process_dequeue_from_all_devices(struct kfd_process *p)
>   {
>   	struct kfd_process_device *pdd;
> @@ -313,6 +365,12 @@ int pqm_destroy_queue(struct process_queue_manager *pqm, unsigned int qid)
>   		return -1;
>   	}
>   
> +	if (pqm_get_gws(pqm, qid)) {

All pqm_get_gws really does an extra lookup of the pqn, which you 
already know here. Instead, could you just replace this with:

     if (pqn->q->gws) ...

And remove pqm_get_gws.

> +		amdgpu_amdkfd_remove_gws_from_process(pqm->process->kgd_process_info,
> +				pqm_get_gws(pqm, qid));
> +		pqm_set_gws(pqm, qid, NULL);

If you call pqm_set_gws here, it will unnecessarily cause a preemption 
and runlist update. It also does more lookups of pdd and pqn, both of 
which you already know here. You don't really need to update the queue 
that you're about to destroy. All you really need to do here is update 
pdd->qpd.num_gws. You can do that without calling pqm_set_gws.

Regards,
   Felix

> +	}
> +
>   	if (pqn->kq) {
>   		/* destroy kernel queue (DIQ) */
>   		dqm = pqn->kq->dev->dqm;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx