Re: [igt-dev] [PATCH i-g-t 05/21] wsim/media-bench: i915 balancing

Chris Wilson
2019-05-10T13:14:47Z

Quoting Tvrtko Ursulin (2019-05-08 13:10:42)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Support i915 virtual engine from gem_wsim (-b i915) and media-bench.pl
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> + /*
> + * Create and configure contexts.
> + */
> + for (i = 0; i < wrk->nr_ctxs; i += 2) {
> + struct ctx *ctx = &wrk->ctx_list[i];
> + uint32_t ctx_id, share_vm = 0;
>
> - wrk->ctx_list[w->context].id = arg.ctx_id;
> + if (ctx->id)
> + continue;
>
> - if (flags & GLOBAL_BALANCE) {
> - wrk->ctx_list[w->context].static_vcs = context_vcs_rr;
> - context_vcs_rr ^= 1;
> - } else {
> - wrk->ctx_list[w->context].static_vcs = ctx_vcs;
> - ctx_vcs ^= 1;
> - }
> + if (flags & I915) {

vm sharing shouldn't be a i915-balancer only option. For single jobs split
across multiple contexts, I would expect they will want to share vm.

> + struct drm_i915_gem_context_create_ext_setparam ext = {
> + .base.name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> + .param.param = I915_CONTEXT_PARAM_VM,
> + };
> + struct drm_i915_gem_context_create_ext args = { };
>
> - if (wrk->prio) {
> + /* Find existing context to share ppgtt with. */
> + for (j = 0; j < wrk->nr_ctxs; j++) {
> struct drm_i915_gem_context_param param = {
> - .ctx_id = arg.ctx_id,
> - .param = I915_CONTEXT_PARAM_PRIORITY,
> - .value = wrk->prio,
> + .param = I915_CONTEXT_PARAM_VM,
> };
> - gem_context_set_param(fd, &param);
> +
> + if (!wrk->ctx_list[j].id)
> + continue;
> +
> + param.ctx_id = wrk->ctx_list[j].id;
> +
> + gem_context_get_param(fd, &param);
> + igt_assert(param.value);
> +
> + share_vm = param.value;
> +
> + ext.param.value = share_vm;
> + args.flags =
> + I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS;
> + args.extensions = to_user_pointer(&ext);
> + break;
> }
> +
> + if (!ctx->targets_instance)
> + args.flags |=
> + I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE;
> +
> + drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT,
> + &args);
> +
> + ctx_id = args.ctx_id;
> + } else {
> + struct drm_i915_gem_context_create args = {};
> +
> + drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &args);
> + ctx_id = args.ctx_id;
> + }
> +
> + igt_assert(ctx_id);
> + ctx->id = ctx_id;
> +
> + if (flags & GLOBAL_BALANCE) {
> + ctx->static_vcs = context_vcs_rr;
> + context_vcs_rr ^= 1;
> + } else {
> + ctx->static_vcs = ctx_vcs;
> + ctx_vcs ^= 1;
> + }
> +
> + __ctx_set_prio(ctx_id, wrk->prio);
> +
> + /*
> + * Do we need a separate context to satisfy this workloads which
> + * both want to target specific engines and be balanced by i915?
> + */
> + if ((flags & I915) && ctx->wants_balance &&
> + ctx->targets_instance) {
> + struct drm_i915_gem_context_create_ext_setparam ext = {
> + .base.name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> + .param.param = I915_CONTEXT_PARAM_VM,
> + .param.value = share_vm,
> + };
> + struct drm_i915_gem_context_create_ext args = {
> + .extensions = to_user_pointer(&ext),
> + .flags =
> + I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS |
> + I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE,
> + };
> +
> + igt_assert(share_vm);
> +
> + drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT,
> + &args);
> +
> + igt_assert(args.ctx_id);
> + ctx_id = args.ctx_id;
> + wrk->ctx_list[i + 1].id = args.ctx_id;
> +
> + __ctx_set_prio(ctx_id, wrk->prio);
> + }
> +
> + if (ctx->wants_balance) {
> + I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance, 2) = {
> + .base.name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE,
> + .num_siblings = 2,
> + .engines = {
> + { .engine_class = I915_ENGINE_CLASS_VIDEO,
> + .engine_instance = 0 },
> + { .engine_class = I915_ENGINE_CLASS_VIDEO,
> + .engine_instance = 1 },
> + },
> + };
> + I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines, 3) = {
> + .extensions = to_user_pointer(&load_balance),
> + .engines = {
> + { .engine_class = I915_ENGINE_CLASS_INVALID,
> + .engine_instance = I915_ENGINE_CLASS_INVALID_NONE },
> + { .engine_class = I915_ENGINE_CLASS_VIDEO,
> + .engine_instance = 0 },
> + { .engine_class = I915_ENGINE_CLASS_VIDEO,
> + .engine_instance = 1 },
> + },
> + };
> +
> + struct drm_i915_gem_context_param param = {
> + .ctx_id = ctx_id,
> + .param = I915_CONTEXT_PARAM_ENGINES,
> + .size = sizeof(set_engines),
> + .value = to_user_pointer(&set_engines),
> + };
> +
> + gem_context_set_param(fd, &param);
> }

if (share_vm)
gem_vm_destroy(share_vm);

Just to drop the local handle as the context has acquired its own
reference.

Other than that, it does what it sets out to do: create a context with
choice of engines and load balancing amongst them.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris