[igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
Ser, Simon
simon.ser at intel.com
Fri May 24 08:27:02 UTC 2019
On Fri, 2019-05-24 at 09:20 +0100, Tvrtko Ursulin wrote:
> On 24/05/2019 08:25, Chris Wilson wrote:
> > Apparently VLA structs (e.g. struct { int array[count] }) is a gcc
> > extension that clang refuses to support as handling memory layout is too
> > difficult for it.
> >
> > Move the on-stack VLA to the heap.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> > benchmarks/gem_wsim.c | 146 +++++++++++++++++++++++++++---------------
> > 1 file changed, 95 insertions(+), 51 deletions(-)
> >
> > diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> > index e2ffb93a9..0a0032bff 100644
> > --- a/benchmarks/gem_wsim.c
> > +++ b/benchmarks/gem_wsim.c
> > @@ -1441,6 +1441,48 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask)
> > return slice_mask;
> > }
> >
> > +static size_t sizeof_load_balance(int count)
> > +{
> > + struct i915_context_engines_load_balance *ptr;
> > +
> > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]));
>
> This seems wrong - is bound to trigger.
>
> > + return sizeof(*ptr) + sizeof(ptr->engines[count]);
>
> So size of of engine needs to be multiplied by count.
>
> > +}
> > +
> > +static struct i915_context_engines_load_balance *
> > +alloc_load_balance(int count)
> > +{
> > + return calloc(1, sizeof_load_balance(count));
>
> How about alloca so cleanup is simpler? Or is alloca also on the
> unpopular list?
>
> Or possibly what Simon suggested, just a large temporary stack arrays
> would be enough and easiest diff. Just with an assert that it fits.
>
> I can do that if you want?
I think Arek already has a patch for this.
> Regards,
>
> Tvrtko
>
> > +}
> > +
> > +static size_t sizeof_param_engines(int count)
> > +{
> > + struct i915_context_param_engines *ptr;
> > +
> > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]));
> > + return sizeof(*ptr) + sizeof(ptr->engines[count]);
> > +}
> > +
> > +static struct i915_context_param_engines *
> > +alloc_param_engines(int count)
> > +{
> > + return calloc(1, sizeof_param_engines(count));
> > +}
> > +
> > +static size_t sizeof_engines_bond(int count)
> > +{
> > + struct i915_context_engines_bond *ptr;
> > +
> > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]));
> > + return sizeof(*ptr) + sizeof(ptr->engines[count]);
> > +}
> > +
> > +static struct i915_context_engines_bond *
> > +alloc_engines_bond(int count)
> > +{
> > + return calloc(1, sizeof_engines_bond(count));
> > +}
> > +
> > static int
> > prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
> > {
> > @@ -1676,66 +1718,54 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
> > }
> >
> > if (ctx->engine_map) {
> > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines,
> > - ctx->engine_map_count + 1);
> > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance,
> > - ctx->engine_map_count);
> > + struct i915_context_param_engines *set_engines =
> > + alloc_param_engines(ctx->engine_map_count + 1);
> > + struct i915_context_engines_load_balance *load_balance =
> > + alloc_load_balance(ctx->engine_map_count);
> > 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),
> > + .size = sizeof_param_engines(ctx->engine_map_count + 1),
> > + .value = to_user_pointer(set_engines),
> > };
> > + struct i915_context_engines_bond *last = NULL;
> >
> > if (ctx->wants_balance) {
> > - set_engines.extensions =
> > - to_user_pointer(&load_balance);
> > + set_engines->extensions =
> > + to_user_pointer(load_balance);
> >
> > - memset(&load_balance, 0, sizeof(load_balance));
> > - load_balance.base.name =
> > + load_balance->base.name =
> > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
> > - load_balance.num_siblings =
> > + load_balance->num_siblings =
> > ctx->engine_map_count;
> >
> > for (j = 0; j < ctx->engine_map_count; j++)
> > - load_balance.engines[j] =
> > + load_balance->engines[j] =
> > get_engine(ctx->engine_map[j]);
> > - } else {
> > - set_engines.extensions = 0;
> > }
> >
> > /* Reserve slot for virtual engine. */
> > - set_engines.engines[0].engine_class =
> > + set_engines->engines[0].engine_class =
> > I915_ENGINE_CLASS_INVALID;
> > - set_engines.engines[0].engine_instance =
> > + set_engines->engines[0].engine_instance =
> > I915_ENGINE_CLASS_INVALID_NONE;
> >
> > for (j = 1; j <= ctx->engine_map_count; j++)
> > - set_engines.engines[j] =
> > + set_engines->engines[j] =
> > get_engine(ctx->engine_map[j - 1]);
> >
> > + last = NULL;
> > for (j = 0; j < ctx->bond_count; j++) {
> > unsigned long mask = ctx->bonds[j].mask;
> > - I915_DEFINE_CONTEXT_ENGINES_BOND(bond,
> > - __builtin_popcount(mask));
> > - struct i915_context_engines_bond *p = NULL, *prev;
> > + struct i915_context_engines_bond *bond =
> > + alloc_engines_bond(__builtin_popcount(mask));
> > unsigned int b, e;
> >
> > - prev = p;
> > - p = alloca(sizeof(bond));
> > - assert(p);
> > - memset(p, 0, sizeof(bond));
> > -
> > - if (j == 0)
> > - load_balance.base.next_extension =
> > - to_user_pointer(p);
> > - else if (j < (ctx->bond_count - 1))
> > - prev->base.next_extension =
> > - to_user_pointer(p);
> > + bond->base.next_extension = to_user_pointer(last);
> > + bond->base.name = I915_CONTEXT_ENGINES_EXT_BOND;
> >
> > - p->base.name = I915_CONTEXT_ENGINES_EXT_BOND;
> > - p->virtual_index = 0;
> > - p->master = get_engine(ctx->bonds[j].master);
> > + bond->virtual_index = 0;
> > + bond->master = get_engine(ctx->bonds[j].master);
> >
> > for (b = 0, e = 0; mask; e++, mask >>= 1) {
> > unsigned int idx;
> > @@ -1743,44 +1773,58 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
> > if (!(mask & 1))
> > continue;
> >
> > - idx = find_engine(&set_engines.engines[1],
> > + idx = find_engine(&set_engines->engines[1],
> > ctx->engine_map_count,
> > e);
> > - p->engines[b++] =
> > - set_engines.engines[1 + idx];
> > + bond->engines[b++] =
> > + set_engines->engines[1 + idx];
> > }
> > +
> > + last = bond;
> > }
> > + load_balance->base.next_extension = to_user_pointer(last);
> >
> > gem_context_set_param(fd, ¶m);
> > +
> > + while (last) {
> > + struct i915_context_engines_bond *next =
> > + from_user_pointer(last->base.next_extension);
> > + free(last);
> > + last = next;
> > + }
> > + free(load_balance);
> > + free(set_engines);
> > } else if (ctx->wants_balance) {
> > const unsigned int count = num_engines_in_class(VCS);
> > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance,
> > - count);
> > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines,
> > - count + 1);
> > + struct i915_context_engines_load_balance *load_balance =
> > + alloc_load_balance(count);
> > + struct i915_context_param_engines *set_engines =
> > + alloc_param_engines(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),
> > + .size = sizeof_param_engines(count + 1),
> > + .value = to_user_pointer(set_engines),
> > };
> >
> > - set_engines.extensions = to_user_pointer(&load_balance);
> > + set_engines->extensions = to_user_pointer(load_balance);
> >
> > - set_engines.engines[0].engine_class =
> > + set_engines->engines[0].engine_class =
> > I915_ENGINE_CLASS_INVALID;
> > - set_engines.engines[0].engine_instance =
> > + set_engines->engines[0].engine_instance =
> > I915_ENGINE_CLASS_INVALID_NONE;
> > - fill_engines_class(&set_engines.engines[1], VCS);
> > + fill_engines_class(&set_engines->engines[1], VCS);
> >
> > - memset(&load_balance, 0, sizeof(load_balance));
> > - load_balance.base.name =
> > + load_balance->base.name =
> > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
> > - load_balance.num_siblings = count;
> > + load_balance->num_siblings = count;
> >
> > - fill_engines_class(&load_balance.engines[0], VCS);
> > + fill_engines_class(&load_balance->engines[0], VCS);
> >
> > gem_context_set_param(fd, ¶m);
> > +
> > + free(set_engines);
> > + free(load_balance);
> > }
> >
> > if (wrk->sseu) {
> >
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list