[PATCH v2] amdgpu: implement context priority for amdgpu_cs_ctx_create2

[PATCH v2] amdgpu: implement context priority for amdgpu_cs_ctx_create2

[Andres Rodriguez]

Corresponding libdrm changes for:
[PATCH v2] High priority usermode contexts

Comments on version bump appreciated.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] amdgpu: implement context priority for amdgpu_cs_ctx_create2 v2

[Andres Rodriguez]

Add a new context creation function that allows specifying the context
priority.

A high priority context has the potential of starving lower priority
contexts. The current kernel driver implementation only allows only the
root user to allocate high priority contexts.

v2: corresponding changes for kernel patch v2
---
 amdgpu/amdgpu.h          | 17 +++++++++++++++--
 amdgpu/amdgpu_cs.c       | 17 +++++++++++++----
 configure.ac             |  2 +-
 include/drm/amdgpu_drm.h |  6 +++++-
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index 7b26a04..eef7a62 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -794,8 +794,9 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle handle,
  * context will always be executed in order (first come, first serve).
  *
  *
- * \param   dev	    - \c [in] Device handle. See #amdgpu_device_initialize()
- * \param   context - \c [out] GPU Context handle
+ * \param   dev      - \c [in] Device handle. See #amdgpu_device_initialize()
+ * \param   priority - \c [in] Context creation flags. See AMDGPU_CTX_PRIORITY_*
+ * \param   context  - \c [out] GPU Context handle
  *
  * \return   0 on success\n
  *          <0 - Negative POSIX Error code
@@ -803,6 +804,18 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle handle,
  * \sa amdgpu_cs_ctx_free()
  *
 */
+int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
+			 uint32_t priority,
+			 amdgpu_context_handle *context);
+/**
+ * Create GPU execution Context
+ *
+ * Refer to amdgpu_cs_ctx_create2 for full documentation. This call
+ * is missing the priority parameter.
+ *
+ * \sa amdgpu_cs_ctx_create2()
+ *
+*/
 int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
 			 amdgpu_context_handle *context);
 
diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
index fb5b3a8..2f7669f 100644
--- a/amdgpu/amdgpu_cs.c
+++ b/amdgpu/amdgpu_cs.c
@@ -46,13 +46,14 @@ static int amdgpu_cs_reset_sem(amdgpu_semaphore_handle sem);
 /**
  * Create command submission context
  *
- * \param   dev - \c [in] amdgpu device handle
- * \param   context - \c [out] amdgpu context handle
+ * \param   dev      - \c [in] Device handle. See #amdgpu_device_initialize()
+ * \param   priority - \c [in] Context creation flags. See AMDGPU_CTX_PRIORITY_*
+ * \param   context  - \c [out] GPU Context handle
  *
  * \return  0 on success otherwise POSIX Error code
 */
-int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
-			 amdgpu_context_handle *context)
+int amdgpu_cs_ctx_create2(amdgpu_device_handle dev, uint32_t priority,
+							amdgpu_context_handle *context)
 {
 	struct amdgpu_context *gpu_context;
 	union drm_amdgpu_ctx args;
@@ -77,6 +78,8 @@ int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
 	/* Create the context */
 	memset(&args, 0, sizeof(args));
 	args.in.op = AMDGPU_CTX_OP_ALLOC_CTX;
+	args.in.priority = priority;
+
 	r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_CTX, &args, sizeof(args));
 	if (r)
 		goto error;
@@ -96,6 +99,12 @@ error:
 	return r;
 }
 
+int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
+			 amdgpu_context_handle *context)
+{
+	return amdgpu_cs_ctx_create2(dev, 0, context);
+}
+
 /**
  * Release command submission context
  *
diff --git a/configure.ac b/configure.ac
index e0597c3..682171b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -20,7 +20,7 @@
 
 AC_PREREQ([2.63])
 AC_INIT([libdrm],
-        [2.4.74],
+        [2.4.75],
         [https://bugs.freedesktop.org/enter_bug.cgi?product=DRI],
         [libdrm])
 
diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
index d8f2497..01cf3c9 100644
--- a/include/drm/amdgpu_drm.h
+++ b/include/drm/amdgpu_drm.h
@@ -154,13 +154,17 @@ union drm_amdgpu_bo_list {
 /* unknown cause */
 #define AMDGPU_CTX_UNKNOWN_RESET	3
 
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_HIGH	0
+#define AMDGPU_CTX_PRIORITY_NORMAL	1
+
 struct drm_amdgpu_ctx_in {
 	/** AMDGPU_CTX_OP_* */
 	uint32_t	op;
 	/** For future use, no flags defined so far */
 	uint32_t	flags;
 	uint32_t	ctx_id;
-	uint32_t	_pad;
+	uint32_t	priority;
 };
 
 union drm_amdgpu_ctx_out {
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] amdgpu: implement context priority for amdgpu_cs_ctx_create2 v2

[Emil Velikov]

Hi Andres,

Pardon for jumping in uninvited, there's a few comments below that you
might find useful.

On 4 January 2017 at 01:56, Andres Rodriguez <andresx7@gmail.com> wrote:

> --- a/configure.ac
> +++ b/configure.ac
> @@ -20,7 +20,7 @@
>
>  AC_PREREQ([2.63])
>  AC_INIT([libdrm],
> -        [2.4.74],
> +        [2.4.75],
This part should be separate and done only as one rolls a release.
Currently we have more than a dozen of people who can do that, with 3+
from the AMD team.

>          [https://bugs.freedesktop.org/enter_bug.cgi?product=DRI],
>          [libdrm])
>
> diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
> index d8f2497..01cf3c9 100644
> --- a/include/drm/amdgpu_drm.h
> +++ b/include/drm/amdgpu_drm.h
Please sync this file as described in the README, in the same directory.

>
> +/* Context priority level */
> +#define AMDGPU_CTX_PRIORITY_HIGH       0
> +#define AMDGPU_CTX_PRIORITY_NORMAL     1
You really want NORMAL to be 0 here, to preserve the behaviour. In
other words, [sane] old userspace with new kernel will set HIGH prio,
where NORMAL is used with older kernels.

> +
>  struct drm_amdgpu_ctx_in {
>         /** AMDGPU_CTX_OP_* */
>         uint32_t        op;
>         /** For future use, no flags defined so far */
>         uint32_t        flags;
>         uint32_t        ctx_id;
> -       uint32_t        _pad;
> +       uint32_t        priority;
This part is a bit of meh:

Namely things can go crazy in either one of these scenarios:
 - old kernel [validates _pad to be zero/other], new userspace feeds
"invalid" value.
 - new kernel. old [nasty] userspace which leaves the value uninitialised.

Re-using _pad is ok, just make sure things don't explode all over the place.

Regards,
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] amdgpu: implement context priority for amdgpu_cs_ctx_create2 v2

[Andres Rodriguez]

Thanks for the feedback Emil, it is very welcome :)

On 1/4/2017 8:47 AM, Emil Velikov wrote:
> Hi Andres,
>
> Pardon for jumping in uninvited, there's a few comments below that you
> might find useful.
>
> On 4 January 2017 at 01:56, Andres Rodriguez <andresx7@gmail.com> wrote:
>
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -20,7 +20,7 @@
>>
>>   AC_PREREQ([2.63])
>>   AC_INIT([libdrm],
>> -        [2.4.74],
>> +        [2.4.75],
> This part should be separate and done only as one rolls a release.
> Currently we have more than a dozen of people who can do that, with 3+
> from the AMD team.
I'll take this out of the patch.
>
>>           [https://bugs.freedesktop.org/enter_bug.cgi?product=DRI],
>>           [libdrm])
>>
>> diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
>> index d8f2497..01cf3c9 100644
>> --- a/include/drm/amdgpu_drm.h
>> +++ b/include/drm/amdgpu_drm.h
> Please sync this file as described in the README, in the same directory.
Thanks for pointing that out, I totally missed the README
>
>> +/* Context priority level */
>> +#define AMDGPU_CTX_PRIORITY_HIGH       0
>> +#define AMDGPU_CTX_PRIORITY_NORMAL     1
> You really want NORMAL to be 0 here, to preserve the behaviour. In
> other words, [sane] old userspace with new kernel will set HIGH prio,
> where NORMAL is used with older kernels.
Will fix.
>
>> +
>>   struct drm_amdgpu_ctx_in {
>>          /** AMDGPU_CTX_OP_* */
>>          uint32_t        op;
>>          /** For future use, no flags defined so far */
>>          uint32_t        flags;
>>          uint32_t        ctx_id;
>> -       uint32_t        _pad;
>> +       uint32_t        priority;
> This part is a bit of meh:
>
> Namely things can go crazy in either one of these scenarios:
>   - old kernel [validates _pad to be zero/other], new userspace feeds
> "invalid" value.
>   - new kernel. old [nasty] userspace which leaves the value uninitialised.
>
> Re-using _pad is ok, just make sure things don't explode all over the place.
>
> Regards,
> Emil
Regards,
Andres

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx