Re: [PATCH v2 09/11] drm/i915/scheduler: Support user-defined priorities

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/11/2016 13:59, Chris Wilson wrote:
> Use a priority stored in the context as the initial value when
> submitting a request. This allows us to change the default priority on a
> per-context basis, allowing different contexts to be favoured with GPU
> time at the expense of lower importance work. The user can adjust the
> context's priority via I915_CONTEXT_PARAM_PRIORITY, with more positive
> values being higher priority (they will be serviced earlier, after their
> dependencies have been resolved). Any prerequisite work for an execbuf
> will have its priority raised to match the new request as required.
>
> Normal users can specify any value in the range of -1023 to 0 [default],
> i.e. they can reduce the priority of their workloads (and temporarily
> boost it back to normal if so desired).
>
> Privileged users can specify any value in the range of -1023 to 1023,
> [default is 0], i.e. they can raise their priority above all overs and
> so potentially starve the system.
>
> Note that the existing schedulers are not fair, nor load balancing, the
> execution is strictly by priority on a first-come, first-served basis,
> and the driver may choose to boost some requests above the range
> available to users.
>
> This priority was originally based around nice(2), but evolved to allow
> clients to adjust their priority within a small range, and allow for a
> privileged high priority range.
>
> For example, this can be used to implement EGL_IMG_context_priority
> https://www.khronos.org/registry/egl/extensions/IMG/EGL_IMG_context_priority.txt
>
> 	EGL_CONTEXT_PRIORITY_LEVEL_IMG determines the priority level of
>         the context to be created. This attribute is a hint, as an
>         implementation may not support multiple contexts at some
>         priority levels and system policy may limit access to high
>         priority contexts to appropriate system privilege level. The
>         default value for EGL_CONTEXT_PRIORITY_LEVEL_IMG is
>         EGL_CONTEXT_PRIORITY_MEDIUM_IMG."
>
> so we can map
>
> 	PRIORITY_HIGH -> 1023 [privileged, will failback to 0]
> 	PRIORITY_MED -> 0 [default]
> 	PRIORITY_LOW -> -1023
>
> They also map onto the priorities used by VkQueue (and a VkQueue is
> essentially a timeline, our i915_gem_context under full-ppgtt).
>
> Testcase: igt/gem_exec_schedule
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_request.c |  2 +-
>  include/uapi/drm/i915_drm.h             |  3 +++
>  4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5c658c6d06e4..d253aeee0fb2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -949,6 +949,7 @@ struct i915_gem_context {
>  	/* Unique identifier for this context, used by the hw for tracking */
>  	unsigned int hw_id;
>  	u32 user_handle;
> +	int priority; /* greater priorities are serviced first */
>
>  	u32 ggtt_alignment;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6dd475735f0a..48b5aacf5fc2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -476,6 +476,7 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>
> +	ctx->priority = -I915_PRIORITY_MAX; /* lowest priority; idle task */

Does this matter at all? What are we submitting from the kernel context? 
If we are some things then is it even correct?

Another thing, -I915_PRIORITY_MAX looks strange, why not have 
I915_PRIORITY_MIN?

>  	dev_priv->kernel_context = ctx;
>
>  	DRM_DEBUG_DRIVER("%s context support initialized\n",
> @@ -1100,6 +1101,9 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>  	case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE:
>  		args->value = !!(ctx->flags & CONTEXT_NO_ERROR_CAPTURE);
>  		break;
> +	case I915_CONTEXT_PARAM_PRIORITY:
> +		args->value = ctx->priority;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -1155,6 +1159,23 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  				ctx->flags &= ~CONTEXT_NO_ERROR_CAPTURE;
>  		}
>  		break;
> +
> +	case I915_CONTEXT_PARAM_PRIORITY:
> +		{
> +			int priority = args->value;
> +
> +			if (args->size)
> +				ret = -EINVAL;
> +			else if (priority >= I915_PRIORITY_MAX ||
> +				 priority <= -I915_PRIORITY_MAX)
> +				ret = -EINVAL;
> +			else if (priority > 0 && !capable(CAP_SYS_ADMIN))
> +				ret = -EPERM;
> +			else
> +				ctx->priority = priority;
> +		}
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 278b103a4e95..cfda095f0234 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -861,7 +861,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, bool flush_caches)
>  	 * run at the earliest possible convenience.
>  	 */
>  	if (engine->schedule)
> -		engine->schedule(request, 0);
> +		engine->schedule(request, request->ctx->priority);
>
>  	local_bh_disable();
>  	i915_sw_fence_commit(&request->submit);
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 1c12a350eca3..47901a8ad682 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -391,6 +391,8 @@ typedef struct drm_i915_irq_wait {
>
>  /* Query whether DRM_I915_GEM_EXECBUFFER2 supports user defined execution
>   * priorities and the driver will attempt to execute batches in priority order.
> + * The initial priority for each batch is supplied by the context and is
> + * controlled via I915_CONTEXT_PARAM_PRIORITY.
>   */
>  #define I915_PARAM_HAS_SCHEDULER	 41
>
> @@ -1224,6 +1226,7 @@ struct drm_i915_gem_context_param {
>  #define I915_CONTEXT_PARAM_NO_ZEROMAP	0x2
>  #define I915_CONTEXT_PARAM_GTT_SIZE	0x3
>  #define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE	0x4
> +#define I915_CONTEXT_PARAM_PRIORITY	0x5
>  	__u64 value;
>  };
>
>

Otherwise looks OK to me.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx