Re: [PATCH V3 i-g-t] tests/xe_exec_threads: Make hang tests reset domain aware

[John Harrison <john.c.harrison@intel.com>]

On 4/10/2024 02:40, Tejas Upadhyay wrote:
> RCS/CCS are dependent engines as they are sharing reset
> domain. Whenever there is reset from CCS, all the exec queues
> running on RCS are victimised mainly on Lunarlake.
>
> Lets skip parallel execution on CCS with RCS.
But why? In a real-world scenario, this should not happen.

As per earlier comments, before changing the test we need to understand 
more about what is actually happening and why. The previous descriptions 
of what is failing and on what platforms do not make sense.

Also, the request for more explanation was not just for the email 
discussion but to actually add that description into the test. I even 
explicitly asked for that in one of the review emails. The 
xe_exec_threads executable reports 114 separate subtests, none of which 
has any documentation at all. The comments within the code basically 
come down to saying 'wait for completion' immediately before waiting.

At one point Matthew Brost stated:
 > If the HANG flag is set, 1 of the exec queue per thread will insert a
 > non-preemptable spinner. It is expected the GuC will reset this exec
 > queue only. Cross CCS / RCS resets will break this as one of the
 > 'good' exec queues from another thread could also be reset.

To which my replay was that if only one spinner is non-pre-emptible then 
all the other spinners should get automatically switched out prior to 
the reset being triggered and should therefore not be killed. The 'good' 
queues should survive and be totally oblivious to the reset occurring.

So either the above description is inaccurate and all the spinners are 
actually non-pre-emptible. In which case, if that is not the intention 
of the test then the correct fix is to make them pre-emptible and it 
should then pass. Or if the description is accurate, then the test is 
correctly failing because it has found a bug which needs to be fixed. 
Either way, the correct solution is not to simply skip the failing case 
and hide it under the rug.

John.


>
> It helps in fixing following errors:
> 1. Test assertion failure function test_legacy_mode, file, Failed assertion: data[i].data == 0xc0ffee
>
> 2.Test assertion failure function xe_exec, file ../lib/xe/xe_ioctl.c, Failed assertion: __xe_exec(fd, exec) == 0, error: -125 != 0
>
> V3:
>    - Check victimization reset domain wise irrespective of platform - Lucas/MattR
> V2:
>    - Add error details
>
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> ---
>   tests/intel/xe_exec_threads.c | 43 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
> index 8083980f9..24eac39de 100644
> --- a/tests/intel/xe_exec_threads.c
> +++ b/tests/intel/xe_exec_threads.c
> @@ -710,6 +710,24 @@ static void *thread(void *data)
>   	return NULL;
>   }
>   
> +static bool is_engine_contexts_victimized(__u16 eclass, unsigned int flags,
> +					  bool has_rcs, bool multi_ccs,
> +					  bool *ccs0_created)
> +{
> +	if (!(eclass == DRM_XE_ENGINE_CLASS_COMPUTE && flags & HANG))
> +		return false;
> +	if (has_rcs) {
> +		return true;
> +	} else if (multi_ccs) {
> +		/* In case of multi ccs, allow only 1 ccs to run HANG test*/
> +		if (*ccs0_created)
> +			return true;
> +		*ccs0_created = true;
> +	}
> +
> +	return false;
> +}
> +
>   /**
>    * SUBTEST: threads-%s
>    * Description: Run threads %arg[1] test with multi threads
> @@ -955,9 +973,20 @@ static void threads(int fd, int flags)
>   	bool go = false;
>   	int n_threads = 0;
>   	int gt;
> +	bool has_rcs = false;
> +	bool multi_ccs = false, has_ccs = false, ccs0_created = false;
>   
> -	xe_for_each_engine(fd, hwe)
> +	xe_for_each_engine(fd, hwe) {
> +		if (hwe->engine_class == DRM_XE_ENGINE_CLASS_RENDER) {
> +			has_rcs = true;
> +		} else if (hwe->engine_class == DRM_XE_ENGINE_CLASS_COMPUTE) {
> +			if (has_ccs)
> +				multi_ccs = true;
> +			else
> +				has_ccs = true;
> +		}
>   		++n_engines;
> +	}
>   
>   	if (flags & BALANCER) {
>   		xe_for_each_gt(fd, gt)
> @@ -990,6 +1019,18 @@ static void threads(int fd, int flags)
>   	}
>   
>   	xe_for_each_engine(fd, hwe) {
> +		/* RCS/CCS sharing reset domain hence dependent engines.
> +		 * When CCS is doing reset, all the contexts of RCS are
> +		 * victimized. Also in case of multiple CCS instances
> +		 * contexts running on other CCS engine are victimized.
> +		 * so skip the compute engine avoiding parallel execution
> +		 * with RCS and other CCS.
> +		 */
> +		if (is_engine_contexts_victimized(hwe->engine_class, flags,
> +						  has_rcs, multi_ccs,
> +						  &ccs0_created))
> +			continue;
> +
>   		threads_data[i].mutex = &mutex;
>   		threads_data[i].cond = &cond;
>   #define ADDRESS_SHIFT	39