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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 13/05/2019 14:29, Chris Wilson wrote:
> 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@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?
> 
> 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?

It would be of little use since we cannot load balance the two...

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

...and the thing I just realised I am not actually happy with is the 
lack of ability to write portable .wsim's when using engine maps.

Legacy files can configure implicit engine maps based on class (VCS), so 
I think the engine map command needs the same capability. Otherwise the 
.wsim's won't be portable. I want to be able to do:

M.1.VCS
B.1

And that to mean create engine map with all VCS class engines and enable 
load balancing. It can be achieved with legacy (implicit) load balancing 
but that cannot be tied with frame split.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev