Interface changes for prioritized contexts amd-staging-4.9

PATCH New Andres Rodriguez 3 participants 6 messages Apr 24, 2017 – Apr 25, 2017 5 messages not in the archive

Subsystems (3)

DRM drivers DRM misc Radeon and amdgpu DRM drivers

lore

Share & tools

lore

Andres Rodriguez

author

2017-04-24 16:20 UTC #0 lore raw

PATCH 1/1 Interface changes for prioritized contexts amd-staging-4.9

This patch has been rebased onto amd-staging-4.9.

It would be useful to have this patch outside the drm-next-4.12-wip branch as it would unblock submission of the corresponding libdrm and mesa patches.

Splitting this off as it is my understanding that drm-next-4.12-wip may take a while to land on drm-next due to some KIQ regressions.

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 20170424162015.3206-1-andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Andres Rodriguez

author

2017-04-24 16:20 UTC #2 lore raw

PATCH 1/1 drm/amdgpu: add parameter to allocate high priority contexts v8

Add a new context creation parameter to express a global context priority.

The priority ranking in descending order is as follows:
* AMDGPU_CTX_PRIORITY_HIGH
* AMDGPU_CTX_PRIORITY_NORMAL
* AMDGPU_CTX_PRIORITY_LOW

The driver will attempt to schedule work to the hardware according to the priorities. No latency or throughput guarantees are provided by this patch.

This interface intends to service the EGL_IMG_context_priority extension, and vulkan equivalents.

v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE

Reviewed-by Emil Velikov <[email protected]>

Reviewed-by Christian König <[email protected]>

Signed-off-by Andres Rodriguez <[email protected]>

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c       | 38 ++++++++++++++++++++++++---
 drivers/gpu/drm/amd/scheduler/gpu_scheduler.h |  4 ++-
 include/uapi/drm/amdgpu_drm.h                 |  9 ++++++-
 3 files changed, 45 insertions(+), 6 deletions(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c +34 -4

index b4bbbb3..af75571 100644
@@ -8,124 +8,134 @@
  * 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.
  *
  * Authors: monk liu <[email protected]>
  */
 
 #include <drm/drmP.h>
 #include "amdgpu.h"
 
-static int amdgpu_ctx_init(struct amdgpu_device *adev, struct amdgpu_ctx *ctx)
+static int amdgpu_ctx_init(struct amdgpu_device *adev,
+			   enum amd_sched_priority priority,
+			   struct amdgpu_ctx *ctx)
 {
 	unsigned i, j;
 	int r;
 
+	if (priority < 0 || priority >= AMD_SCHED_PRIORITY_MAX)
+		return -EINVAL;
+
+	if (priority >= AMD_SCHED_PRIORITY_HIGH && !capable(CAP_SYS_NICE))
+		return -EACCES;
+
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->adev = adev;
 	kref_init(&ctx->refcount);
 	spin_lock_init(&ctx->ring_lock);
 	ctx->fences = kcalloc(amdgpu_sched_jobs * AMDGPU_MAX_RINGS,
 			      sizeof(struct fence*), GFP_KERNEL);
 	if (!ctx->fences)
 		return -ENOMEM;
 
 	for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
 		ctx->rings[i].sequence = 1;
 		ctx->rings[i].fences = &ctx->fences[amdgpu_sched_jobs * i];
 	}
 
 	ctx->reset_counter = atomic_read(&adev->gpu_reset_counter);
 
 	/* create context entity for each ring */
 	for (i = 0; i < adev->num_rings; i++) {
 		struct amdgpu_ring *ring = adev->rings[i];
 		struct amd_sched_rq *rq;
 
-		rq = &ring->sched.sched_rq[AMD_SCHED_PRIORITY_NORMAL];
+		rq = &ring->sched.sched_rq[priority];
 		r = amd_sched_entity_init(&ring->sched, &ctx->rings[i].entity,
 					  rq, amdgpu_sched_jobs);
 		if (r)
 			goto failed;
 	}
 
 	return 0;
 
 failed:
 	for (j = 0; j < i; j++)
 		amd_sched_entity_fini(&adev->rings[j]->sched,
 				      &ctx->rings[j].entity);
 	kfree(ctx->fences);
 	ctx->fences = NULL;
 	return r;
 }
 
 static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx)
 {
 	struct amdgpu_device *adev = ctx->adev;
 	unsigned i, j;
 
 	if (!adev)
 		return;
 
 	for (i = 0; i < AMDGPU_MAX_RINGS; ++i)
 		for (j = 0; j < amdgpu_sched_jobs; ++j)
 			fence_put(ctx->rings[i].fences[j]);
 	kfree(ctx->fences);
 	ctx->fences = NULL;
 
 	for (i = 0; i < adev->num_rings; i++)
 		amd_sched_entity_fini(&adev->rings[i]->sched,
 				      &ctx->rings[i].entity);
 }
 
 static int amdgpu_ctx_alloc(struct amdgpu_device *adev,
 			    struct amdgpu_fpriv *fpriv,
+			    enum amd_sched_priority priority,
 			    uint32_t *id)
 {
 	struct amdgpu_ctx_mgr *mgr = &fpriv->ctx_mgr;
 	struct amdgpu_ctx *ctx;
 	int r;
 
 	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
 	mutex_lock(&mgr->lock);
 	r = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
 	if (r < 0) {
 		mutex_unlock(&mgr->lock);
 		kfree(ctx);
 		return r;
 	}
+
 	*id = (uint32_t)r;
-	r = amdgpu_ctx_init(adev, ctx);
+	r = amdgpu_ctx_init(adev, priority, ctx);
 	if (r) {
 		idr_remove(&mgr->ctx_handles, *id);
 		*id = 0;
 		kfree(ctx);
 	}
 	mutex_unlock(&mgr->lock);
 	return r;
 }
 
 static void amdgpu_ctx_do_release(struct kref *ref)
 {
 	struct amdgpu_ctx *ctx;
 
 	ctx = container_of(ref, struct amdgpu_ctx, refcount);
 
 	amdgpu_ctx_fini(ctx);
 
 	kfree(ctx);
 }

