[PATCH v2] drm/amdkfd: Remove GPU ID in GWS queue creation

[Kuehling, Felix]

On 2019-08-01 17:14, Greathouse, Joseph wrote:
> The gpu_id argument is not needed when enabling GWS on a queue.
> The queue can only be associated with one device, so the only
> possible situations for the call as previously defined were:
> 1) the gpu_id was for the device associated with the target queue
> and things worked as expected, or 2) the gpu_id was for a device
> not associated with the target queue and the request was undefined.
>
> In particular, the previous result of the undefined operation is
> that you would allocate the number of GWS entries available on the
> gpu_id device, even if the queue was on a device with a different
> number available. For example: a queue on a device without GWS
> capability, but the user passes in a gpu_id for a device with GWS.
> We would end up trying to allocate GWS on the device that does not
> support it.
>
> Rather than leaving this footgun around and making life more
> difficult for user space, we can instead grab the gpu_id from the
> target queue. The gpu_id argument being passed in is thus not
> needed.
>
> v2: Fixed styling, removed gpu_id since it never hit main release
>
> Change-Id: I73c032d7115b62505a6e7c48d2c060fc3c6ee915
> Signed-off-by: Joseph Greathouse <Joseph.Greathouse at amd.com>

See one style comment inline. With that fixed, this patch is 
Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>


> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 26 ++++++++++++++-----
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h         |  2 ++
>   .../amd/amdkfd/kfd_process_queue_manager.c    |  9 +++++++
>   include/uapi/linux/kfd_ioctl.h                |  3 +--
>   4 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index f91126f5f1be..b464ad1e2c4c 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1572,25 +1572,37 @@ static int kfd_ioctl_alloc_queue_gws(struct file *filep,
>   {
>   	int retval;
>   	struct kfd_ioctl_alloc_queue_gws_args *args = data;
> +	struct queue *q;
>   	struct kfd_dev *dev;
>   
>   	if (!hws_gws_support)
>   		return -ENODEV;
>   
> -	dev = kfd_device_by_id(args->gpu_id);
> -	if (!dev) {
> -		pr_debug("Could not find gpu id 0x%x\n", args->gpu_id);
> -		return -ENODEV;
> +	mutex_lock(&p->mutex);
> +	q = pqm_get_user_queue(&p->pqm, args->queue_id);
> +
> +	if (q) {
> +		dev = q->device;
> +	}
> +	else {

The closing brace } and "else" should be on the same line.

Thanks,
   Felix


> +		retval = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS) {
> +		retval = -ENODEV;
> +		goto out_unlock;
>   	}
> -	if (dev->dqm->sched_policy == KFD_SCHED_POLICY_NO_HWS)
> -		return -ENODEV;
>   
> -	mutex_lock(&p->mutex);
>   	retval = pqm_set_gws(&p->pqm, args->queue_id, args->num_gws ? dev->gws : NULL);
>   	mutex_unlock(&p->mutex);
>   
>   	args->first_gws = 0;
>   	return retval;
> +
> +out_unlock:
> +	mutex_unlock(&p->mutex);
> +	return retval;
>   }
>   
>   static int kfd_ioctl_get_dmabuf_info(struct file *filep,
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index aa7bf20d20f8..9b9a8da187c8 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -915,6 +915,8 @@ 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);
> +struct queue *pqm_get_user_queue(struct process_queue_manager *pqm,
> +						unsigned int qid);
>   int pqm_get_wave_state(struct process_queue_manager *pqm,
>   		       unsigned int qid,
>   		       void __user *ctl_stack,
> 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 7e6c3ee82f5b..7a61a5b09ed8 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
> @@ -473,6 +473,15 @@ struct kernel_queue *pqm_get_kernel_queue(
>   	return NULL;
>   }
>   
> +struct queue *pqm_get_user_queue(struct process_queue_manager *pqm,
> +					unsigned int qid)
> +{
> +	struct process_queue_node *pqn;
> +
> +	pqn = get_queue_by_qid(pqm, qid);
> +	return pqn ? pqn->q : NULL;
> +}
> +
>   int pqm_get_wave_state(struct process_queue_manager *pqm,
>   		       unsigned int qid,
>   		       void __user *ctl_stack,
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 070d1bc7e725..4f6676428c5c 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -412,17 +412,16 @@ struct kfd_ioctl_unmap_memory_from_gpu_args {
>   
>   /* Allocate GWS for specific queue
>    *
> - * @gpu_id:      device identifier
>    * @queue_id:    queue's id that GWS is allocated for
>    * @num_gws:     how many GWS to allocate
>    * @first_gws:   index of the first GWS allocated.
>    *               only support contiguous GWS allocation
>    */
>   struct kfd_ioctl_alloc_queue_gws_args {
> -	__u32 gpu_id;		/* to KFD */
>   	__u32 queue_id;		/* to KFD */
>   	__u32 num_gws;		/* to KFD */
>   	__u32 first_gws;	/* from KFD */
> +	__u32 pad;
>   };
>   
>   struct kfd_ioctl_get_dmabuf_info_args {