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@xxxxxxx>
See one style comment inline. With that fixed, this patch is
Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> ---
> 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 {
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
