[igt-dev] [PATCH i-g-t 11/21] gem_wsim: Engine map support

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-13 14:18:59)
> 
> On 10/05/2019 14:26, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-08 13:10:48)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> Support new i915 uAPI for configuring contexts with engine maps.
> >>
> >> Please refer to the README file for more detailed explanation.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> ---
> >> +static int parse_engine_map(struct w_step *step, const char *_str)
> >> +{
> >> +       char *token, *tctx = NULL, *tstart = (char *)_str;
> >> +
> >> +       while ((token = strtok_r(tstart, "|", &tctx))) {
> >> +               enum intel_engine_id engine;
> >> +
> >> +               tstart = NULL;
> >> +
> >> +               if (!strcmp(token, "DEFAULT"))
> >> +                       return -1;
> >> +               else if (!strcmp(token, "VCS"))
> >> +                       return -1;
> >> +
> >> +               engine = str_to_engine(token);
> >> +               if ((int)engine < 0)
> >> +                       return -1;
> >> +
> >> +               if (engine != VCS1 && engine != VCS2)
> >> +                       return -1; /* TODO */
> >> +
> >> +               step->engine_map_count++;
> >> +               step->engine_map = realloc(step->engine_map,
> >> +                                          step->engine_map_count *
> >> +                                          sizeof(step->engine_map[0]));
> >> +               step->engine_map[step->engine_map_count - 1] = engine;
> > 
> > 
> >> +               if (ctx->engine_map) {
> >> +                       I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines,
> >> +                                                         ctx->engine_map_count + 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),
> >> +                       };
> >> +
> >> +                       set_engines.extensions = 0;
> >> +
> >> +                       /* Reserve slot for virtual engine. */
> >> +                       set_engines.engines[0].engine_class =
> >> +                               I915_ENGINE_CLASS_INVALID;
> >> +                       set_engines.engines[0].engine_instance =
> >> +                               I915_ENGINE_CLASS_INVALID_NONE;
> >> +
> >> +                       for (j = 1; j <= ctx->engine_map_count; j++) {
> >> +                               set_engines.engines[j].engine_class =
> >> +                                       I915_ENGINE_CLASS_VIDEO; /* FIXME */
> >> +                               set_engines.engines[j].engine_instance =
> >> +                                       ctx->engine_map[j - 1] - VCS1; /* FIXME */
> >> +                       }
> > 
> > I would suggest the file format starts with class:instance specifiers.
> > Too much FIXME that I think will need a file format change.
> 
> Where do you see the need for a file format change?

Nah, I made the assumption the FIXMEs were because the implementation
was dictated by the file format.
 
> These FIXMEs can be addressed by either adding engine discovery or 
> fixing the code to not assume class and engines to be balanced.

The code is just obeying the .wsim; the question is how to handle a
mismatch between the file and hw -- whether to do a transparent fixup to
use bcs instead of a secondary vcs?
 
> Larger rework might be needed to deal with the internal engine 
> representation after adding engine discovery. Or at least an audit and 
> checking legacy paths. Might be that refactor would be limited to engine 
> string to internal engine id lookup.
> 
> But to change file format I don't see an immediate need. VCS is already 
> defined as any VCS and there are explicit VCS1 and VCS2.

I was more concerned in case vcs was implicit since it was heavily
assumed by the code.
-Chris