Re: [PATCH] tests: save GPU engine information more properly

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 13/10/2023 05:20, Lee Shawn C wrote:
> We encounter a unexpected error on chrome book device while
> running kms_busy test. It will restore GPU engine's timeout
> value but open incorrect file name (XR24 in below).
>
> openat(AT_FDCWD, "/sys/dev/char/226:0", O_RDONLY) = 12
> openat(12, "dev", O_RDONLY)             = 13
> read(13, "226:0\n", 1023)               = 6
> close(13)                               = 0
> openat(12, "engine", O_RDONLY)          = 13
> close(12)                               = 0
> openat(13, "XR24", O_RDONLY)            = -1 ENOENT (No such file or directory)
>
> Test code did not save engine data properly to cause this problem.
> So modify the code to save GPU engine information into a globla variable
> to avoid test deamon to open incorrect file again.
>
> Issue: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/147
> Fixes: 9e635a1c5029 ("tests/kms_busy: Ensure GPU reset when waiting
> for a new FB during modeset")

I guess if we are having one Fixes: could add some more for i915_hangman 
and gem_exec_capture but it is not that important for igt so meh.

Changes look good to me. Presumably it fixes things for you and if CI 
ends up green:

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

Regards,

Tvrtko

> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Vidya Srinivas <vidya.srinivas@xxxxxxxxx>
> Signed-off-by: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
> ---
>   lib/i915/gem_engine_topology.c | 12 ++++++------
>   lib/i915/gem_engine_topology.h |  2 +-
>   tests/intel/gem_exec_capture.c |  2 +-
>   tests/intel/gem_reset_stats.c  |  2 +-
>   tests/intel/i915_hangman.c     |  2 +-
>   tests/intel/kms_busy.c         |  2 +-
>   6 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index 7c6cd9ba97db..afb576afb2dc 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -571,23 +571,23 @@ void gem_engine_properties_configure(int fd, struct gem_engine_properties *param
>   	int ret;
>   	struct gem_engine_properties write = *params;
>   
> -	ret = gem_engine_property_scanf(fd, write.engine->name,
> +	ret = gem_engine_property_scanf(fd, write.engine.name,
>   					"heartbeat_interval_ms",
>   					"%d", &params->heartbeat_interval);
>   	igt_assert_eq(ret, 1);
>   
> -	ret = gem_engine_property_printf(fd, write.engine->name,
> +	ret = gem_engine_property_printf(fd, write.engine.name,
>   					 "heartbeat_interval_ms", "%d",
>   					 write.heartbeat_interval);
>   	igt_assert_lt(0, ret);
>   
>   	if (gem_scheduler_has_preemption(fd)) {
> -		ret = gem_engine_property_scanf(fd, write.engine->name,
> +		ret = gem_engine_property_scanf(fd, write.engine.name,
>   						"preempt_timeout_ms",
>   						"%d", &params->preempt_timeout);
>   		igt_assert_eq(ret, 1);
>   
> -		ret = gem_engine_property_printf(fd, write.engine->name,
> +		ret = gem_engine_property_printf(fd, write.engine.name,
>   						 "preempt_timeout_ms", "%d",
>   						 write.preempt_timeout);
>   		igt_assert_lt(0, ret);
> @@ -598,13 +598,13 @@ void gem_engine_properties_restore(int fd, const struct gem_engine_properties *s
>   {
>   	int ret;
>   
> -	ret = gem_engine_property_printf(fd, saved->engine->name,
> +	ret = gem_engine_property_printf(fd, saved->engine.name,
>   					 "heartbeat_interval_ms", "%d",
>   					 saved->heartbeat_interval);
>   	igt_assert_lt(0, ret);
>   
>   	if (gem_scheduler_has_preemption(fd)) {
> -		ret = gem_engine_property_printf(fd, saved->engine->name,
> +		ret = gem_engine_property_printf(fd, saved->engine.name,
>   						 "preempt_timeout_ms", "%d",
>   						 saved->preempt_timeout);
>   		igt_assert_lt(0, ret);
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index 89642c31721c..9eb9d5ddc888 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -122,7 +122,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
>   	     intel_next_engine(&i__##e__))
>   
>   struct gem_engine_properties {
> -	const struct intel_execution_engine2 *engine;
> +	struct intel_execution_engine2 engine;
>   	int preempt_timeout;
>   	int heartbeat_interval;
>   };
> diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> index ab8305e4dfaf..d231c53b98e1 100644
> --- a/tests/intel/gem_exec_capture.c
> +++ b/tests/intel/gem_exec_capture.c
> @@ -258,7 +258,7 @@ configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
>   	struct gem_engine_properties props;
>   
>   	/* Ensure fast hang detection */
> -	props.engine = e;
> +	props.engine = *e;
>   	props.preempt_timeout = 250;
>   	props.heartbeat_interval = 500;
>   	gem_engine_properties_configure(fd, &props);
> diff --git a/tests/intel/gem_reset_stats.c b/tests/intel/gem_reset_stats.c
> index 6939722c7001..5dfc1fc677ec 100644
> --- a/tests/intel/gem_reset_stats.c
> +++ b/tests/intel/gem_reset_stats.c
> @@ -1004,7 +1004,7 @@ static void test_shared_reset_domain(const intel_ctx_cfg_t *base_cfg,
>   
>   	sync_gpu();
>   
> -	params.engine = e;
> +	params.engine = *e;
>   	params.preempt_timeout = 1;
>   	params.heartbeat_interval = 250;
>   	gem_engine_properties_configure(device, &params);
> diff --git a/tests/intel/i915_hangman.c b/tests/intel/i915_hangman.c
> index dcd34cd0ebfe..8a7de562609c 100644
> --- a/tests/intel/i915_hangman.c
> +++ b/tests/intel/i915_hangman.c
> @@ -600,7 +600,7 @@ igt_main
>   		gem_require_mmap_device_coherent(device);
>   
>   		for_each_physical_engine(device, e) {
> -			saved_params[num_engines].engine = e;
> +			saved_params[num_engines].engine = *e;
>   			saved_params[num_engines].preempt_timeout = 500;
>   			saved_params[num_engines].heartbeat_interval = 1000;
>   			gem_engine_properties_configure(device, saved_params + num_engines++);
> diff --git a/tests/intel/kms_busy.c b/tests/intel/kms_busy.c
> index 5b620658fb18..50616513c5ba 100644
> --- a/tests/intel/kms_busy.c
> +++ b/tests/intel/kms_busy.c
> @@ -404,7 +404,7 @@ static void gpu_engines_init_timeouts(int fd, int max_engines,
>   	for_each_physical_engine(fd, e) {
>   		igt_assert(*num_engines < max_engines);
>   
> -		props[*num_engines].engine = e;
> +		props[*num_engines].engine = *e;
>   		props[*num_engines].preempt_timeout = 0;
>   		props[*num_engines].heartbeat_interval = 250;
>