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

Submitted by Greathouse, Joseph on Aug. 1, 2019, 9:14 p.m.

Details

Message ID 20190801211417.4781-1-Joseph.Greathouse@amd.com
State New
Headers show
Series "drm/amdkfd: Remove GPU ID in GWS queue creation" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Greathouse, Joseph Aug. 1, 2019, 9:14 p.m.
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@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(-)

Patch hide | download patch | download mbox

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 {
+		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 {

Comments

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@amd.com>


See one style comment inline. With that fixed, this patch is 
Reviewed-by: Felix Kuehling <Felix.Kuehling@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 {