All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  7:25 ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  7:25 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev

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@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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]));
+	return sizeof(*ptr) + sizeof(ptr->engines[count]);
+}
+
+static struct i915_context_engines_load_balance *
+alloc_load_balance(int count)
+{
+	return calloc(1, sizeof_load_balance(count));
+}
+
+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, &param);
+
+			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, &param);
+
+			free(set_engines);
+			free(load_balance);
 		}
 
 		if (wrk->sseu) {
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  7:25 ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  7:25 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Tvrtko Ursulin

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@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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]));
+	return sizeof(*ptr) + sizeof(ptr->engines[count]);
+}
+
+static struct i915_context_engines_load_balance *
+alloc_load_balance(int count)
+{
+	return calloc(1, sizeof_load_balance(count));
+}
+
+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, &param);
+
+			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, &param);
+
+			free(set_engines);
+			free(load_balance);
 		}
 
 		if (wrk->sseu) {
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
@ 2019-05-24  7:45   ` Ser, Simon
  -1 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  7:45 UTC (permalink / raw)
  To: intel-gfx, chris; +Cc: igt-dev

On Fri, 2019-05-24 at 08:25 +0100, 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.

IMHO using an upper bound would be much simpler.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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]));
> +	return sizeof(*ptr) + sizeof(ptr->engines[count]);
> +}
> +
> +static struct i915_context_engines_load_balance *
> +alloc_load_balance(int count)
> +{
> +	return calloc(1, sizeof_load_balance(count));
> +}
> +
> +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, &param);
> +
> +			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, &param);
> +
> +			free(set_engines);
> +			free(load_balance);
>  		}
>  
>  		if (wrk->sseu) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  7:45   ` Ser, Simon
  0 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  7:45 UTC (permalink / raw)
  To: intel-gfx, chris; +Cc: igt-dev, Ursulin, Tvrtko

On Fri, 2019-05-24 at 08:25 +0100, 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.

IMHO using an upper bound would be much simpler.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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]));
> +	return sizeof(*ptr) + sizeof(ptr->engines[count]);
> +}
> +
> +static struct i915_context_engines_load_balance *
> +alloc_load_balance(int count)
> +{
> +	return calloc(1, sizeof_load_balance(count));
> +}
> +
> +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, &param);
> +
> +			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, &param);
> +
> +			free(set_engines);
> +			free(load_balance);
>  		}
>  
>  		if (wrk->sseu) {
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
  (?)
  (?)
@ 2019-05-24  7:50 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-05-24  7:50 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: benchmarks/gem_wsim: Heap allocate VLA structs
URL   : https://patchwork.freedesktop.org/series/61085/
State : success

== Summary ==

CI Bug Log - changes from IGT_5012 -> IGTPW_3051
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/61085/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_3051 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [PASS][1] -> [DMESG-WARN][2] ([fdo#106387])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload-with-fault-injection:
    - {fi-icl-u3}:        [DMESG-WARN][3] ([fdo#110718]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/fi-icl-u3/igt@i915_module_load@reload-with-fault-injection.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/fi-icl-u3/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_selftest@live_contexts:
    - fi-skl-gvtdvm:      [DMESG-FAIL][5] ([fdo#110235]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - {fi-icl-u3}:        [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
  [fdo#110718]: https://bugs.freedesktop.org/show_bug.cgi?id=110718


Participating hosts (44 -> 40)
------------------------------

  Additional (1): fi-hsw-4770 
  Missing    (5): fi-kbl-soraka fi-ilk-m540 fi-skl-guc fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_5012 -> IGTPW_3051

  CI_DRM_6138: a17cee540f7ffd493c7944dea5ea6e2d32fdfdb4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3051: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/
  IGT_5012: 996b4346b6a7f2bd0919817648a4f7a382e59757 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
@ 2019-05-24  8:20   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  8:20 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

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, &param);
> +
> +			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, &param);
> +
> +			free(set_engines);
> +			free(load_balance);
>   		}
>   
>   		if (wrk->sseu) {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  8:20   ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  8:20 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

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, &param);
> +
> +			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, &param);
> +
> +			free(set_engines);
> +			free(load_balance);
>   		}
>   
>   		if (wrk->sseu) {
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  8:20   ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
@ 2019-05-24  8:27     ` Ser, Simon
  -1 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  8:27 UTC (permalink / raw)
  To: tvrtko.ursulin, intel-gfx, chris; +Cc: igt-dev

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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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, &param);
> > +
> > +			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, &param);
> > +
> > +			free(set_engines);
> > +			free(load_balance);
> >   		}
> >   
> >   		if (wrk->sseu) {
> > 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  8:27     ` Ser, Simon
  0 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  8:27 UTC (permalink / raw)
  To: tvrtko.ursulin, intel-gfx, chris; +Cc: igt-dev

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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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, &param);
> > +
> > +			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, &param);
> > +
> > +			free(set_engines);
> > +			free(load_balance);
> >   		}
> >   
> >   		if (wrk->sseu) {
> > 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  8:20   ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
@ 2019-05-24  8:33     ` Chris Wilson
  -1 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  8:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
> 
> 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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.

Why does it seem wrong? That's the calculation used previously, and the
ptr->engines[] was meant to be packed in order for
sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
I threw it in there to check if the calculation was sane.

> > +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
> 
> So size of of engine needs to be multiplied by count.

(Just note this is the what the current VLA evaluates to :)

> > +}
> > +
> > +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?

I don't mind. Would shave a few lines indeed, but we need the memsets
back. #define alloca0()?

> 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 don't think that is as clean for the long term.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  8:33     ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  8:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
> 
> 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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.

Why does it seem wrong? That's the calculation used previously, and the
ptr->engines[] was meant to be packed in order for
sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
I threw it in there to check if the calculation was sane.

> > +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
> 
> So size of of engine needs to be multiplied by count.

(Just note this is the what the current VLA evaluates to :)

> > +}
> > +
> > +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?

I don't mind. Would shave a few lines indeed, but we need the memsets
back. #define alloca0()?

> 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 don't think that is as clean for the long term.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  8:33     ` [igt-dev] [Intel-gfx] " Chris Wilson
@ 2019-05-24  8:39       ` Ser, Simon
  -1 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  8:39 UTC (permalink / raw)
  To: tvrtko.ursulin, intel-gfx, chris; +Cc: igt-dev

On Fri, 2019-05-24 at 09:33 +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
> > 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@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> Why does it seem wrong? That's the calculation used previously, and the
> ptr->engines[] was meant to be packed in order for
> sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
> I threw it in there to check if the calculation was sane.
> 
> > > +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
> > 
> > So size of of engine needs to be multiplied by count.
> 
> (Just note this is the what the current VLA evaluates to :)
> 
> > > +}
> > > +
> > > +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?
> 
> I don't mind. Would shave a few lines indeed, but we need the memsets
> back. #define alloca0()?
> 
> > 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 don't think that is as clean for the long term.

I don't understand the motivation here. Can you elaborate?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  8:39       ` Ser, Simon
  0 siblings, 0 replies; 22+ messages in thread
From: Ser, Simon @ 2019-05-24  8:39 UTC (permalink / raw)
  To: tvrtko.ursulin, intel-gfx, chris; +Cc: igt-dev

On Fri, 2019-05-24 at 09:33 +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
> > 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@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> Why does it seem wrong? That's the calculation used previously, and the
> ptr->engines[] was meant to be packed in order for
> sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
> I threw it in there to check if the calculation was sane.
> 
> > > +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
> > 
> > So size of of engine needs to be multiplied by count.
> 
> (Just note this is the what the current VLA evaluates to :)
> 
> > > +}
> > > +
> > > +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?
> 
> I don't mind. Would shave a few lines indeed, but we need the memsets
> back. #define alloca0()?
> 
> > 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 don't think that is as clean for the long term.

I don't understand the motivation here. Can you elaborate?
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  8:33     ` [igt-dev] [Intel-gfx] " Chris Wilson
@ 2019-05-24  8:44       ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  8:44 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 24/05/2019 09:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
>>
>> 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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> Why does it seem wrong? That's the calculation used previously, and the
> ptr->engines[] was meant to be packed in order for
> sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
> I threw it in there to check if the calculation was sane.

Because sizeof(ptr->engines[0]) == sizeof(ptr->engines[N]), since the 
code is not declaring N big array, just referencing the element N. So 
for more than one engine I expect it explodes. Unless I am way wrong.. I 
guess someone needs to run it.. :)

>>> +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
>>
>> So size of of engine needs to be multiplied by count.
> 
> (Just note this is the what the current VLA evaluates to :)
> 
>>> +}
>>> +
>>> +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?
> 
> I don't mind. Would shave a few lines indeed, but we need the memsets
> back. #define alloca0()?

And a helper macro to generically deal with struct header + engines 
array so it doesn't need to be repeated three times. Yadayada too much 
work.. :) ...

>> 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 don't think that is as clean for the long term.

... this should be just fine for now so I'd vote for it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs
@ 2019-05-24  8:44       ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  8:44 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 24/05/2019 09:33, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-24 09:20:47)
>>
>> 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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> Why does it seem wrong? That's the calculation used previously, and the
> ptr->engines[] was meant to be packed in order for
> sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0]). Anyway,
> I threw it in there to check if the calculation was sane.

Because sizeof(ptr->engines[0]) == sizeof(ptr->engines[N]), since the 
code is not declaring N big array, just referencing the element N. So 
for more than one engine I expect it explodes. Unless I am way wrong.. I 
guess someone needs to run it.. :)

>>> +     return sizeof(*ptr) + sizeof(ptr->engines[count]);
>>
>> So size of of engine needs to be multiplied by count.
> 
> (Just note this is the what the current VLA evaluates to :)
> 
>>> +}
>>> +
>>> +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?
> 
> I don't mind. Would shave a few lines indeed, but we need the memsets
> back. #define alloca0()?

And a helper macro to generically deal with struct header + engines 
array so it doesn't need to be repeated three times. Yadayada too much 
work.. :) ...

>> 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 don't think that is as clean for the long term.

... this should be just fine for now so I'd vote for it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
@ 2019-05-24  8:45   ` Chris Wilson
  -1 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  8:45 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev

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. So calculate the size by hand!

v2: Use alloca().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 115 +++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 51 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index e2ffb93a9..db19925b1 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -1441,6 +1441,29 @@ 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;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+static size_t sizeof_param_engines(int count)
+{
+	struct i915_context_param_engines *ptr;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+static size_t sizeof_engines_bond(int count)
+{
+	struct i915_context_engines_bond *ptr;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+#define alloca0(sz) ({ size_t sz__ = (sz); memset(alloca(sz__), 0, sz__); })
+
 static int
 prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
 {
@@ -1676,66 +1699,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 =
+				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
+			struct i915_context_engines_load_balance *load_balance =
+				alloca0(sizeof_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 =
+					alloca0(sizeof_engines_bond(__builtin_popcount(mask)));
 				unsigned int b, e;
 
-				prev = p;
-				p = alloca(sizeof(bond));
-				assert(p);
-				memset(p, 0, sizeof(bond));
+				bond->base.next_extension = to_user_pointer(last);
+				bond->base.name = I915_CONTEXT_ENGINES_EXT_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);
-
-				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,42 +1754,44 @@ 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, &param);
 		} 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 =
+				alloca0(sizeof_load_balance(count));
+			struct i915_context_param_engines *set_engines =
+				alloca0(sizeof_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, &param);
 		}
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [igt-dev] [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes
@ 2019-05-24  8:45   ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-05-24  8:45 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Tvrtko Ursulin

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. So calculate the size by hand!

v2: Use alloca().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 115 +++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 51 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index e2ffb93a9..db19925b1 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -1441,6 +1441,29 @@ 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;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+static size_t sizeof_param_engines(int count)
+{
+	struct i915_context_param_engines *ptr;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+static size_t sizeof_engines_bond(int count)
+{
+	struct i915_context_engines_bond *ptr;
+
+	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
+}
+
+#define alloca0(sz) ({ size_t sz__ = (sz); memset(alloca(sz__), 0, sz__); })
+
 static int
 prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
 {
@@ -1676,66 +1699,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 =
+				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
+			struct i915_context_engines_load_balance *load_balance =
+				alloca0(sizeof_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 =
+					alloca0(sizeof_engines_bond(__builtin_popcount(mask)));
 				unsigned int b, e;
 
-				prev = p;
-				p = alloca(sizeof(bond));
-				assert(p);
-				memset(p, 0, sizeof(bond));
+				bond->base.next_extension = to_user_pointer(last);
+				bond->base.name = I915_CONTEXT_ENGINES_EXT_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);
-
-				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,42 +1754,44 @@ 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, &param);
 		} 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 =
+				alloca0(sizeof_load_balance(count));
+			struct i915_context_param_engines *set_engines =
+				alloca0(sizeof_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, &param);
 		}
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes
  2019-05-24  8:45   ` [igt-dev] " Chris Wilson
@ 2019-05-24  9:35     ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  9:35 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 24/05/2019 09:45, 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. So calculate the size by hand!
> 
> v2: Use alloca().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   benchmarks/gem_wsim.c | 115 +++++++++++++++++++++++-------------------
>   1 file changed, 64 insertions(+), 51 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index e2ffb93a9..db19925b1 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -1441,6 +1441,29 @@ 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;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +static size_t sizeof_param_engines(int count)
> +{
> +	struct i915_context_param_engines *ptr;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +static size_t sizeof_engines_bond(int count)
> +{
> +	struct i915_context_engines_bond *ptr;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +#define alloca0(sz) ({ size_t sz__ = (sz); memset(alloca(sz__), 0, sz__); })
> +
>   static int
>   prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
>   {
> @@ -1676,66 +1699,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 =
> +				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
> +			struct i915_context_engines_load_balance *load_balance =
> +				alloca0(sizeof_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 =
> +					alloca0(sizeof_engines_bond(__builtin_popcount(mask)));
>   				unsigned int b, e;
>   
> -				prev = p;
> -				p = alloca(sizeof(bond));
> -				assert(p);
> -				memset(p, 0, sizeof(bond));
> +				bond->base.next_extension = to_user_pointer(last);
> +				bond->base.name = I915_CONTEXT_ENGINES_EXT_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);
> -
> -				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,42 +1754,44 @@ 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, &param);
>   		} 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 =
> +				alloca0(sizeof_load_balance(count));
> +			struct i915_context_param_engines *set_engines =
> +				alloca0(sizeof_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, &param);
>   		}
> 

Seems correct.

You couldn't resist throwing in the reverse bond list building 
simplification as well. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [Intel-gfx] [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes
@ 2019-05-24  9:35     ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2019-05-24  9:35 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 24/05/2019 09:45, 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. So calculate the size by hand!
> 
> v2: Use alloca().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   benchmarks/gem_wsim.c | 115 +++++++++++++++++++++++-------------------
>   1 file changed, 64 insertions(+), 51 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index e2ffb93a9..db19925b1 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -1441,6 +1441,29 @@ 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;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +static size_t sizeof_param_engines(int count)
> +{
> +	struct i915_context_param_engines *ptr;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +static size_t sizeof_engines_bond(int count)
> +{
> +	struct i915_context_engines_bond *ptr;
> +
> +	return sizeof(*ptr) + count * sizeof(ptr->engines[0]);
> +}
> +
> +#define alloca0(sz) ({ size_t sz__ = (sz); memset(alloca(sz__), 0, sz__); })
> +
>   static int
>   prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
>   {
> @@ -1676,66 +1699,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 =
> +				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
> +			struct i915_context_engines_load_balance *load_balance =
> +				alloca0(sizeof_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 =
> +					alloca0(sizeof_engines_bond(__builtin_popcount(mask)));
>   				unsigned int b, e;
>   
> -				prev = p;
> -				p = alloca(sizeof(bond));
> -				assert(p);
> -				memset(p, 0, sizeof(bond));
> +				bond->base.next_extension = to_user_pointer(last);
> +				bond->base.name = I915_CONTEXT_ENGINES_EXT_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);
> -
> -				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,42 +1754,44 @@ 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, &param);
>   		} 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 =
> +				alloca0(sizeof_load_balance(count));
> +			struct i915_context_param_engines *set_engines =
> +				alloca0(sizeof_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, &param);
>   		}
> 

Seems correct.

You couldn't resist throwing in the reverse bond list building 
simplification as well. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2)
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
                   ` (4 preceding siblings ...)
  (?)
@ 2019-05-24 11:07 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-05-24 11:07 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: benchmarks/gem_wsim: Heap allocate VLA structs (rev2)
URL   : https://patchwork.freedesktop.org/series/61085/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6140 -> IGTPW_3053
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/61085/revisions/2/mbox/

Known issues
------------

  Here are the changes found in IGTPW_3053 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       [PASS][1] -> [DMESG-WARN][2] ([fdo#108965])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-glk-dsi:         [PASS][3] -> [INCOMPLETE][4] ([fdo#103359] / [k.org#198133])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-glk-dsi/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-glk-dsi/igt@gem_exec_suspend@basic-s3.html

  
#### Possible fixes ####

  * igt@gem_ctx_param@basic:
    - {fi-icl-u3}:        [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-icl-u3/igt@gem_ctx_param@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-icl-u3/igt@gem_ctx_param@basic.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [INCOMPLETE][7] ([fdo#107718]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_selftest@live_contexts:
    - {fi-icl-dsi}:       [INCOMPLETE][9] ([fdo#107713] / [fdo#108569]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-icl-dsi/igt@i915_selftest@live_contexts.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-icl-dsi/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [INCOMPLETE][11] ([fdo#108602] / [fdo#108744]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html

  * igt@i915_selftest@live_requests:
    - fi-bsw-n3050:       [INCOMPLETE][13] ([fdo#105876]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/fi-bsw-n3050/igt@i915_selftest@live_requests.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/fi-bsw-n3050/igt@i915_selftest@live_requests.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (52 -> 46)
------------------------------

  Additional (1): fi-apl-guc 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_5012 -> IGTPW_3053

  CI_DRM_6140: 0dc04d35a90b3a884e00a31f0c843c433cb5540f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3053: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/
  IGT_5012: 996b4346b6a7f2bd0919817648a4f7a382e59757 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
                   ` (5 preceding siblings ...)
  (?)
@ 2019-05-25 13:11 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-05-25 13:11 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: benchmarks/gem_wsim: Heap allocate VLA structs
URL   : https://patchwork.freedesktop.org/series/61085/
State : success

== Summary ==

CI Bug Log - changes from IGT_5012_full -> IGTPW_3051_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/61085/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_3051_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +5 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl7/igt@gem_workarounds@suspend-resume-context.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl6/igt@gem_workarounds@suspend-resume-context.html

  * igt@i915_pm_rps@min-max-config-idle:
    - shard-kbl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103665])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl4/igt@i915_pm_rps@min-max-config-idle.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl7/igt@i915_pm_rps@min-max-config-idle.html

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-glk:          [PASS][5] -> [INCOMPLETE][6] ([fdo#103359] / [k.org#198133])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-glk2/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-glk1/igt@kms_rotation_crc@multiplane-rotation-cropping-bottom.html

  * igt@kms_rotation_crc@primary-rotation-180:
    - shard-kbl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#103558] / [fdo#105602]) +26 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl6/igt@kms_rotation_crc@primary-rotation-180.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl3/igt@kms_rotation_crc@primary-rotation-180.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][9] -> [FAIL][10] ([fdo#99912])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl1/igt@kms_setmode@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl1/igt@kms_setmode@basic.html

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([fdo#110037])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl3/igt@kms_universal_plane@universal-plane-pipe-a-functional.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl7/igt@kms_universal_plane@universal-plane-pipe-a-functional.html

  * igt@perf_pmu@rc6-runtime-pm-long:
    - shard-apl:          [PASS][13] -> [FAIL][14] ([fdo#105010])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl1/igt@perf_pmu@rc6-runtime-pm-long.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl4/igt@perf_pmu@rc6-runtime-pm-long.html
    - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#105010])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-glk9/igt@perf_pmu@rc6-runtime-pm-long.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-glk3/igt@perf_pmu@rc6-runtime-pm-long.html
    - shard-kbl:          [PASS][17] -> [FAIL][18] ([fdo#105010])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl3/igt@perf_pmu@rc6-runtime-pm-long.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl7/igt@perf_pmu@rc6-runtime-pm-long.html

  
#### Possible fixes ####

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-kbl:          [SKIP][19] ([fdo#109271]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl6/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl6/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][21] ([fdo#108566]) -> [PASS][22] +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl5/igt@i915_suspend@fence-restore-tiled2untiled.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl2/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque:
    - shard-kbl:          [FAIL][23] ([fdo#103232]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html
    - shard-apl:          [FAIL][25] ([fdo#103232]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl3/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl5/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html

  * igt@kms_flip@modeset-vs-vblank-race-interruptible:
    - shard-apl:          [FAIL][27] ([fdo#103060]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl7/igt@kms_flip@modeset-vs-vblank-race-interruptible.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl8/igt@kms_flip@modeset-vs-vblank-race-interruptible.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][29] ([fdo#99912]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-apl3/igt@kms_setmode@basic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-apl4/igt@kms_setmode@basic.html

  
#### Warnings ####

  * igt@kms_atomic_transition@2x-modeset-transitions-nonblocking:
    - shard-kbl:          [SKIP][31] ([fdo#109271]) -> [SKIP][32] ([fdo#105602] / [fdo#109271]) +12 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl7/igt@kms_atomic_transition@2x-modeset-transitions-nonblocking.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl3/igt@kms_atomic_transition@2x-modeset-transitions-nonblocking.html

  * igt@kms_concurrent@pipe-d:
    - shard-kbl:          [SKIP][33] ([fdo#109271] / [fdo#109278]) -> [SKIP][34] ([fdo#105602] / [fdo#109271] / [fdo#109278]) +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl1/igt@kms_concurrent@pipe-d.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl3/igt@kms_concurrent@pipe-d.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-kbl:          [FAIL][35] ([fdo#108145]) -> [DMESG-FAIL][36] ([fdo#103558] / [fdo#105602] / [fdo#108145])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5012/shard-kbl7/igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/shard-kbl3/igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110037]: https://bugs.freedesktop.org/show_bug.cgi?id=110037
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (6 -> 4)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

  * IGT: IGT_5012 -> IGTPW_3051

  CI_DRM_6138: a17cee540f7ffd493c7944dea5ea6e2d32fdfdb4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3051: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/
  IGT_5012: 996b4346b6a7f2bd0919817648a4f7a382e59757 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3051/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2)
  2019-05-24  7:25 ` [igt-dev] " Chris Wilson
                   ` (6 preceding siblings ...)
  (?)
@ 2019-05-25 16:10 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-05-25 16:10 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: benchmarks/gem_wsim: Heap allocate VLA structs (rev2)
URL   : https://patchwork.freedesktop.org/series/61085/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6140_full -> IGTPW_3053_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with IGTPW_3053_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_3053_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/61085/revisions/2/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_3053_full:

### IGT changes ###

#### Warnings ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          [INCOMPLETE][1] ([fdo#103540]) -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw8/igt@gem_tiled_swapping@non-threaded.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw7/igt@gem_tiled_swapping@non-threaded.html

  * igt@prime_vgem@busy-bsd1:
    - shard-snb:          [INCOMPLETE][3] ([fdo#105411]) -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-snb4/igt@prime_vgem@busy-bsd1.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-snb2/igt@prime_vgem@busy-bsd1.html

  
Known issues
------------

  Here are the changes found in IGTPW_3053_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         [PASS][5] -> [FAIL][6] ([fdo#108686])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb7/igt@gem_tiled_swapping@non-threaded.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb6/igt@gem_tiled_swapping@non-threaded.html

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +7 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-apl4/igt@gem_workarounds@suspend-resume.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-apl3/igt@gem_workarounds@suspend-resume.html

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          [PASS][9] -> [SKIP][10] ([fdo#109271])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-snb4/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-snb1/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@kms_color@pipe-b-legacy-gamma:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([fdo#104782])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-glk2/igt@kms_color@pipe-b-legacy-gamma.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-glk3/igt@kms_color@pipe-b-legacy-gamma.html
    - shard-apl:          [PASS][13] -> [FAIL][14] ([fdo#104782])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-apl6/igt@kms_color@pipe-b-legacy-gamma.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-apl1/igt@kms_color@pipe-b-legacy-gamma.html
    - shard-kbl:          [PASS][15] -> [FAIL][16] ([fdo#104782])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-kbl7/igt@kms_color@pipe-b-legacy-gamma.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-kbl4/igt@kms_color@pipe-b-legacy-gamma.html

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
    - shard-hsw:          [PASS][17] -> [FAIL][18] ([fdo#105767])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw7/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw6/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-hsw:          [PASS][19] -> [SKIP][20] ([fdo#109271]) +12 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw7/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw1/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html

  * igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions:
    - shard-hsw:          [PASS][21] -> [FAIL][22] ([fdo#103355])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw2/igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109349])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb1/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-hsw:          [PASS][25] -> [INCOMPLETE][26] ([fdo#103540]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw7/igt@kms_flip@flip-vs-suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw2/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [PASS][27] -> [FAIL][28] ([fdo#103167]) +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
    - shard-kbl:          [PASS][29] -> [FAIL][30] ([fdo#103167])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite:
    - shard-glk:          [PASS][31] -> [FAIL][32] ([fdo#103167])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-glk5/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-glk1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][33] -> [FAIL][34] ([fdo#108341])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb6/igt@kms_psr@no_drrs.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [PASS][35] -> [SKIP][36] ([fdo#109441]) +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb2/igt@kms_psr@psr2_basic.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb7/igt@kms_psr@psr2_basic.html

  
#### Possible fixes ####

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][37] ([fdo#108566]) -> [PASS][38] +2 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-apl2/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque:
    - shard-kbl:          [FAIL][39] ([fdo#103232]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-alpha-opaque.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x21-sliding:
    - shard-apl:          [FAIL][41] ([fdo#103232]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-apl5/igt@kms_cursor_crc@pipe-b-cursor-64x21-sliding.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-64x21-sliding.html

  * igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          [FAIL][43] ([fdo#106509] / [fdo#107409]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-glk6/igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-glk1/igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic.html

  * igt@kms_flip@2x-plain-flip:
    - shard-hsw:          [SKIP][45] ([fdo#109271]) -> [PASS][46] +27 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-hsw1/igt@kms_flip@2x-plain-flip.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-hsw4/igt@kms_flip@2x-plain-flip.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [FAIL][47] ([fdo#103167]) -> [PASS][48] +5 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-glk:          [INCOMPLETE][49] ([fdo#103359] / [k.org#198133]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-glk3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-glk3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][51] ([fdo#103166]) -> [PASS][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [SKIP][53] ([fdo#109441]) -> [PASS][54] +2 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb6/igt@kms_psr@psr2_cursor_plane_onoff.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-odd:
    - shard-iclb:         [TIMEOUT][55] ([fdo#109673]) -> [INCOMPLETE][56] ([fdo#107713] / [fdo#109100])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy-odd.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-iclb1/igt@gem_mmap_gtt@forked-big-copy-odd.html

  * igt@prime_vgem@wait-bsd1:
    - shard-snb:          [FAIL][57] -> [INCOMPLETE][58] ([fdo#105411])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6140/shard-snb1/igt@prime_vgem@wait-bsd1.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/shard-snb1/igt@prime_vgem@wait-bsd1.html

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 6)
------------------------------

  Missing    (4): pig-skl-6260u shard-skl pig-hsw-4770r pig-glk-j5005 


Build changes
-------------

  * IGT: IGT_5012 -> IGTPW_3053
  * Piglit: piglit_4509 -> None

  CI_DRM_6140: 0dc04d35a90b3a884e00a31f0c843c433cb5540f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3053: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/
  IGT_5012: 996b4346b6a7f2bd0919817648a4f7a382e59757 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3053/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-05-25 16:10 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24  7:25 [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Chris Wilson
2019-05-24  7:25 ` [igt-dev] " Chris Wilson
2019-05-24  7:45 ` Ser, Simon
2019-05-24  7:45   ` Ser, Simon
2019-05-24  7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-05-24  8:20 ` [PATCH i-g-t] " Tvrtko Ursulin
2019-05-24  8:20   ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
2019-05-24  8:27   ` [igt-dev] " Ser, Simon
2019-05-24  8:27     ` [igt-dev] [Intel-gfx] " Ser, Simon
2019-05-24  8:33   ` Chris Wilson
2019-05-24  8:33     ` [igt-dev] [Intel-gfx] " Chris Wilson
2019-05-24  8:39     ` [igt-dev] " Ser, Simon
2019-05-24  8:39       ` [igt-dev] [Intel-gfx] " Ser, Simon
2019-05-24  8:44     ` Tvrtko Ursulin
2019-05-24  8:44       ` [Intel-gfx] " Tvrtko Ursulin
2019-05-24  8:45 ` [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes Chris Wilson
2019-05-24  8:45   ` [igt-dev] " Chris Wilson
2019-05-24  9:35   ` Tvrtko Ursulin
2019-05-24  9:35     ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin
2019-05-24 11:07 ` [igt-dev] ✓ Fi.CI.BAT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork
2019-05-25 13:11 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs Patchwork
2019-05-25 16:10 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.