Re: [igt-dev] [PATCH i-g-t] lib/i915/gem_engine_topology.c - intel_get_current_engine invalid result

[Antonio Argenziano <antonio.argenziano@intel.com>]

On 13/02/20 11:26, Dale B Stimson wrote:
> Function intel_get_current_engine() should return NULL (instead of
> engine 0) if there are no engines.
> 
> Function intel_init_engine_list() should not store potential engine
> data in the output structure unless the engine is present.
> 
> Function intel_init_engine_list() should arguably not filter the static
> engine list with gem_has_ring if fd == -1, so that subtests can still
> be individually invoked to show subtest FAIL instead of test notrun.
> 
> Symptom: A device open failure in gem_ctx_isolation resulted in
> an endless __for_each_physical_engine "per-engine" loop with the
> purported last potential engine being processed every time.
> 
> Diagnosis: device open (or debugfs open) failed, leaving fd == -1.
> Control skipped the rest of the initial igt_fixture block, after
> which an attempt was made to iterate through engines using macro
> __for_each_physical_engine.
> 
> Macro __for_each_physical_engine called intel_init_engine_list()
> to initialize the loop control data.  Because fd == -1,
> intel_init_engine_list() fell back to using __for_each_static_engine().
> All of the engines in the static engine list are rejected due to
> gem_has_ring returning false (because of fd == -1), leaving 0 engines.
> That resulted in loop control data with engine_data.nengines == 0
> and the data for the last engine considered stored at index 0.
> 
> Still in macro __for_each_physical_engine, intel_get_current_engine()
> was called to get the engine to process.  It should have returned NULL,
> but instead returned the engine entry at index 0, which
> had received information describing the last potential engine.
> This happened without end.
> 
> Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
> ---
>   lib/i915/gem_engine_topology.c | 29 ++++++++++++++++-------------
>   1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index 9daa03df4..b8ed49bc9 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -156,10 +156,10 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
>   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)
> +	if (ed->n >= ed->nengines)
>   		ed->current_engine = NULL;
> +	else if (!ed->n)
> +		ed->current_engine = &ed->engines[0];
>   
>   	return ed->current_engine;
>   }
> @@ -222,18 +222,21 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
>   		igt_debug("using pre-allocated engine list\n");
>   
>   		__for_each_static_engine(e2) {
> -			struct intel_execution_engine2 *__e2 =
> -				&engine_data.engines[engine_data.nengines];
> -
> -			strcpy(__e2->name, e2->name);
> -			__e2->instance   = e2->instance;
> -			__e2->class      = e2->class;
> -			__e2->flags      = e2->flags;
> -			__e2->is_virtual = false;
> -
>   			if (igt_only_list_subtests() ||
> -			    gem_has_ring(fd, e2->flags))
> +			    (fd < 0) ||

Patch LGTM, Chris do you have any issues merging this before someone 
implements some tests for the infrastructure?

Acked-by: Antonio Argenziano <antonio.argenziano@intel.com>

> +			    gem_has_ring(fd, e2->flags)) {
> +				struct intel_execution_engine2 *__e2 =
> +					&engine_data.engines[
> +					engine_data.nengines];
> +
> +				strcpy(__e2->name, e2->name);
> +				__e2->instance   = e2->instance;
> +				__e2->class      = e2->class;
> +				__e2->flags      = e2->flags;
> +				__e2->is_virtual = false;
> +
>   				engine_data.nengines++;
> +                        }
>   		}
>   		return engine_data;
>   	}
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev