Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-04 Thread Shashank Sharma

Hello Jiadong,

Please check the first patch of the series, where we added the UAPI


- Shashank

On 04/01/2023 09:55, Zhu, Jiadong wrote:

[AMD Official Use Only - General]

Hi Shashank,

I don't find how amdgpu_userq_ioctl is called, shall 
DRM_IOCTL_DEF_DRV(amdgpu_userq_ioctl...) be added somewhere to expose the ioctl?

Thanks,
Jiadong

-Original Message-
From: amd-gfx  On Behalf Of Shashank 
Sharma
Sent: Saturday, December 24, 2022 3:37 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Sharma, Shashank ; 
Koenig, Christian ; Yadav, Arvind ; Paneer 
Selvam, Arunpravin 
Subject: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

This patch adds skeleton code for usermode queue creation. It typically 
contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
 struct amdgpu_mqd_prop *p);
  };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
 boolenable_mes_kiq;
 struct amdgpu_mes   mes;
 struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

 /* df */
 struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
 unsigned long   ras_counter_ce;
 unsigned long   ras_counter_ue;
 uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;
  };

  struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a,
+sizeof(__u64)))
+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev) {
+int index;
+struct 

RE: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-04 Thread Zhu, Jiadong
[AMD Official Use Only - General]

Hi Shashank,

I don't find how amdgpu_userq_ioctl is called, shall 
DRM_IOCTL_DEF_DRV(amdgpu_userq_ioctl...) be added somewhere to expose the ioctl?

Thanks,
Jiadong

-Original Message-
From: amd-gfx  On Behalf Of Shashank 
Sharma
Sent: Saturday, December 24, 2022 3:37 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander ; Sharma, Shashank 
; Koenig, Christian ; Yadav, 
Arvind ; Paneer Selvam, Arunpravin 

Subject: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

This patch adds skeleton code for usermode queue creation. It typically 
contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
 drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
 .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
 5 files changed, 246 insertions(+)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
 create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
struct amdgpu_mqd_prop *p);
 };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
 #define AMDGPU_RESET_MAGIC_NUM 64
 #define AMDGPU_MAX_DF_PERFMONS 4
 #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
boolenable_mes_kiq;
struct amdgpu_mes   mes;
struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

/* df */
struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
unsigned long   ras_counter_ce;
unsigned long   ras_counter_ue;
uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;
 };

 struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a,
+sizeof(__u64)))
+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev) {
+int index;
+struct amdgpu_userq_globals *uqg = >userq;
+
+index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
+return index;
+}
+
+static void

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Christian König

Am 03.01.23 um 15:34 schrieb Alex Deucher:

On Tue, Jan 3, 2023 at 4:35 AM Christian König
 wrote:

Am 03.01.23 um 10:22 schrieb Shashank Sharma:

On 03/01/2023 10:15, Christian König wrote:

Am 03.01.23 um 10:12 schrieb Shashank Sharma:

On 02/01/2023 13:39, Christian König wrote:

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

 /* df */
   struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
   unsigned longras_counter_ce;
   unsigned longras_counter_ue;
   uint32_tstable_pstate;
+struct amdgpu_usermode_queue*userq;

Why should we have this in the ctx here???

We are allocating a few things dynamically for the queue, which
would be valid until we destroy this queue. Also we need to save
this queue

container at some place for the destroy function,  and I thought
it would make sense to keep this with the context ptr, as this is
how we are

identifying the incoming request.

I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely
related to anything the user queues should be doing.

Please completely drop that relationship and don't use any of the
ctx object stuff in the user queue code.


Historically the workload submission always came with a context (due
to CS IOCTL), so we thought it would make sense to still have its
relevance in the new workload submission method. Would you prefer
this new submission to be independent of AMDGPU context ?

Well not prefer, the point is that this doesn't make any sense at all.

See the amdgpu_ctx object contains the resulting fence pointers for
the CS IOCTL as well as information necessary for the CS IOCTL to
work (e.g. scheduler entities etc...).

I don't see how anything from that stuff would be useful for the MES
or user queues.

Christian.


I am getting your point, and it makes sense as well. But in such
scenario, we might have to create something parallel to
AMDGPU_USERQ_CTX which is doing very much the same.

