On Mon, Oct 04, 2021 at 03:36:49PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Introduce the concept of context nice value which matches the process
> nice.
>
> We do this by extending the struct i915_sched_attr and add a helper
> (i915_sched_attr_priority) to be used to convert to effective priority
> when used by backend code and for priority sorting.
>
> Context nice is then inherited from the process which creates the GEM
> context and utilised secondary to context priority, only when the latter
> has been left at the default setting, in order to avoid disturbing any
> application made choices of low and high (batch processing and maybe
> latency sensitive compositing). In those cases nice value adjusts the
> effective priority in the narrow band of -19 to +20 around
> I915_CONTEXT_DEFAULT_PRIORITY.
>
> This means that in theory userspace using the context priority uapi
> directly has a wider range of possible adjustments (thought to be
> beneficial), but in practice that only applies to execlists platforms.
> With GuC there are only three priority buckets (less than zero is low
> priority, zero is normal and greater than zero is high) which therefore
> interact as expected with the nice adjustment. It makes the question of
> should the nice be a sub-range of GEM priorities, or stretched across the
> whole, a moot one.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 1 +
> .../gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++--
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
> drivers/gpu/drm/i915/i915_request.c | 2 +-
> drivers/gpu/drm/i915/i915_request.h | 5 +++++
> drivers/gpu/drm/i915/i915_scheduler.c | 12 ++++++++----
> drivers/gpu/drm/i915/i915_scheduler.h | 14 ++++++++++++++
> drivers/gpu/drm/i915/i915_scheduler_types.h | 8 ++++++++
> 8 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 8d4d687ab1d0..fed0733cb652 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -257,6 +257,7 @@ proto_context_create(struct drm_i915_private *i915, unsigned int flags)
> if (i915->params.enable_hangcheck)
> pc->user_flags |= BIT(UCONTEXT_PERSISTENCE);
> pc->sched.priority = I915_PRIORITY_NORMAL;
> + pc->sched.nice = task_nice(current);
>
> if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {
> if (!HAS_EXECLISTS(i915)) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index e91d803a6453..1a02c65823a7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -250,7 +250,7 @@ static struct i915_priolist *to_priolist(struct rb_node *rb)
>
> static int rq_prio(const struct i915_request *rq)
> {
> - return READ_ONCE(rq->sched.attr.priority);
> + return i915_request_priority(rq);
> }
>
> static int effective_prio(const struct i915_request *rq)
> @@ -3221,8 +3221,8 @@ static void kick_execlists(const struct i915_request *rq,
> {
> struct intel_engine_cs *engine = rq->engine;
> struct i915_sched_engine *sched_engine = engine->sched_engine;
> + const int prio = i915_sched_attr_priority(attr);
> const struct i915_request *inflight;
> - const int prio = attr->priority;
>
> /*
> * We only need to kick the tasklet once for the high priority
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index b5883a4365ca..f258607685a2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2417,7 +2417,7 @@ static void guc_bump_inflight_request_prio(struct i915_request *rq,
> const struct i915_sched_attr *attr)
> {
> struct intel_context *ce = rq->context;
> - const int prio = attr->priority;
> + const int prio = i915_sched_attr_priority(attr);
> u8 new_guc_prio = map_i915_prio_to_guc_prio(prio);
>
> /* Short circuit function */
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 79da5eca60af..a8c6f3a64895 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1930,7 +1930,7 @@ static int print_sched_attr(const struct i915_sched_attr *attr,
> return x;
>
> x += snprintf(buf + x, len - x,
> - " prio=%d", attr->priority);
> + " prio=%d nice=%d", attr->priority, attr->nice);
>
> return x;
> }
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 7bd9ed20623e..c2c4c344837e 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -399,6 +399,11 @@ long i915_request_wait(struct i915_request *rq,
> #define I915_WAIT_PRIORITY BIT(1) /* small priority bump for the request */
> #define I915_WAIT_ALL BIT(2) /* used by i915_gem_object_wait() */
>
> +static inline int i915_request_priority(const struct i915_request *rq)
> +{
> + return i915_sched_attr_priority(&rq->sched.attr);
> +}
> +
> void i915_request_show(struct drm_printer *m,
> const struct i915_request *rq,
> const char *prefix,
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 534bab99fcdc..e75793e36454 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -155,7 +155,9 @@ lock_sched_engine(struct i915_sched_node *node,
> static void __i915_schedule(struct i915_sched_node *node,
> const struct i915_sched_attr *attr)
> {
> - const int prio = max(attr->priority, node->attr.priority);
> + const int prio =
> + max(i915_sched_attr_priority(attr),
> + i915_sched_attr_priority(&node->attr));
> struct i915_sched_engine *sched_engine;
> struct i915_dependency *dep, *p;
> struct i915_dependency stack;
> @@ -209,7 +211,7 @@ static void __i915_schedule(struct i915_sched_node *node,
> if (node_signaled(p->signaler))
> continue;
>
> - if (prio > READ_ONCE(p->signaler->attr.priority))
> + if (prio > i915_sched_attr_priority(&p->signaler->attr))
> list_move_tail(&p->dfs_link, &dfs);
> }
> }
> @@ -247,7 +249,8 @@ static void __i915_schedule(struct i915_sched_node *node,
> lockdep_assert_held(&sched_engine->lock);
>
> /* Recheck after acquiring the engine->timeline.lock */
> - if (prio <= node->attr.priority || node_signaled(node))
> + if (prio <= i915_sched_attr_priority(&node->attr) ||
> + node_signaled(node))
> continue;
>
> GEM_BUG_ON(node_to_request(node)->engine->sched_engine !=
> @@ -257,7 +260,7 @@ static void __i915_schedule(struct i915_sched_node *node,
> if (sched_engine->bump_inflight_request_prio)
> sched_engine->bump_inflight_request_prio(from, attr);
>
> - WRITE_ONCE(node->attr.priority, prio);
> + WRITE_ONCE(node->attr, *attr);
>
> /*
> * Once the request is ready, it will be placed into the
> @@ -305,6 +308,7 @@ void i915_sched_node_init(struct i915_sched_node *node)
> void i915_sched_node_reinit(struct i915_sched_node *node)
> {
> node->attr.priority = I915_PRIORITY_INVALID;
> + node->attr.nice = 0;
> node->semaphores = 0;
> node->flags = 0;
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index 0b9b86af6c7f..75ccc9f55d14 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -38,6 +38,20 @@ void i915_sched_node_fini(struct i915_sched_node *node);
> void i915_schedule(struct i915_request *request,
> const struct i915_sched_attr *attr);
>
> +static inline int i915_sched_attr_priority(const struct i915_sched_attr *attr)
> +{
> + int prio = attr->priority;
> +
> + /*
> + * Only allow I915_CONTEXT_DEFAULT_PRIORITY to be affected by the
> + * nice setting.
> + */
> + if (!prio)
> + prio = -attr->nice;
> +
> + return prio;
> +}
> +
> struct list_head *
> i915_sched_lookup_priolist(struct i915_sched_engine *sched_engine, int prio);
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h
> index 24b9ac1c2ce2..159237aa7609 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler_types.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h
> @@ -29,6 +29,14 @@ struct i915_sched_attr {
> * The &drm_i915_private.kernel_context is assigned the lowest priority.
> */
> int priority;
> +
> + /**
> + * @nice: context nice level
> + *
> + * Nice level follows the CPU scheduler nice value as set for the
> + * process owning the GPU context.
> + */
> + int nice;
Same comment as the previous patch, I'd avoid adding a field to this
structure as I don't think this will play with the DRM scheduler very
well. If anything I think we should drop i915_sched_attr completely and
just store these values directly in the request and pass around the
request rather than i915_sched_attr.
Matt
> };
>
> /*
> --
> 2.30.2
>
