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

[Tvrtko Ursulin]

On 10/05/2019 14:14, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-08 13:10:42)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Support i915 virtual engine from gem_wsim (-b i915) and media-bench.pl
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at 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.

Could do but I wanted to limit the new features to new features. :) 
Pencil in for later okay?

>> +                       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.

Well spotted!

> 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 at chris-wilson.co.uk>

Thanks,

Tvrtko