We can still do it to make a logically separate entity, but any
suggestions on where to keep this udev_ctx ptr (if not in adev, as
well as not ctx) ?


Take a look at the amdgpu_ctx_mgr object with the mutex and the idr and
how this is embedded into the amdgpu_fpriv object. It should become
pretty clear from there on.

I don't think we need an userq_ctx or similar, each userq should be an
independent object. What we need is an userq_mgr object which holds the
collection of all the useq objects the client application has created
through it's fpriv connection to the driver.

Don't we want to associate the queues to a ctx for guilty tracking
purposes when there is a hang?


Nope, absolutely not.

The hang detection around the context was just another design bug we 
inherited from the windows driver.


What we should do instead is to use the error field in the dma_fence 
object just like every other driver and component does.


Christian.



Alex


Regards,
Christian.


- Shashank



- Shashank



Christian.


- Shashank


Regards,
Christian.


   };
 struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a,

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Alex Deucher
On Tue, Jan 3, 2023 at 4:35 AM Christian König
 wrote:
>
> Am 03.01.23 um 10:22 schrieb Shashank Sharma:
> >
> > On 03/01/2023 10:15, Christian König wrote:
> >> Am 03.01.23 um 10:12 schrieb Shashank Sharma:
> >>>
> >>> On 02/01/2023 13:39, Christian König wrote:
>  Hi Shashank,
> 
>  Am 26.12.22 um 11:41 schrieb Shashank Sharma:
> > [SNIP]
> >>> /* df */
> >>>   struct amdgpu_dfdf;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> >>> index 0fa0e56daf67..f7413859b14f 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> >>> @@ -57,6 +57,7 @@ struct amdgpu_ctx {
> >>>   unsigned longras_counter_ce;
> >>>   unsigned longras_counter_ue;
> >>>   uint32_tstable_pstate;
> >>> +struct amdgpu_usermode_queue*userq;
> >>
> >> Why should we have this in the ctx here???
> >
> > We are allocating a few things dynamically for the queue, which
> > would be valid until we destroy this queue. Also we need to save
> > this queue
> >
> > container at some place for the destroy function,  and I thought
> > it would make sense to keep this with the context ptr, as this is
> > how we are
> >
> > identifying the incoming request.
> 
>  I have absolutely no idea how you end up with that design.
> 
>  The ctx object is the CS IOCTL context, that is not even remotely
>  related to anything the user queues should be doing.
> 
>  Please completely drop that relationship and don't use any of the
>  ctx object stuff in the user queue code.
> 
> >>> Historically the workload submission always came with a context (due
> >>> to CS IOCTL), so we thought it would make sense to still have its
> >>> relevance in the new workload submission method. Would you prefer
> >>> this new submission to be independent of AMDGPU context ?
> >>
> >> Well not prefer, the point is that this doesn't make any sense at all.
> >>
> >> See the amdgpu_ctx object contains the resulting fence pointers for
> >> the CS IOCTL as well as information necessary for the CS IOCTL to
> >> work (e.g. scheduler entities etc...).
> >>
> >> I don't see how anything from that stuff would be useful for the MES
> >> or user queues.
> >>
> >> Christian.
> >
> >
> > I am getting your point, and it makes sense as well. But in such
> > scenario, we might have to create something parallel to
> > AMDGPU_USERQ_CTX which is doing very much the same.
> >
> > We can still do it to make a logically separate entity, but any
> > suggestions on where to keep this udev_ctx ptr (if not in adev, as
> > well as not ctx) ?
>
>
> Take a look at the amdgpu_ctx_mgr object with the mutex and the idr and
> how this is embedded into the amdgpu_fpriv object. It should become
> pretty clear from there on.
>
> I don't think we need an userq_ctx or similar, each userq should be an
> independent object. What we need is an userq_mgr object which holds the
> collection of all the useq objects the client application has created
> through it's fpriv connection to the driver.

Don't we want to associate the queues to a ctx for guilty tracking
purposes when there is a hang?

Alex

>
> Regards,
> Christian.
>
> >
> > - Shashank
> >
> >
> >>
> >>>
> >>> - Shashank
> >>>
> >>>
>  Christian.
> 
> >
> > - Shashank
> >
> >>
> >> Regards,
> >> Christian.
> >>
> >>>   };
> >>> struct amdgpu_ctx_mgr {
> >>> 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 ..3b6e8f75495c
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> >>> @@ -0,0 +1,187 @@
> >>> +/*
> >>> + * Copyright 2022 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
> >>> 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Christian König

Am 03.01.23 um 10:22 schrieb Shashank Sharma:


On 03/01/2023 10:15, Christian König wrote:

Am 03.01.23 um 10:12 schrieb Shashank Sharma:


On 02/01/2023 13:39, Christian König wrote:

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which 
would be valid until we destroy this queue. Also we need to save 
this queue


container at some place for the destroy function,  and I thought 
it would make sense to keep this with the context ptr, as this is 
how we are


identifying the incoming request.


I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely 
related to anything the user queues should be doing.


Please completely drop that relationship and don't use any of the 
ctx object stuff in the user queue code.


Historically the workload submission always came with a context (due 
to CS IOCTL), so we thought it would make sense to still have its 
relevance in the new workload submission method. Would you prefer 
this new submission to be independent of AMDGPU context ?


Well not prefer, the point is that this doesn't make any sense at all.

See the amdgpu_ctx object contains the resulting fence pointers for 
the CS IOCTL as well as information necessary for the CS IOCTL to 
work (e.g. scheduler entities etc...).


I don't see how anything from that stuff would be useful for the MES 
or user queues.


Christian.



I am getting your point, and it makes sense as well. But in such 
scenario, we might have to create something parallel to 
AMDGPU_USERQ_CTX which is doing very much the same.


We can still do it to make a logically separate entity, but any 
suggestions on where to keep this udev_ctx ptr (if not in adev, as 
well as not ctx) ?



Take a look at the amdgpu_ctx_mgr object with the mutex and the idr and 
how this is embedded into the amdgpu_fpriv object. It should become 
pretty clear from there on.


I don't think we need an userq_ctx or similar, each userq should be an 
independent object. What we need is an userq_mgr object which holds the 
collection of all the useq objects the client application has created 
through it's fpriv connection to the driver.


Regards,
Christian.



- Shashank






- Shashank



Christian.



- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))

+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, 
GFP_KERNEL);

+    return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, 
struct amdgpu_usermode_queue *queue)

+{
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Shashank Sharma



On 02/01/2023 14:53, Christian König wrote:

Am 29.12.22 um 18:41 schrieb Alex Deucher:
On Fri, Dec 23, 2022 at 2:37 PM Shashank Sharma 
 wrote:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 
++

  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 
drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h


diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile

index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
 struct amdgpu_mqd_prop *p);
  };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
 bool    enable_mes_kiq;
 struct amdgpu_mes   mes;
 struct amdgpu_mqd mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

 /* df */
 struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
 unsigned long   ras_counter_ce;
 unsigned long   ras_counter_ue;
 uint32_t    stable_pstate;
+   struct amdgpu_usermode_queue    *userq;

There can be multiple queues per context.  We should make this a list.


  };

  struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))


You seem to have a very very big misunderstanding here.

access_ok() is used for CPU pointer validation, but this here are 
pointers into the GPUVM address space. This is something completely 
different!


Thanks, It seems like there is a misunderstanding in my side on 
definition of these input parameters, let me follow up.


- Shashank




Regards,
Christian.


+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Shashank Sharma



On 03/01/2023 10:15, Christian König wrote:

Am 03.01.23 um 10:12 schrieb Shashank Sharma:


On 02/01/2023 13:39, Christian König wrote:

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which 
would be valid until we destroy this queue. Also we need to save 
this queue


container at some place for the destroy function,  and I thought it 
would make sense to keep this with the context ptr, as this is how 
we are


identifying the incoming request.


I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely 
related to anything the user queues should be doing.


Please completely drop that relationship and don't use any of the 
ctx object stuff in the user queue code.


Historically the workload submission always came with a context (due 
to CS IOCTL), so we thought it would make sense to still have its 
relevance in the new workload submission method. Would you prefer 
this new submission to be independent of AMDGPU context ?


Well not prefer, the point is that this doesn't make any sense at all.

See the amdgpu_ctx object contains the resulting fence pointers for 
the CS IOCTL as well as information necessary for the CS IOCTL to work 
(e.g. scheduler entities etc...).


