Re: [igt-dev] [PATCH i-g-t 15/17] gem_wsim: Engine bond command

Tvrtko Ursulin
2018-10-26T11:11:46Z

On 18/10/2018 22:48, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-10-18 16:28:13)
>> @@ -1196,6 +1256,7 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
>> { .base.name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE,
>> .engines_mask = -1,
>> };
>> + struct i915_context_engines_bond *bonds = NULL;
>>
>> if (ctx->wants_balance) {
>> set_engines.extensions =
>> @@ -1211,7 +1272,31 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
>> ctx->engine_map[j] - VCS1; /* FIXME */
>> }
>>
>> + if (ctx->bond_count) {
>> + bonds = calloc(ctx->bond_count, sizeof(*bonds));
>> + load_balance.base.next_extension =
>> + to_user_pointer(&bonds[0]);
>> + }
>> +
>> + for (j = 0; j < ctx->bond_count; j++) {
>> + struct i915_context_engines_bond *bond =
>> + &bonds[j];
>> +
>> + if (j < (ctx->bond_count - 1))
>> + bond->base.next_extension =
>> + to_user_pointer(bond + 1);
>> +
>> + bond->base.name = I915_CONTEXT_ENGINES_EXT_BOND;
>> + bond->master_class = I915_ENGINE_CLASS_VIDEO;
>> + bond->master_instance =
>> + ctx->bonds[j].master - VCS1;
>> + bond->sibling_mask = ctx->bonds[j].mask;
>> + }
>> +
>> gem_context_set_param(fd, &param);
>> +
>> + if (bonds)
>> + free(bonds);
>
> free(NULL) is legal, so just free(bonds) here.

The only one i am usually sure of is kfree. :)

> Looking at how you have constructed the map for the extension, I'm
> reasonably happy with how this works in practice (outside of the igt
> tests). Is the flexibility (of next_extension and separate bond structs)
> too much?

Shrug. My only gripe is that it is a nice generic mechanism used only in
ctx set param, which feels a bit on it's head (*), but in principle I
think it is fine.

Regards,

Tvrtko

(*) Sounds like it would be nicer if it was a core concept in more of
our uapi's. Going from the bottom up like create_context + extension
this + extension that. And the same for other our ioctls, where
applicable. But perhaps it is a stretch it would fit to many more places.