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

Tvrtko Ursulin
2019-05-13T13:18:59Z

On 10/05/2019 14:26, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-08 13:10:48)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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