Re: [PATCH v2 3/9] drm/amdgpu: add new IOCTL for usermode queue

Thu, 16 Feb 2023 02:15:52 -0800 

Hey Christian,

On 16/02/2023 08:23, Christian König wrote:

Am 15.02.23 um 19:43 schrieb Shashank Sharma:

From: Shashank Sharma <contactshashanksha...@gmail.com>

This patch adds:
- A new IOCTL function to create and destroy
- A new structure to keep all the user queue data in one place.
- A function to generate unique index for the queue.

V1: Worked on review comments from RFC patch series:
   - Alex: Keep a list of queues, instead of single queue per process.
   - Christian: Use the queue manager instead of global ptrs,
            Don't keep the queue structure in amdgpu_ctx

V2: Worked on review comments:
  - Christian:
    - Formatting of text
    - There is no need for queuing of userqueues, with idr in place
  - Alex:
    - Remove use_doorbell, its unnecessary
    - Reuse amdgpu_mqd_props for saving mqd fields

  - Code formatting and re-arrangement

Cc: Alex Deucher <alexander.deuc...@amd.com>
Cc: Christian Koenig <christian.koe...@amd.com>
Signed-off-by: Shashank Sharma <contactshashanksha...@gmail.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 114 ++++++++++++++++++
  .../gpu/drm/amd/include/amdgpu_userqueue.h    |   2 +
  3 files changed, 117 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
index 2d6bcfd727c8..229976a2d0e7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2749,6 +2749,7 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), +    DRM_IOCTL_DEF_DRV(AMDGPU_USERQ, amdgpu_userq_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), 
  };
    static const struct drm_driver amdgpu_kms_driver = {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c 
index 13e1eebc1cb6..ecf31d86f3de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -22,6 +22,120 @@
   */
    #include "amdgpu.h"
+#include "amdgpu_vm.h"
+#include "amdgpu_userqueue.h"
+
+static inline int
+amdgpu_userqueue_index(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_queue *queue) 
+{
+    return idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ, GFP_KERNEL); 
+}
+
+static inline void
+amdgpu_userqueue_free_index(struct amdgpu_userq_mgr *uq_mgr, int queue_id) 
+{
+    idr_remove(&uq_mgr->userq_idr, queue_id);
+}
+
+static struct amdgpu_usermode_queue *
+amdgpu_userqueue_find(struct amdgpu_userq_mgr *uq_mgr, int qid)
+{
+    return idr_find(&uq_mgr->userq_idr, qid);
+}
+
+static int amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args) 
+{
+    struct amdgpu_usermode_queue *queue;
+    struct amdgpu_fpriv *fpriv = filp->driver_priv;
+    struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr;
+    struct drm_amdgpu_userq_mqd *mqd_in = &args->in.mqd;
+    int r;
+
+    /* Do we have support userqueues for this IP ? */
+    if (!uq_mgr->userq_funcs[mqd_in->ip_type]) {
+        DRM_ERROR("GFX User queues not supported for this IP: %d\n", mqd_in->ip_type); 
+        return -EINVAL;
+    }
+
+    queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
+    if (!queue) {
+        DRM_ERROR("Failed to allocate memory for queue\n");
+        return -ENOMEM;
+    }
+
+    mutex_lock(&uq_mgr->userq_mutex);
+    queue->userq_prop.wptr_gpu_addr = mqd_in->wptr_va;
+    queue->userq_prop.rptr_gpu_addr = mqd_in->rptr_va;
+    queue->userq_prop.queue_size = mqd_in->queue_size;
+    queue->userq_prop.hqd_base_gpu_addr = mqd_in->queue_va;
+    queue->userq_prop.queue_size = mqd_in->queue_size;
+
+    queue->doorbell_handle = mqd_in->doorbell_handle;
+    queue->queue_type = mqd_in->ip_type;
+    queue->flags = mqd_in->flags;
+    queue->vm = &fpriv->vm;
+    queue->shadow_ctx_gpu_addr = mqd_in->shadow_va;



+    queue->queue_id = amdgpu_userqueue_index(uq_mgr, queue);
+    if (queue->queue_id < 0) {
+        DRM_ERROR("Failed to allocate a queue id\n");
+        r = queue->queue_id;
+        goto free_queue;
+    }
Don't keep the assigned id inside the queue structure. This is only used as handle between userspace and kernel and not useful inside the kernel otherwise. 

This prevents people from using it in hw communication.


Noted,

- Shashank



Apart from that this looks good to me,
Christian.


+
+    args->out.queue_id = queue->queue_id;
+    args->out.flags = 0;
+    mutex_unlock(&uq_mgr->userq_mutex);
+    return 0;
+
+free_queue:
+    mutex_unlock(&uq_mgr->userq_mutex);
+    kfree(queue);
+    return r;
+}
+
+static void amdgpu_userqueue_destroy(struct drm_file *filp, int queue_id) 
+{
+    struct amdgpu_fpriv *fpriv = filp->driver_priv;
+    struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr;
+    struct amdgpu_usermode_queue *queue;
+
+    queue = amdgpu_userqueue_find(uq_mgr, queue_id);
+    if (!queue) {
+        DRM_DEBUG_DRIVER("Invalid queue id to destroy\n");
+        return;
+    }
+
+    mutex_lock(&uq_mgr->userq_mutex);
+    amdgpu_userqueue_free_index(uq_mgr, queue->queue_id);
+    mutex_unlock(&uq_mgr->userq_mutex);
+    kfree(queue);
+}
+
+int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
+               struct drm_file *filp)
+{
+    union drm_amdgpu_userq *args = data;
+    int r = 0;
+
+    switch (args->in.op) {
+    case AMDGPU_USERQ_OP_CREATE:
+        r = amdgpu_userqueue_create(filp, args);
+        if (r)
+            DRM_ERROR("Failed to create usermode queue\n");
+        break;
+
+    case AMDGPU_USERQ_OP_FREE:
+        amdgpu_userqueue_destroy(filp, args->in.queue_id);
+        break;
+
+    default:
+        DRM_ERROR("Invalid user queue op specified: %d\n", args->in.op); 
+        return -EINVAL;
+    }
+
+    return r;
+}
+
    int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct amdgpu_device *adev) 
  {
diff --git a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h 
index 7eeb8c9e6575..7625a862b1fc 100644
--- a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
+++ b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
@@ -42,6 +42,8 @@ struct amdgpu_userq_funcs {
      void (*mqd_destroy)(struct amdgpu_userq_mgr *, struct amdgpu_usermode_queue *); 
  };
  +int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); 
+
  int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct amdgpu_device *adev); 
    void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr);

Reply via email to