[Intel-gfx] [igt-dev] [PATCH i-g-t 14/21] gem_wsim: Engine map load balance command

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-15 12:44:41)
> 
> On 10/05/2019 14:31, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-08 13:10:51)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> A new workload command for enabling a load balanced context map (aka
> >> Virtual Engine). Example usage:
> >>
> >>    B.1
> >>
> >> This turns on load balancing for context one, assuming it has already been
> >> configured with an engine map. Only DEFAULT engine specifier can be used
> >> with load balanced engine maps.
> > 
> > Restriction makes sense for keeping linenoise^W file format simple.
> > 
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> ---
> >> @@ -1172,6 +1210,8 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
> >>                  if (ctx->engine_map) {
> >>                          I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines,
> >>                                                            ctx->engine_map_count + 1);
> >> +                       I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance,
> >> +                                                                ctx->engine_map_count);
> >>                          struct drm_i915_gem_context_param param = {
> >>                                  .ctx_id = ctx_id,
> >>                                  .param = I915_CONTEXT_PARAM_ENGINES,
> >> @@ -1179,7 +1219,25 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
> >>                                  .value = to_user_pointer(&set_engines),
> >>                          };
> >>   
> >> -                       set_engines.extensions = 0;
> >> +                       if (ctx->wants_balance) {
> >> +                               set_engines.extensions =
> >> +                                       to_user_pointer(&load_balance);
> >> +
> >> +                               memset(&load_balance, 0, sizeof(load_balance));
> >> +                               load_balance.base.name =
> >> +                                       I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
> >> +                               load_balance.num_siblings =
> >> +                                       ctx->engine_map_count;
> >> +
> >> +                               for (j = 0; j < ctx->engine_map_count; j++) {
> >> +                                       load_balance.engines[j].engine_class =
> >> +                                               I915_ENGINE_CLASS_VIDEO; /* FIXME */
> >> +                                       load_balance.engines[j].engine_instance =
> >> +                                               ctx->engine_map[j] - VCS1; /* FIXME */
> > 
> > Ok, more fallout from fixing ctx->engine_map[] first?
> 
> Not sure I understand the question.

The proliferation of FIXME, the assumption of CLASS_VIDEO and an
impedance mismatch between engine_map and class:instance. Basically
those FIXME raise the question of what do you intend this to look like?
-Chris