From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <12b425f9-1b13-5c75-2e74-0c45ec4f46e0@linux.intel.com> Date: Fri, 13 Oct 2023 08:47:30 +0100 MIME-Version: 1.0 Content-Language: en-US To: Lee Shawn C , intel-gfx@lists.freedesktop.org, igt-dev@lists.freedesktop.org References: <20231013042012.409376-1-shawn.c.lee@intel.com> From: Tvrtko Ursulin In-Reply-To: <20231013042012.409376-1-shawn.c.lee@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH] tests: save GPU engine information more properly List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 Regards, Tvrtko > Cc: Tvrtko Ursulin > Cc: Imre Deak > Cc: Vidya Srinivas > Signed-off-by: Lee Shawn C > --- > 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", ¶ms->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", ¶ms->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, ¶ms); > 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; >