I don't see how anything from that stuff would be useful for the MES 
or user queues.


Christian.



I am getting your point, and it makes sense as well. But in such 
scenario, we might have to create something parallel to AMDGPU_USERQ_CTX 
which is doing very much the same.


We can still do it to make a logically separate entity, but any 
suggestions on where to keep this udev_ctx ptr (if not in adev, as well 
as not ctx) ?


- Shashank






- Shashank



Christian.



- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))

+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, 
GFP_KERNEL);

+    return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)

+{
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int
+amdgpu_userqueue_validate_input(struct amdgpu_device *adev, 
struct drm_amdgpu_userq_mqd *mqd_in)

+{
+    if (mqd_in->queue_va == 0 || mqd_in->doorbell_handle == 0 || 
mqd_in->doorbell_offset == 0) {

+    DRM_ERROR("Invalid queue object address\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->queue_size == 0 || mqd_in->rptr_va == 0 || 
mqd_in->wptr_va == 0) {

+    DRM_ERROR("Invalid queue object value\n");
+    return -EINVAL;
+    }
+
+ 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Shashank Sharma



On 29/12/2022 18:41, Alex Deucher wrote:

On Fri, Dec 23, 2022 at 2:37 PM Shashank Sharma  wrote:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
 struct amdgpu_mqd_prop *p);
  };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
 boolenable_mes_kiq;
 struct amdgpu_mes   mes;
 struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

 /* df */
 struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
 unsigned long   ras_counter_ce;
 unsigned long   ras_counter_ue;
 uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;

There can be multiple queues per context.  We should make this a list.


Noted, will change it into a queue. We are still in discussion (in 
another thread) if we have to move this from context to some place else.



  };

  struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))
+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+int index;
+struct amdgpu_userq_globals *uqg = >userq;
+
+index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
+return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)
+{
+struct amdgpu_userq_globals *uqg = >userq;
+
+ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Christian König

Am 03.01.23 um 10:12 schrieb Shashank Sharma:


On 02/01/2023 13:39, Christian König wrote:

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which 
would be valid until we destroy this queue. Also we need to save 
this queue


container at some place for the destroy function,  and I thought it 
would make sense to keep this with the context ptr, as this is how 
we are


identifying the incoming request.


I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely 
related to anything the user queues should be doing.


Please completely drop that relationship and don't use any of the ctx 
object stuff in the user queue code.


Historically the workload submission always came with a context (due 
to CS IOCTL), so we thought it would make sense to still have its 
relevance in the new workload submission method. Would you prefer this 
new submission to be independent of AMDGPU context ?


Well not prefer, the point is that this doesn't make any sense at all.

See the amdgpu_ctx object contains the resulting fence pointers for the 
CS IOCTL as well as information necessary for the CS IOCTL to work (e.g. 
scheduler entities etc...).


I don't see how anything from that stuff would be useful for the MES or 
user queues.


Christian.



- Shashank



Christian.



- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))

+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, 
GFP_KERNEL);

+    return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)

