Re: [PATCH v5 02/10] drm/amdgpu: add usermode queue base code

Shashank Sharma <shashank.sharma@xxxxxxx> · Thu, 6 Jul 2023 18:52:49 +0200

On 06/07/2023 18:36, Alex Deucher wrote:
On Thu, Jul 6, 2023 at 8:36 AM Shashank Sharma <shashank.sharma@xxxxxxx> wrote:
This patch adds skeleton code for amdgpu usermode queue.
It contains:
- A new files with init functions of usermode queues.
- A queue context manager in driver private data.

V1: Worked on design review comments from RFC patch series:
(https://patchwork.freedesktop.org/series/112214/)
- 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:
  - Reformatted code, split the big patch into two

V3:
- Integration with doorbell manager

V4:
- Align the structure member names to the largest member's column
   (Luben)
- Added SPDX license (Luben)

V5:
- Do not add amdgpu.h in amdgpu_userqueue.h (Christian).
- Move struct amdgpu_userq_mgr into amdgpu_userqueue.h (Christian).

Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Christian Koenig <christian.koenig@xxxxxxx>
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/Makefile           |  2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       |  6 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 40 ++++++++++++
  .../gpu/drm/amd/include/amdgpu_userqueue.h    | 62 +++++++++++++++++++
  6 files changed, 113 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_userqueue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 415a7fa395c4..4b9bae995094 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -216,6 +216,8 @@ amdgpu-y += \
  # add amdkfd interfaces
  amdgpu-y += amdgpu_amdkfd.o

+# add usermode queue
+amdgpu-y += amdgpu_userqueue.o

  ifneq ($(CONFIG_HSA_AMD),)
  AMDKFD_PATH := ../amdkfd
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 02b827785e39..fab842138cd5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -107,6 +107,7 @@
  #include "amdgpu_fdinfo.h"
  #include "amdgpu_mca.h"
  #include "amdgpu_ras.h"
+#include "amdgpu_userqueue.h"

  #define MAX_GPU_INSTANCE               16

@@ -463,6 +464,7 @@ struct amdgpu_fpriv {
         struct mutex            bo_list_lock;
         struct idr              bo_list_handles;
         struct amdgpu_ctx_mgr   ctx_mgr;
+       struct amdgpu_userq_mgr userq_mgr;
  };

  int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b1ca1ab6d6ad..4c5e44d41652 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -50,6 +50,7 @@
  #include "amdgpu_ras.h"
  #include "amdgpu_xgmi.h"
  #include "amdgpu_reset.h"
+#include "amdgpu_userqueue.h"

  /*
   * KMS wrapper.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 0efb38539d70..68e5375b648b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -44,6 +44,7 @@
  #include "amdgpu_display.h"
  #include "amdgpu_ras.h"
  #include "amd_pcie.h"
+#include "amdgpu_userqueue.h"

  void amdgpu_unregister_gpu_instance(struct amdgpu_device *adev)
  {
@@ -1234,6 +1235,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)

         amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);

+       r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);
+       if (r)
+               DRM_WARN("Can't setup usermode queues, use legacy workload submission only\n");
+
         file_priv->driver_priv = fpriv;
         goto out_suspend;

@@ -1301,6 +1306,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,

         amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
         amdgpu_vm_fini(adev, &fpriv->vm);
+       amdgpu_userq_mgr_fini(&fpriv->userq_mgr);

         if (pasid)
                 amdgpu_pasid_free_delayed(pd->tbo.base.resv, pasid);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
new file mode 100644
index 000000000000..effc0c7c02cf
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#include "amdgpu.h"
+
+int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct amdgpu_device *adev)
+{
+       mutex_init(&userq_mgr->userq_mutex);
+       idr_init_base(&userq_mgr->userq_idr, 1);
+       userq_mgr->adev = adev;
+
+       return 0;
+}
+
+void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr)
+{
+       idr_destroy(&userq_mgr->userq_idr);
+       mutex_destroy(&userq_mgr->userq_mutex);
+}
diff --git a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
new file mode 100644
index 000000000000..79ffa131a514
--- /dev/null
+++ b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef AMDGPU_USERQUEUE_H_
+#define AMDGPU_USERQUEUE_H_
+
+#define AMDGPU_MAX_USERQ_COUNT 512
+
+struct amdgpu_mqd_prop;
+
+struct amdgpu_usermode_queue {
+       int                     queue_type;
+       uint64_t                doorbell_handle;
+       uint64_t                doorbell_index;
+       uint64_t                flags;
+       struct amdgpu_mqd_prop  *userq_prop;
+       struct amdgpu_userq_mgr *userq_mgr;
+       struct amdgpu_vm        *vm;
+};
+
+struct amdgpu_userq_funcs {
+       int (*mqd_create)(struct amdgpu_userq_mgr *uq_mgr,
+                         struct drm_amdgpu_userq_in *args,
+                         struct amdgpu_usermode_queue *queue);
+       void (*mqd_destroy)(struct amdgpu_userq_mgr *uq_mgr,
+                           struct amdgpu_usermode_queue *uq);
+};
+
+/* Usermode queues for gfx */
+struct amdgpu_userq_mgr {
+       struct idr                      userq_idr;
+       struct mutex                    userq_mutex;
+       struct amdgpu_device            *adev;
+       const struct amdgpu_userq_funcs *userq_funcs[AMDGPU_HW_IP_NUM];
Why did we decide to put these in the userq_mgr rather than having
them in adev?  I tried to find the original v1 thread.  It just seems
like extra work to assign a bunch of pointers every time we create a
userq_mgr.  I don't see a case where we would ever want them to be
different per userq_mgr instance.  It also keeps all of the IP
specific knowledge in the IP specific code.  E.g., if some IP only
supports this for specific versions, we could assign the pointers to
adev in that IP's code rather than adding a bunch of logic to the
generic userq code to know which IP versions may or may not support
this.

So far we have been discussing on why not to keep this whole structure 
in adev, which was as discussed due to a previous review comment (want 
to make this data specific to a session between open() and close(), and 
also not to overpopulate adev), but we do not have a specific reason to 
keep these function ptrs in uq_mgr. I can move just these function 
pointers in adev and we can initialize this once during the IP init, the 
small cost to pay would be right now all the things related to usermode 
queue is at this one place, in the other case it would be scattered 
across two (Which can be ignored, if we prefer :))

- Shashank

Alex


+};
+
+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);
+
+#endif
--
2.40.1







