> -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf > Of Nicolai Hähnle > Sent: Saturday, April 29, 2017 1:45 PM > To: Andres Rodriguez; amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH] drm/amdgpu: add parameter to allocate high priority > contexts v9 > > On 29.04.2017 18:30, Andres Rodriguez wrote: > > > > > > On 2017-04-29 04:34 AM, Nicolai Hähnle wrote: > >> Thanks for the update! > >> > >> > >> On 26.04.2017 03:10, Andres Rodriguez wrote: > >>> Add a new context creation parameter to express a global context > >>> priority. > >>> > >>> The priority ranking in descending order is as follows: > >>> * AMDGPU_CTX_PRIOR ITY_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 > >>> v9: change the priority parameter to __s32 > >>> > >>> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com> > >>> Reviewed-by: Christian König <christian.koenig at amd.com> > >>> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com> > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 38 > >>> ++++++++++++++++++++++++--- > >>> drivers/gpu/drm/amd/scheduler/gpu_scheduler.h | 4 ++- > >>> include/uapi/drm/amdgpu_drm.h | 8 +++++- > >>> 3 files changed, 44 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > >>> index b4bbbb3..af75571 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > >>> @@ -25,11 +25,19 @@ > >>> #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); > >>> @@ -51,7 +59,7 @@ static int amdgpu_ctx_init(struct amdgpu_device > >>> *adev, struct amdgpu_ctx *ctx) > >>> 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) > >>> @@ -90,6 +98,7 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx > *ctx) > >>> > >>> 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; > >>> @@ -107,8 +116,9 @@ static int amdgpu_ctx_alloc(struct > amdgpu_device > >>> *adev, > >>> 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; > >>> @@ -182,11 +192,27 @@ static int amdgpu_ctx_query(struct > amdgpu_device > >>> *adev, > >>> 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; > >> > >> This needs to be changed now to support the range. > >> > >> > > > > I actually don't intend on the priority parameter to behave like a > > range. libdrm is expected to pass in HIGH/NORMAL/LOW, and nothing in > > between. > > Okay, makes sense. > > > > The current version of the hardware only supports a handful of discrete > > priority configurations. So I would rather avoid creating the illusion > > that a priority of -333 is any different than 0. > > > > What I like about your suggestion of spreading out the values further > > apart (-1023, 0, 1023 vs -1, 0, +1), is that it gives us the option to > > add new priority values and keep everything ordered. Or, we could also > > expand into ranges and still maintain backwards compatibility (if the HW > > supports it). > > > > The EGL extension and the vulkan draft extension for context priorities > > also use discrete values. So I don't really see a case for pursuing > > range based priorities when the APIs and the HW don't support it. > > > > > >>> + 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; > >>> @@ -194,10 +220,14 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, > >>> void *data, > >>> > >>> r = 0; > >>> id = args->in.ctx_id; > >>> + priority = amdgpu_to_sched_priority(args->in.priority); > >>> + > >>> + if (priority >= AMD_SCHED_PRIORITY_MAX) > >>> + return -EINVAL; > >> > >> We check ioctl parameters before using them. In this case the range > >> check should happen before all this. Misbehaving user-space programs > >> shouldn't be able to run into the WARN in amdgpu_to_sched_priority that > >> easily, and most of all they shouldn't silently have their ioctls > >> succeed. Otherwise, we limit our ability to evolve the interface. > >> > > > > The checking is intended to happen in amdgpu_to_sched priority. The > > check is non-fatal in case a usermode program used to have _pad filled > > with garbage. > > > > I agree silently succeeding here is non ideal. My stab at providing some > > feedback is the WARN, so that someone may at least notice that their > > program is faulty. > > > > The solution I have isn't perfect for backwards compat either. If the > > faulty app used to have 1024 as their _pad, then they would fail with > > this patch, but work correctly without it. I think that case is > > statistically unlikely, so we should be okay *fingers crossed*. Note > > that libdrm, which accounts for most clients of the amdgpu ioctl > > interface has always memsetted this field to 0. And that accounts for a > > majority of the clients of this interface. > > > > If there are any suggestions for a better approach to deal with this > > situation don't hesitate to let me know. I honestly don't like silently > > changing usermode's intentions, but I also don't want to get an angry > > email saying "RULE#1: WE DON'T BREAK USERSPACE!". > > Oh wow, I didn't realize there was no checking of _pad. Looks like > there's no check on flags either? That sucks :/ There's no reason we can't add more checks. If something breaks it can be reverted. Alex > > I agree, there's just no really clean solution to this without adding a > new ioctl, so I think your approach is fine. > > Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com> > > > > Thanks for the feedback, > > Andres > > > >> Cheers, > >> Nicolai > >> > >> > >>> > >>> 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: > >>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h > >>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h > >>> index 8cb41d3..613e682 100644 > >>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h > >>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h > >>> @@ -109,7 +109,9 @@ struct amd_sched_backend_ops { > >>> > >>> 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 > >>> }; > >>> diff --git a/include/uapi/drm/amdgpu_drm.h > >>> b/include/uapi/drm/amdgpu_drm.h > >>> index d76d525..6a2d97c 100644 > >>> --- a/include/uapi/drm/amdgpu_drm.h > >>> +++ b/include/uapi/drm/amdgpu_drm.h > >>> @@ -162,13 +162,19 @@ union drm_amdgpu_bo_list { > >>> /* unknown cause */ > >>> #define AMDGPU_CTX_UNKNOWN_RESET 3 > >>> > >>> +/* Context priority level */ > >>> +#define AMDGPU_CTX_PRIORITY_LOW -1023 > >>> +#define AMDGPU_CTX_PRIORITY_NORMAL 0 > >>> +/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */ > >>> +#define AMDGPU_CTX_PRIORITY_HIGH 1023 > >>> + > >>> 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; > >>> + __s32 priority; > >>> }; > >>> > >>> union drm_amdgpu_ctx_out { > >>> > >> > >> > > > -- > Lerne, wie die Welt wirklich ist, > Aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