+{
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int
+amdgpu_userqueue_validate_input(struct amdgpu_device *adev, 
struct drm_amdgpu_userq_mqd *mqd_in)

+{
+    if (mqd_in->queue_va == 0 || mqd_in->doorbell_handle == 0 || 
mqd_in->doorbell_offset == 0) {

+    DRM_ERROR("Invalid queue object address\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->queue_size == 0 || mqd_in->rptr_va == 0 || 
mqd_in->wptr_va == 0) {

+    DRM_ERROR("Invalid queue object value\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->ip_type < AMDGPU_HW_IP_GFX || mqd_in->ip_type >= 
AMDGPU_HW_IP_NUM) {

+    DRM_ERROR("Invalid HW IP type 0x%x\n", mqd_in->ip_type);
+    return -EINVAL;
+    }
+
+    if (!CHECK_ACCESS(mqd_in->queue_va) || 
!CHECK_ACCESS(mqd_in->rptr_va) ||

+    !CHECK_ACCESS(mqd_in->wptr_va)) {
+    DRM_ERROR("Invalid mapping of queue ptrs, access 
error\n");

+  

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-03 Thread Shashank Sharma



On 02/01/2023 13:39, Christian König wrote:

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which would 
be valid until we destroy this queue. Also we need to save this queue


container at some place for the destroy function,  and I thought it 
would make sense to keep this with the context ptr, as this is how we 
are


identifying the incoming request.


I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely 
related to anything the user queues should be doing.


Please completely drop that relationship and don't use any of the ctx 
object stuff in the user queue code.


Historically the workload submission always came with a context (due to 
CS IOCTL), so we thought it would make sense to still have its relevance 
in the new workload submission method. Would you prefer this new 
submission to be independent of AMDGPU context ?


- Shashank



Christian.



- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))

+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, 
GFP_KERNEL);

+    return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)

+{
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int
+amdgpu_userqueue_validate_input(struct amdgpu_device *adev, struct 
drm_amdgpu_userq_mqd *mqd_in)

+{
+    if (mqd_in->queue_va == 0 || mqd_in->doorbell_handle == 0 || 
mqd_in->doorbell_offset == 0) {

+    DRM_ERROR("Invalid queue object address\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->queue_size == 0 || mqd_in->rptr_va == 0 || 
mqd_in->wptr_va == 0) {

+    DRM_ERROR("Invalid queue object value\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->ip_type < AMDGPU_HW_IP_GFX || mqd_in->ip_type >= 
AMDGPU_HW_IP_NUM) {

+    DRM_ERROR("Invalid HW IP type 0x%x\n", mqd_in->ip_type);
+    return -EINVAL;
+    }
+
+    if (!CHECK_ACCESS(mqd_in->queue_va) || 
!CHECK_ACCESS(mqd_in->rptr_va) ||

+    !CHECK_ACCESS(mqd_in->wptr_va)) {
+    DRM_ERROR("Invalid mapping of queue ptrs, access 
error\n");

+    return -EINVAL;
+    }
+
+    DRM_DEBUG_DRIVER("Input parameters to create queue are valid\n");
+    return 0;
+}
+
+int amdgpu_userqueue_create(struct amdgpu_device *adev, struct 
drm_file *filp,

+    union drm_amdgpu_userq *args)
+{
+    int r, pasid;
+    struct amdgpu_usermode_queue *queue;
+    struct amdgpu_fpriv *fpriv = filp->driver_priv;
+    struct amdgpu_vm 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-02 Thread Christian König

Am 29.12.22 um 18:41 schrieb Alex Deucher:

On Fri, Dec 23, 2022 at 2:37 PM Shashank Sharma  wrote:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
 struct amdgpu_mqd_prop *p);
  };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
 boolenable_mes_kiq;
 struct amdgpu_mes   mes;
 struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

 /* df */
 struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
 unsigned long   ras_counter_ce;
 unsigned long   ras_counter_ue;
 uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;

There can be multiple queues per context.  We should make this a list.


  };

  struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))


You seem to have a very very big misunderstanding here.

access_ok() is used for CPU pointer validation, but this here are 
pointers into the GPUVM address space. This is something completely 
different!


Regards,
Christian.


+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+int index;
+struct amdgpu_userq_globals *uqg = >userq;
+
+index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
+return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)
+{
+struct 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2023-01-02 Thread Christian König

Hi Shashank,

Am 26.12.22 um 11:41 schrieb Shashank Sharma:

[SNIP]

    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which would 
be valid until we destroy this queue. Also we need to save this queue


container at some place for the destroy function,  and I thought it 
would make sense to keep this with the context ptr, as this is how we are


identifying the incoming request.


I have absolutely no idea how you end up with that design.

The ctx object is the CS IOCTL context, that is not even remotely 
related to anything the user queues should be doing.


Please completely drop that relationship and don't use any of the ctx 
object stuff in the user queue code.


Christian.



- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, 
sizeof(__u64)))

+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+    int index;
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, 
GFP_KERNEL);

+    return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)