@@ -165,56 +175,76 @@ static int amdgpu_ctx_query(struct amdgpu_device *adev,
 		return -EINVAL;
 	}
 
 	/* TODO: these two are always zero */
 	out->state.flags = 0x0;
 	out->state.hangs = 0x0;
 
 	/* determine if a GPU reset has occured since the last call */
 	reset_counter = atomic_read(&adev->gpu_reset_counter);
 	/* TODO: this should ideally return NO, GUILTY, or INNOCENT. */
 	if (ctx->reset_counter == reset_counter)
 		out->state.reset_status = AMDGPU_CTX_NO_RESET;
 	else
 		out->state.reset_status = AMDGPU_CTX_UNKNOWN_RESET;
 	ctx->reset_counter = reset_counter;
 
 	mutex_unlock(&mgr->lock);
 	return 0;
 }
 
+static enum amd_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
+{
+	switch (amdgpu_priority) {
+	case AMDGPU_CTX_PRIORITY_HIGH:
+		return AMD_SCHED_PRIORITY_HIGH;
+	case AMDGPU_CTX_PRIORITY_NORMAL:
+		return AMD_SCHED_PRIORITY_NORMAL;
+	case AMDGPU_CTX_PRIORITY_LOW:
+		return AMD_SCHED_PRIORITY_LOW;
+	default:
+		WARN(1, "Invalid context priority %d\n", amdgpu_priority);
+		return AMD_SCHED_PRIORITY_NORMAL;
+	}
+}
+
 int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *filp)
 {
 	int r;
 	uint32_t id;
+	enum amd_sched_priority priority;
 
 	union drm_amdgpu_ctx *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_fpriv *fpriv = filp->driver_priv;
 
 	r = 0;
 	id = args->in.ctx_id;
+	priority = amdgpu_to_sched_priority(args->in.priority);
+
+	if (priority >= AMD_SCHED_PRIORITY_MAX)
+		return -EINVAL;
 
 	switch (args->in.op) {
 	case AMDGPU_CTX_OP_ALLOC_CTX:
-		r = amdgpu_ctx_alloc(adev, fpriv, &id);
+		r = amdgpu_ctx_alloc(adev, fpriv, priority, &id);
 		args->out.alloc.ctx_id = id;
 		break;
 	case AMDGPU_CTX_OP_FREE_CTX:
 		r = amdgpu_ctx_free(fpriv, id);
 		break;
 	case AMDGPU_CTX_OP_QUERY_STATE:
 		r = amdgpu_ctx_query(adev, fpriv, id, &args->out);
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	return r;
 }
 
 struct amdgpu_ctx *amdgpu_ctx_get(struct amdgpu_fpriv *fpriv, uint32_t id)
 {
 	struct amdgpu_ctx *ctx;
 	struct amdgpu_ctx_mgr *mgr;

drivers/gpu/drm/amd/scheduler/gpu_scheduler.h +3 -1

index 8cb41d3..613e682 100644
@@ -92,41 +92,43 @@ static inline struct amd_sched_fence *to_amd_sched_fence(struct fence *f)
 
 	if (f->ops == &amd_sched_fence_ops_finished)
 		return container_of(f, struct amd_sched_fence, finished);
 
 	return NULL;
 }
 
 /**
  * Define the backend operations called by the scheduler,
  * these functions should be implemented in driver side
 */
 struct amd_sched_backend_ops {
 	struct fence *(*dependency)(struct amd_sched_job *sched_job);
 	struct fence *(*run_job)(struct amd_sched_job *sched_job);
 	void (*timedout_job)(struct amd_sched_job *sched_job);
 	void (*free_job)(struct amd_sched_job *sched_job);
 };
 
 enum amd_sched_priority {
 	AMD_SCHED_PRIORITY_MIN,
-	AMD_SCHED_PRIORITY_NORMAL = AMD_SCHED_PRIORITY_MIN,
+	AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
+	AMD_SCHED_PRIORITY_NORMAL,
+	AMD_SCHED_PRIORITY_HIGH,
 	AMD_SCHED_PRIORITY_KERNEL,
 	AMD_SCHED_PRIORITY_MAX
 };
 
 /**
  * One scheduler is implemented for each hardware ring
 */
 struct amd_gpu_scheduler {
 	const struct amd_sched_backend_ops	*ops;
 	uint32_t			hw_submission_limit;
 	long				timeout;
 	const char			*name;
 	struct amd_sched_rq		sched_rq[AMD_SCHED_PRIORITY_MAX];
 	wait_queue_head_t		wake_up_worker;
 	wait_queue_head_t		job_scheduled;
 	atomic_t			hw_rq_count;
 	atomic64_t			job_id_count;
 	struct task_struct		*thread;
 	struct list_head	ring_mirror_list;
 	spinlock_t			job_list_lock;

include/uapi/drm/amdgpu_drm.h +8 -1

index d76d525..af88d85 100644
@@ -145,47 +145,54 @@ struct drm_amdgpu_bo_list_out {
 
 union drm_amdgpu_bo_list {
 	struct drm_amdgpu_bo_list_in in;
 	struct drm_amdgpu_bo_list_out out;
 };
 
 /* context related */
 #define AMDGPU_CTX_OP_ALLOC_CTX	1
 #define AMDGPU_CTX_OP_FREE_CTX	2
 #define AMDGPU_CTX_OP_QUERY_STATE	3
 
 /* GPU reset status */
 #define AMDGPU_CTX_NO_RESET		0
 /* this the context caused it */
 #define AMDGPU_CTX_GUILTY_RESET		1
 /* some other context caused it */
 #define AMDGPU_CTX_INNOCENT_RESET	2
 /* unknown cause */
 #define AMDGPU_CTX_UNKNOWN_RESET	3
 
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_NORMAL	0
+#define AMDGPU_CTX_PRIORITY_LOW		1
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH	2
+#define AMDGPU_CTX_PRIORITY_NUM		3
+
 struct drm_amdgpu_ctx_in {
 	/** AMDGPU_CTX_OP_* */
 	__u32	op;
 	/** For future use, no flags defined so far */
 	__u32	flags;
 	__u32	ctx_id;
-	__u32	_pad;
+	__u32	priority;
 };
 
 union drm_amdgpu_ctx_out {
 		struct {
 			__u32	ctx_id;
 			__u32	_pad;
 		} alloc;
 
 		struct {
 			/** For future use, no flags defined so far */
 			__u64	flags;
 			/** Number of resets caused by this context so far. */
 			__u32	hangs;
 			/** Reset status since the last call of the ioctl. */
 			__u32	reset_status;
 		} state;
 };
 
 union drm_amdgpu_ctx {
 	struct drm_amdgpu_ctx_in in;

Signature · 6 lines

2.9.3

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 20170424162015.3206-2-andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Nicolai Hähnle

2017-04-25 18:01 UTC #4 lore raw

Re: [PATCH] drm/amdgpu: add parameter to allocate high priority contexts v8

Quoted 22 lines · Andres Rodriguez

Add a new context creation parameter to express a global context priority.
The priority ranking in descending order is as follows:
* AMDGPU_CTX_PRIORITY_HIGH
* AMDGPU_CTX_PRIORITY_NORMAL
* AMDGPU_CTX_PRIORITY_LOW
The driver will attempt to schedule work to the hardware according to the priorities. No latency or throughput guarantees are provided by this patch.
This interface intends to service the EGL_IMG_context_priority extension, and vulkan equivalents.
v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE

Quoted 3 lines

Reviewed-by Emil Velikov <[email protected]>
Reviewed-by Christian König <[email protected]>
Signed-off-by Andres Rodriguez <[email protected]>

I didn't follow all the discussion, so feel free to shut me up if this has already been discussed, but...

[snip]

include/uapi/drm/amdgpu_drm.h struct drm_amdgpu_bo_list_out { · line 170

Quoted 6 lines

include/uapi/drm/amdgpu_drm.h +6 -0

+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_NORMAL	0
+#define AMDGPU_CTX_PRIORITY_LOW		1
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH	2
+#define AMDGPU_CTX_PRIORITY_NUM		3

I get that normal priority needs to be 0 for backwards compatibility, but having LOW between NORMAL and HIGH is still odd. Have you considered using signed integers as a way to fix that?

(AMDGPU_CTX_PRIORITY_NUM doesn't seem to be used anywhere...)

Cheers,
Nicolai

Quoted 30 lines

include/uapi/drm/amdgpu_drm.h +2 -1

+
 struct drm_amdgpu_ctx_in {
 	/** AMDGPU_CTX_OP_* */
 	__u32	op;
 	/** For future use, no flags defined so far */
 	__u32	flags;
 	__u32	ctx_id;
-	__u32	_pad;
+	__u32	priority;
 };
 
 union drm_amdgpu_ctx_out {
 		struct {
 			__u32	ctx_id;
 			__u32	_pad;
 		} alloc;
 
 		struct {
 			/** For future use, no flags defined so far */
 			__u64	flags;
 			/** Number of resets caused by this context so far. */
 			__u32	hangs;
 			/** Reset status since the last call of the ioctl. */
 			__u32	reset_status;
 		} state;
 };
 
 union drm_amdgpu_ctx {
 	struct drm_amdgpu_ctx_in in;

Signature · 6 lines

Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): 2c2a2270-5c75-fa9f-b0f9-7af68348bc7e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Andres Rodriguez

author

2017-04-25 20:28 UTC #6 lore raw

Re: [PATCH] drm/amdgpu: add parameter to allocate high priority contexts v8

Quoted 33 lines · Nicolai Hähnle

Quoted 23 lines · Andres Rodriguez
Add a new context creation parameter to express a global context priority.
The priority ranking in descending order is as follows:
* AMDGPU_CTX_PRIORITY_HIGH
* AMDGPU_CTX_PRIORITY_NORMAL
* AMDGPU_CTX_PRIORITY_LOW
The driver will attempt to schedule work to the hardware according to the priorities. No latency or throughput guarantees are provided by this patch.
This interface intends to service the EGL_IMG_context_priority extension, and vulkan equivalents.
v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE
Quoted 3 lines
Reviewed-by Emil Velikov <[email protected]>
Reviewed-by Christian König <[email protected]>
Signed-off-by Andres Rodriguez <[email protected]>
I didn't follow all the discussion, so feel free to shut me up if this has already been discussed, but...
[snip]

Quoted 10 lines

include/uapi/drm/amdgpu_drm.h struct drm_amdgpu_bo_list_out { · line 170
Quoted 6 lines
include/uapi/drm/amdgpu_drm.h +6 -0
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_NORMAL    0
+#define AMDGPU_CTX_PRIORITY_LOW        1
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH    2
+#define AMDGPU_CTX_PRIORITY_NUM        3
I get that normal priority needs to be 0 for backwards compatibility, but having LOW between NORMAL and HIGH is still odd. Have you considered using signed integers as a way to fix that?

Thanks for the suggestion, that should make it a lot cleaner.

Regards,
Andres

Quoted 39 lines

(AMDGPU_CTX_PRIORITY_NUM doesn't seem to be used anywhere...)

Cheers,
Nicolai

Quoted 30 lines

include/uapi/drm/amdgpu_drm.h +2 -1

+
 struct drm_amdgpu_ctx_in {
     /** AMDGPU_CTX_OP_* */
     __u32    op;
     /** For future use, no flags defined so far */
     __u32    flags;
     __u32    ctx_id;
-    __u32    _pad;
+    __u32    priority;
 };
 
 union drm_amdgpu_ctx_out {
         struct {
             __u32    ctx_id;
             __u32    _pad;
         } alloc;
 
         struct {
             /** For future use, no flags defined so far */
             __u64    flags;
             /** Number of resets caused by this context so far. */
             __u32    hangs;
             /** Reset status since the last call of the ioctl. */
             __u32    reset_status;
         } state;
 };
 
 union drm_amdgpu_ctx {
     struct drm_amdgpu_ctx_in in;

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): f5b65546-d3a6-ec1c-aa73-ad0c205f5caa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Alex Deucher

2017-04-25 22:21 UTC #8 lore raw

Re: [PATCH] drm/amdgpu: add parameter to allocate high priority contexts v8

Quoted 53 lines · Andres Rodriguez

Quoted 37 lines · Nicolai Hähnle
Quoted 24 lines · Andres Rodriguez
Add a new context creation parameter to express a global context priority.
The priority ranking in descending order is as follows:
* AMDGPU_CTX_PRIORITY_HIGH
* AMDGPU_CTX_PRIORITY_NORMAL
* AMDGPU_CTX_PRIORITY_LOW
The driver will attempt to schedule work to the hardware according to the priorities. No latency or throughput guarantees are provided by this patch.
This interface intends to service the EGL_IMG_context_priority extension, and vulkan equivalents.
v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE
Quoted 3 lines
Reviewed-by Emil Velikov <[email protected]>
Reviewed-by Christian König <[email protected]>
Signed-off-by Andres Rodriguez <[email protected]>
I didn't follow all the discussion, so feel free to shut me up if this has already been discussed, but...
[snip]
Quoted 1 line
Quoted 11 lines
include/uapi/drm/amdgpu_drm.h struct drm_amdgpu_bo_list_out { · line 170
Quoted 6 lines
include/uapi/drm/amdgpu_drm.h +6 -0
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_NORMAL    0
+#define AMDGPU_CTX_PRIORITY_LOW        1
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH    2
+#define AMDGPU_CTX_PRIORITY_NUM        3
I get that normal priority needs to be 0 for backwards compatibility, but having LOW between NORMAL and HIGH is still odd. Have you considered using signed integers as a way to fix that?

Quoted 1 line

Thanks for the suggestion, that should make it a lot cleaner.

Maybe make the range -1023 to 1023 for consistency with the similar proposed interface on Intel?
https://lists.freedesktop.org/archives/intel-gfx/2017-April/126155.html

Alex

Quoted 44 lines

Regards,
Andres

Quoted 39 lines

(AMDGPU_CTX_PRIORITY_NUM doesn't seem to be used anywhere...)

Cheers,
Nicolai

Quoted 30 lines

include/uapi/drm/amdgpu_drm.h +2 -1

+
 struct drm_amdgpu_ctx_in {
     /** AMDGPU_CTX_OP_* */
     __u32    op;
     /** For future use, no flags defined so far */
     __u32    flags;
     __u32    ctx_id;
-    __u32    _pad;
+    __u32    priority;
 };
 
 union drm_amdgpu_ctx_out {
         struct {
             __u32    ctx_id;
             __u32    _pad;
         } alloc;
 
         struct {
             /** For future use, no flags defined so far */
             __u64    flags;
             /** Number of resets caused by this context so far. */
             __u32    hangs;
             /** Reset status since the last call of the ioctl. */
             __u32    reset_status;
         } state;
 };
 
 union drm_amdgpu_ctx {
     struct drm_amdgpu_ctx_in in;

Quoted 4 lines

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): CADnq5_M-p8TUiD_RdJSz_K68S+svwhWsaBUM+u9V23PP5VKDWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Andres Rodriguez

author

2017-04-25 22:26 UTC #10 lore raw

Re: [PATCH] drm/amdgpu: add parameter to allocate high priority contexts v8

Quoted 54 lines · Alex Deucher

Quoted 53 lines · Andres Rodriguez
Quoted 48 lines · Nicolai Hähnle
Quoted 24 lines · Andres Rodriguez
Add a new context creation parameter to express a global context priority.
The priority ranking in descending order is as follows:
* AMDGPU_CTX_PRIORITY_HIGH
* AMDGPU_CTX_PRIORITY_NORMAL
* AMDGPU_CTX_PRIORITY_LOW
The driver will attempt to schedule work to the hardware according to the priorities. No latency or throughput guarantees are provided by this patch.
This interface intends to service the EGL_IMG_context_priority extension, and vulkan equivalents.
v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE
Quoted 3 lines
Reviewed-by Emil Velikov <[email protected]>
Reviewed-by Christian König <[email protected]>
Signed-off-by Andres Rodriguez <[email protected]>
I didn't follow all the discussion, so feel free to shut me up if this has already been discussed, but...
[snip]
Review
Quoted 7 lines
(quoted diff) +6 -0
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_NORMAL    0
+#define AMDGPU_CTX_PRIORITY_LOW        1
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH    2
+#define AMDGPU_CTX_PRIORITY_NUM        3
I get that normal priority needs to be 0 for backwards compatibility, but having LOW between NORMAL and HIGH is still odd. Have you considered using signed integers as a way to fix that?

Quoted 9 lines

Quoted 1 line
Thanks for the suggestion, that should make it a lot cleaner.
Maybe make the range -1023 to 1023 for consistency with the similar proposed interface on Intel?
https://lists.freedesktop.org/archives/intel-gfx/2017-April/126155.html
Alex

Sure, that gives us a range to add new priority leves.

Andres

Quoted 44 lines

Regards,
Andres

Quoted 39 lines

(AMDGPU_CTX_PRIORITY_NUM doesn't seem to be used anywhere...)

Cheers,
Nicolai

Quoted 30 lines

(quoted diff) +2 -1

+
 struct drm_amdgpu_ctx_in {
     /** AMDGPU_CTX_OP_* */
     __u32    op;
     /** For future use, no flags defined so far */
     __u32    flags;
     __u32    ctx_id;
-    __u32    _pad;
+    __u32    priority;
 };
 
 union drm_amdgpu_ctx_out {
         struct {
             __u32    ctx_id;
             __u32    _pad;
         } alloc;
 
         struct {
             /** For future use, no flags defined so far */
             __u64    flags;
             /** Number of resets caused by this context so far. */
             __u32    hangs;
             /** Reset status since the last call of the ioctl. */
             __u32    reset_status;
         } state;
 };
 
 union drm_amdgpu_ctx {
     struct drm_amdgpu_ctx_in in;

Quoted 4 lines

Quoted 4 lines
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx