Re: [igt-dev] [PATCH i-g-t] lib: Fix intel_get_current_physical_engine() iterator

[Andi Shyti <andi.shyti@intel.com>]

Hi Chris,

On Wed, May 29, 2019 at 02:24:21PM +0100, Chris Wilson wrote:
> If we run out of engines, intel_get_current_physical_engine() degrades
> into an infinite loop as although it advanced the iterator, it did not
> update its local engine pointer.

The patch looks like it does everything "but" what you say in the
commit log :)

> 
> Reported-by: Petri Latvala <petri.latvala@intel.com>
> Fixes: 17c77e7b0c3c ("lib/i915: add gem_engine_topology library and for_each loop definition")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>  lib/i915/gem_engine_topology.c | 49 +++++-----------------------------
>  lib/i915/gem_engine_topology.h | 36 ++++++++++++++++---------
>  2 files changed, 29 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b9516..17f67786f 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -81,11 +81,10 @@ static void ctx_map_engines(int fd, struct intel_engine_data *ed,
>  			    struct drm_i915_gem_context_param *param)
>  {
>  	struct i915_context_param_engines *engines =
> -			from_user_pointer(param->value);
> +		from_user_pointer(param->value);
>  	int i = 0;
>  
> -	for (typeof(engines->engines[0]) *p =
> -	     &engines->engines[0];
> +	for (struct i915_engine_class_instance *p = &engines->engines[0];
>  	     i < ed->nengines; i++, p++) {
>  		p->engine_class = ed->engines[i].class;
>  		p->engine_instance = ed->engines[i].instance;
> @@ -136,7 +135,7 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
>  {
>  	uint8_t buff[SIZEOF_QUERY] = { };
>  	struct drm_i915_query_engine_info *query_engine =
> -			(struct drm_i915_query_engine_info *) buff;
> +		(struct drm_i915_query_engine_info *)buff;

Until here, nothing is related to the description in the commit
log. Can we put the above in a different patch?

> -struct intel_execution_engine2 *
> -intel_get_current_engine(struct intel_engine_data *ed)
> -{
> -	if (!ed->n)
> -		ed->current_engine = &ed->engines[0];
> -	else if (ed->n >= ed->nengines)
> -		ed->current_engine = NULL;
> -
> -	return ed->current_engine;
> -}
> -
> -void intel_next_engine(struct intel_engine_data *ed)
> -{
> -	if (ed->n + 1 < ed->nengines) {
> -		ed->n++;
> -		ed->current_engine = &ed->engines[ed->n];
> -	} else {
> -		ed->n = ed->nengines;
> -		ed->current_engine = NULL;
> -	}
> -}
> -
> -struct intel_execution_engine2 *
> -intel_get_current_physical_engine(struct intel_engine_data *ed)
> -{
> -	struct intel_execution_engine2 *e;
> -
> -	for (e = intel_get_current_engine(ed);
> -	     e && e->is_virtual;
> -	     intel_next_engine(ed))
> -		;
> -
> -	return e;
> -}
> -

Moving these functions to inline in the header file is unrelated
to the patch topic, right?

>  static int gem_topology_get_param(int fd,
>  				  struct drm_i915_gem_context_param *p)
>  {
> @@ -197,10 +161,9 @@ static int gem_topology_get_param(int fd,
>  		return 0;
>  
>  	/* size will store the engine count */
> -	p->size = (p->size - sizeof(struct i915_context_param_engines)) /
> -		  (offsetof(struct i915_context_param_engines,
> -			    engines[1]) -
> -		  sizeof(struct i915_context_param_engines));
> +	igt_assert(p->size >= sizeof(struct i915_context_param_engines));
> +	p->size -= sizeof(struct i915_context_param_engines);
> +	p->size /= sizeof(struct i915_engine_class_instance);

This is also unrelated.

>  struct intel_engine_data {
> -	uint32_t nengines;
> -	uint32_t n;
> -	struct intel_execution_engine2 *current_engine;

so we don't have anymore current_engine... I had the feeling the
Tvrtko really wanted it :)

> +	uint32_t nengines, cur;

[ some copy paste ]

> -	uint32_t nengines;
> -	uint32_t n;
> +	uint32_t nengines, cur;

mmhhh... why?

> +static inline struct intel_execution_engine2 *
> +intel_get_current_engine(struct intel_engine_data *ed)
> +{
> +	if (ed->cur >= ed->nengines)
> +		return NULL;
> +
> +	return &ed->engines[ed->cur];
> +}
> +
> +static inline struct intel_execution_engine2 *
> +intel_get_current_physical_engine(struct intel_engine_data *ed)
> +{
> +	struct intel_execution_engine2 *e;
> +
> +	for (; (e = intel_get_current_engine(ed)) && e->is_virtual; ed->cur++)
> +		;

The above two lines are the only ones related to the commit
message.  Can we keep this patch smaller? and put cosmetics in a
different patchset?

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