+{
+    struct amdgpu_userq_globals *uqg = >userq;
+
+    ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int
+amdgpu_userqueue_validate_input(struct amdgpu_device *adev, struct 
drm_amdgpu_userq_mqd *mqd_in)

+{
+    if (mqd_in->queue_va == 0 || mqd_in->doorbell_handle == 0 || 
mqd_in->doorbell_offset == 0) {

+    DRM_ERROR("Invalid queue object address\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->queue_size == 0 || mqd_in->rptr_va == 0 || 
mqd_in->wptr_va == 0) {

+    DRM_ERROR("Invalid queue object value\n");
+    return -EINVAL;
+    }
+
+    if (mqd_in->ip_type < AMDGPU_HW_IP_GFX || mqd_in->ip_type >= 
AMDGPU_HW_IP_NUM) {

+    DRM_ERROR("Invalid HW IP type 0x%x\n", mqd_in->ip_type);
+    return -EINVAL;
+    }
+
+    if (!CHECK_ACCESS(mqd_in->queue_va) || 
!CHECK_ACCESS(mqd_in->rptr_va) ||

+    !CHECK_ACCESS(mqd_in->wptr_va)) {
+    DRM_ERROR("Invalid mapping of queue ptrs, access 
error\n");

+    return -EINVAL;
+    }
+
+    DRM_DEBUG_DRIVER("Input parameters to create queue are valid\n");
+    return 0;
+}
+
+int amdgpu_userqueue_create(struct amdgpu_device *adev, struct 
drm_file *filp,

+    union drm_amdgpu_userq *args)
+{
+    int r, pasid;
+    struct amdgpu_usermode_queue *queue;
+    struct amdgpu_fpriv *fpriv = filp->driver_priv;
+    struct amdgpu_vm *vm = >vm;
+    struct amdgpu_ctx *ctx = amdgpu_ctx_get(fpriv, args->in.ctx_id);
+    struct drm_amdgpu_userq_mqd *mqd_in = >in.mqd;
+
+    if (!ctx) {
+    DRM_ERROR("Invalid GPU context\n");
+    return -EINVAL;
+    }
+
+    if (vm->pasid < 0) {
+    DRM_WARN("No PASID info found\n");
+    pasid = 0;
+   

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2022-12-29 Thread Alex Deucher
On Fri, Dec 23, 2022 at 2:37 PM Shashank Sharma  wrote:
>
> This patch adds skeleton code for usermode queue creation. It
> typically contains:
> - A new structure to keep all the user queue data in one place.
> - An IOCTL function to create/free a usermode queue.
> - A function to generate unique index for the queue.
> - A global ptr in amdgpu_dev
>
> Cc: Alex Deucher 
> Cc: Christian Koenig 
> Signed-off-by: Shashank Sharma 
> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
>  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
>  5 files changed, 246 insertions(+)
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 6ad39cf71bdd..e2a34ee57bfb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -749,6 +749,11 @@ struct amdgpu_mqd {
> struct amdgpu_mqd_prop *p);
>  };
>
> +struct amdgpu_userq_globals {
> +   struct ida ida;
> +   struct mutex userq_mutex;
> +};
> +
>  #define AMDGPU_RESET_MAGIC_NUM 64
>  #define AMDGPU_MAX_DF_PERFMONS 4
>  #define AMDGPU_PRODUCT_NAME_LEN 64
> @@ -955,6 +960,7 @@ struct amdgpu_device {
> boolenable_mes_kiq;
> struct amdgpu_mes   mes;
> struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
> +   struct amdgpu_userq_globals userq;
>
> /* df */
> struct amdgpu_dfdf;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> index 0fa0e56daf67..f7413859b14f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> @@ -57,6 +57,7 @@ struct amdgpu_ctx {
> unsigned long   ras_counter_ce;
> unsigned long   ras_counter_ue;
> uint32_tstable_pstate;
> +   struct amdgpu_usermode_queue*userq;

There can be multiple queues per context.  We should make this a list.

>  };
>
>  struct amdgpu_ctx_mgr {
> 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 ..3b6e8f75495c
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright 2022 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"
> +#include "amdgpu_vm.h"
> +#include "amdgpu_mes.h"
> +#include "amdgpu_usermode_queue.h"
> +#include "soc15_common.h"
> +
> +#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))
> +
> +static int
> +amdgpu_userqueue_index(struct amdgpu_device *adev)
> +{
> +int index;
> +struct amdgpu_userq_globals *uqg = >userq;
> +
> +index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
> +return index;
> +}
> +
> +static void
> +amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
> amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_userq_globals *uqg = >userq;
> +
> +ida_simple_remove(>ida, 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2022-12-26 Thread Shashank Sharma

Hello Christian,

On 25/12/2022 16:44, Christian König wrote:

Am 23.12.22 um 20:36 schrieb Shashank Sharma:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile

index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
  struct amdgpu_mqd_prop *p);
  };
  +struct amdgpu_userq_globals {
+    struct ida ida;
+    struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
  bool    enable_mes_kiq;
  struct amdgpu_mes   mes;
  struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+    struct amdgpu_userq_globals    userq;


This is a pretty big NAK to this. User mode queues should absolutely 
not be global!


This must be per fpriv, see how amdgpu_ctx/amdgpu_ctx_mgr is designed.

Noted,


Or is that for the interface with the MES? If yes than that should be 
part of the MES code, not here.
This is actually to keep a mutex and keep an IDR object. I will first 
check how amdgpu_ctx handles it, as you suggested.



    /* df */
  struct amdgpu_df    df;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h

index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
  unsigned long    ras_counter_ce;
  unsigned long    ras_counter_ue;
  uint32_t    stable_pstate;
+    struct amdgpu_usermode_queue    *userq;


Why should we have this in the ctx here???


We are allocating a few things dynamically for the queue, which would be 
valid until we destroy this queue. Also we need to save this queue


container at some place for the destroy function,  and I thought it 
would make sense to keep this with the context ptr, as this is how we are


identifying the incoming request.

- Shashank



Regards,
Christian.


  };
    struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2022-12-26 Thread Shashank Sharma

Hello Oded,

Thank you for your comments,

On 24/12/2022 19:19, Oded Gabbay wrote:

On Fri, Dec 23, 2022 at 9:37 PM Shashank Sharma  wrote:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
 struct amdgpu_mqd_prop *p);
  };

+struct amdgpu_userq_globals {
+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
 boolenable_mes_kiq;
 struct amdgpu_mes   mes;
 struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;

 /* df */
 struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
 unsigned long   ras_counter_ce;
 unsigned long   ras_counter_ue;
 uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;
  };

  struct amdgpu_ctx_mgr {
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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))
+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+int index;
+struct amdgpu_userq_globals *uqg = >userq;
+
+index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
+return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)
+{
+struct amdgpu_userq_globals *uqg = >userq;
+
+ida_simple_remove(>ida, queue->queue_id);
+}
+
+static int
+amdgpu_userqueue_validate_input(struct amdgpu_device *adev, struct 
drm_amdgpu_userq_mqd *mqd_in)
+{
+if (mqd_in->queue_va == 0 || mqd_in->doorbell_handle == 0 || 

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2022-12-25 Thread Christian König

Am 23.12.22 um 20:36 schrieb Shashank Sharma:

This patch adds skeleton code for usermode queue creation. It
typically contains:
- A new structure to keep all the user queue data in one place.
- An IOCTL function to create/free a usermode queue.
- A function to generate unique index for the queue.
- A global ptr in amdgpu_dev

Cc: Alex Deucher 
Cc: Christian Koenig 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
  5 files changed, 246 insertions(+)
  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 6ad39cf71bdd..e2a34ee57bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -749,6 +749,11 @@ struct amdgpu_mqd {
struct amdgpu_mqd_prop *p);
  };
  
+struct amdgpu_userq_globals {

+   struct ida ida;
+   struct mutex userq_mutex;
+};
+
  #define AMDGPU_RESET_MAGIC_NUM 64
  #define AMDGPU_MAX_DF_PERFMONS 4
  #define AMDGPU_PRODUCT_NAME_LEN 64
@@ -955,6 +960,7 @@ struct amdgpu_device {
boolenable_mes_kiq;
struct amdgpu_mes   mes;
struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
+   struct amdgpu_userq_globals userq;


This is a pretty big NAK to this. User mode queues should absolutely not 
be global!


This must be per fpriv, see how amdgpu_ctx/amdgpu_ctx_mgr is designed.

Or is that for the interface with the MES? If yes than that should be 
part of the MES code, not here.


  
  	/* df */

struct amdgpu_dfdf;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 0fa0e56daf67..f7413859b14f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -57,6 +57,7 @@ struct amdgpu_ctx {
unsigned long   ras_counter_ce;
unsigned long   ras_counter_ue;
uint32_tstable_pstate;
+   struct amdgpu_usermode_queue*userq;


Why should we have this in the ctx here???

Regards,
Christian.


  };
  
  struct amdgpu_ctx_mgr {

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 ..3b6e8f75495c
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright 2022 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"
+#include "amdgpu_vm.h"
+#include "amdgpu_mes.h"
+#include "amdgpu_usermode_queue.h"
+#include "soc15_common.h"
+
+#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))
+
+static int
+amdgpu_userqueue_index(struct amdgpu_device *adev)
+{
+int index;
+struct amdgpu_userq_globals *uqg = >userq;
+
+index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
+return index;
+}
+
+static void
+amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
amdgpu_usermode_queue *queue)
+{
+struct amdgpu_userq_globals *uqg = >userq;
+
+   

Re: [RFC 2/7] drm/amdgpu: Add usermode queue for gfx work

2022-12-24 Thread Oded Gabbay
On Fri, Dec 23, 2022 at 9:37 PM Shashank Sharma  wrote:
>
> This patch adds skeleton code for usermode queue creation. It
> typically contains:
> - A new structure to keep all the user queue data in one place.
> - An IOCTL function to create/free a usermode queue.
> - A function to generate unique index for the queue.
> - A global ptr in amdgpu_dev
>
> Cc: Alex Deucher 
> Cc: Christian Koenig 
> Signed-off-by: Shashank Sharma 
> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile   |   2 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h   |   6 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 187 ++
>  .../drm/amd/include/amdgpu_usermode_queue.h   |  50 +
>  5 files changed, 246 insertions(+)
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>  create mode 100644 drivers/gpu/drm/amd/include/amdgpu_usermode_queue.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 6ad39cf71bdd..e2a34ee57bfb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -209,6 +209,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 8639a4f9c6e8..4b566fcfca18 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -749,6 +749,11 @@ struct amdgpu_mqd {
> struct amdgpu_mqd_prop *p);
>  };
>
> +struct amdgpu_userq_globals {
> +   struct ida ida;
> +   struct mutex userq_mutex;
> +};
> +
>  #define AMDGPU_RESET_MAGIC_NUM 64
>  #define AMDGPU_MAX_DF_PERFMONS 4
>  #define AMDGPU_PRODUCT_NAME_LEN 64
> @@ -955,6 +960,7 @@ struct amdgpu_device {
> boolenable_mes_kiq;
> struct amdgpu_mes   mes;
> struct amdgpu_mqd   mqds[AMDGPU_HW_IP_NUM];
> +   struct amdgpu_userq_globals userq;
>
> /* df */
> struct amdgpu_dfdf;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> index 0fa0e56daf67..f7413859b14f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
> @@ -57,6 +57,7 @@ struct amdgpu_ctx {
> unsigned long   ras_counter_ce;
> unsigned long   ras_counter_ue;
> uint32_tstable_pstate;
> +   struct amdgpu_usermode_queue*userq;
>  };
>
>  struct amdgpu_ctx_mgr {
> 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 ..3b6e8f75495c
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright 2022 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"
> +#include "amdgpu_vm.h"
> +#include "amdgpu_mes.h"
> +#include "amdgpu_usermode_queue.h"
> +#include "soc15_common.h"
> +
> +#define CHECK_ACCESS(a) (access_ok((const void __user *)a, sizeof(__u64)))
> +
> +static int
> +amdgpu_userqueue_index(struct amdgpu_device *adev)
> +{
> +int index;
> +struct amdgpu_userq_globals *uqg = >userq;
> +
> +index = ida_simple_get(>ida, 2, AMDGPU_MAX_USERQ, GFP_KERNEL);
> +return index;
> +}
> +
> +static void
> +amdgpu_userqueue_remove_index(struct amdgpu_device *adev, struct 
> amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_userq_globals *uqg = >userq;
> +
> +ida_simple_remove(>ida, queue->queue_id);
> +}
> +
> +static int
>