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

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

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.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev