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

[Tvrtko Ursulin]

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?

These FIXMEs can be addressed by either adding engine discovery or 
fixing the code to not assume class and engines to be balanced.

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.

But even if the need to change it arises, I think wouldn't be a problem 
if left for later.

Regards,

Tvrtko