<div dir="ltr"><div>Cool. Follow up question then: </div><div>We can either: </div><div>1. Expose a priority number </div><div>2. Expose a priority enum, e.g. PRIORITY_HIGH, PRIORITY_NORMAL (to start) </div><div>The HW can do numerical priorities, but the SW scheduler would require a bit of rework to consume arbitrary priorities. </div><div>At least for our use case, I don't really see a need for super fine-grained priorities. Something like HIGH/NORMAL is sufficient, with possibility of expanding to add REALTIME/LOW. So I think going for an enum is simpler/better. </div><div>yay or nay on enum? </div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Jan 3, 2017 at 6:02 PM, Alex Deucher <<a href="mailto:alexdeucher@gmail.com" target="_blank">alexdeucher@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jan 3, 2017 at 6:00 PM, Andres Rodriguez <<a href="mailto:andresx7@gmail.com">andresx7@gmail.com</a>> wrote: > I was thinking of that originally, but the allocation context already has a > flags variable which allows us to preserve the IOCTL ABI. > > We could reserve a few bits of the flags for a priority level instead if > that sounds good? We can also use _pad for something else. Alex <div class="HOEnZb"><div class="h5"> > > Regards, > Andres > > On Tue, Jan 3, 2017 at 5:59 PM, Alex Deucher <<a href="mailto:alexdeucher@gmail.com">alexdeucher@gmail.com</a>> wrote: >> >> On Tue, Jan 3, 2017 at 5:54 PM, Andres Rodriguez <<a href="mailto:andresx7@gmail.com">andresx7@gmail.com</a>> >> wrote: >> > Add a new context creation flag, AMDGPU_CTX_FLAG_HIGHPRIORITY. This flag >> > results in the allocated context receiving a higher scheduler priority >> > that other contexts system-wide. >> > >> > Signed-off-by: Andres Rodriguez <<a href="mailto:andresx7@gmail.com">andresx7@gmail.com</a>> >> > --- >> > drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 24 >> > ++++++++++++++++++------ >> > drivers/gpu/drm/amd/scheduler/gpu_scheduler.h | 1 + >> > include/uapi/drm/amdgpu_drm.h | 3 ++- >> > 3 files changed, 21 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >> > index 400c66b..ce470e2 100644 >> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c >> > @@ -25,11 +25,17 @@ >> > #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, int priority, >> > struct amdgpu_ctx *ctx) >> > { >> > unsigned i, j; >> > int r; >> > >> > + if (priority < 0 || priority >= AMD_SCHED_MAX_PRIORITY) >> > + return -EINVAL; >> > + >> > + if (priority == AMD_SCHED_PRIORITY_HIGH && >> > !capable(CAP_SYS_ADMIN)) >> > + return -EACCES; >> > + >> > memset(ctx, 0, sizeof(*ctx)); >> > ctx->adev = adev; >> > kref_init(&ctx->refcount); >> > @@ -51,7 +57,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,11 +96,15 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx) >> > >> > static int amdgpu_ctx_alloc(struct amdgpu_device *adev, >> > struct amdgpu_fpriv *fpriv, >> > + int flags, >> > uint32_t *id) >> > { >> > struct amdgpu_ctx_mgr *mgr = &fpriv->ctx_mgr; >> > struct amdgpu_ctx *ctx; >> > - int r; >> > + int r, priority; >> > + >> > + priority = flags & AMDGPU_CTX_FLAG_HIGHPRIORITY ? >> > + AMD_SCHED_PRIORITY_HIGH : AMD_SCHED_PRIORITY_NORMAL; >> > >> > ctx = kmalloc(sizeof(*ctx), GFP_KERNEL); >> > if (!ctx) >> > @@ -107,8 +117,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; >> > @@ -186,7 +197,7 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void >> > *data, >> > struct drm_file *filp) >> > { >> > int r; >> > - uint32_t id; >> > + uint32_t id, flags; >> > >> > union drm_amdgpu_ctx *args = data; >> > struct amdgpu_device *adev = dev->dev_private; >> > @@ -194,10 +205,11 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void >> > *data, >> > >> > r = 0; >> > id = args->in.ctx_id; >> > + flags = args->in.flags; >> > >> > switch (args->in.op) { >> > case AMDGPU_CTX_OP_ALLOC_CTX: >> > - r = amdgpu_ctx_alloc(adev, fpriv, &id); >> > + r = amdgpu_ctx_alloc(adev, fpriv, flags, &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 d8dc681..2e458de 100644 >> > --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h >> > +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h >> > @@ -108,6 +108,7 @@ struct amd_sched_backend_ops { >> > >> > enum amd_sched_priority { >> > AMD_SCHED_PRIORITY_KERNEL = 0, >> > + AMD_SCHED_PRIORITY_HIGH, >> > AMD_SCHED_PRIORITY_NORMAL, >> > AMD_SCHED_MAX_PRIORITY >> > }; >> > diff --git a/include/uapi/drm/amdgpu_drm.h >> > b/include/uapi/drm/amdgpu_drm.h >> > index 3961836..702332e 100644 >> > --- a/include/uapi/drm/amdgpu_drm.h >> > +++ b/include/uapi/drm/amdgpu_drm.h >> > @@ -160,10 +160,11 @@ union drm_amdgpu_bo_list { >> > /* unknown cause */ >> > #define AMDGPU_CTX_UNKNOWN_RESET 3 >> > >> > +#define AMDGPU_CTX_FLAG_HIGHPRIORITY (1 << 0) >> >> Would it be better to expose a priority level rather than a single >> flag? If we want to expose more than just normal/high in the future >> for example. >> >> Alex >> >> > struct drm_amdgpu_ctx_in { >> > /** AMDGPU_CTX_OP_* */ >> > __u32 op; >> > - /** For future use, no flags defined so far */ >> > + /** AMDGPU_CTX_FLAG_* */ >> > __u32 flags; >> > __u32 ctx_id; >> > __u32 _pad; >> > -- >> > 2.7.4 >> > >> > _______________________________________________ >> > amd-gfx mailing list >> > <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> >> > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> > > </div></div></blockquote></div> </div>