[i-g-t PATCH V2] tests/intel/xe_pm: one suspend/resume cycle for all xe engines
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Sep 27 17:31:31 UTC 2024
On Fri, Sep 27, 2024 at 01:38:14PM +0200, Peter Senna Tschudin wrote:
> Changes the behavior from running one suspend/resume cycle for each
> xe engine to running a single suspend and resume cycle for all engines
> considerably reducing the xe_pm run time.
\o/
Thanks a lot for that.
I'm wondering if the thread is not an overkill, but I don't have
cleaner suggestions...
>
> V2: Fix race condition around child_ready and fix subject line
>
> Signed-off-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
> ---
> tests/intel/xe_pm.c | 139 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 110 insertions(+), 29 deletions(-)
>
> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> index eee89428c..066c5ddf4 100644
> --- a/tests/intel/xe_pm.c
> +++ b/tests/intel/xe_pm.c
> @@ -54,6 +54,22 @@ typedef struct {
> uint64_t orig_threshold;
> int fw_handle = -1;
>
> +static pthread_mutex_t suspend_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t suspend_cond = PTHREAD_COND_INITIALIZER;
> +static pthread_mutex_t child_ready_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t child_ready_cond = PTHREAD_COND_INITIALIZER;
> +static bool child_ready = false;
> +
> +typedef struct {
> + device_t device;
> + struct drm_xe_engine_class_instance *eci;
> + int n_exec_queues;
> + int n_execs;
> + enum igt_suspend_state s_state;
> + enum igt_acpi_d_state d_state;
> + unsigned int flags;
> +} test_exec_args;
s/test_exec_args/thread_exec_args
> +
> static void dpms_on_off(device_t device, int mode)
> {
> int i;
> @@ -273,6 +289,7 @@ static void close_fw_handle(int sig)
> * @prefetch: prefetch
> * @unbind-all: unbind-all
> */
keep these comments block near the main test function
But I would keep the test_exec as the name of the main function.
> +
> static void
> test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
then name this function thread_child_exec or something like that.
> int n_exec_queues, int n_execs, enum igt_suspend_state s_state,
> @@ -396,10 +413,16 @@ test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
> igt_assert_eq(data[i].data, 0xc0ffee);
>
> if (i == n_execs / 2 && s_state != NO_SUSPEND) {
> - enum igt_suspend_test test = s_state == SUSPEND_STATE_DISK ?
> - SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> -
> - igt_system_suspend_autoresume(s_state, test);
> + /* Tell the parent that we are ready for a suspend and resume */
> + pthread_mutex_lock(&child_ready_lock);
> + child_ready = true;
> + pthread_cond_signal(&child_ready_cond);
> + pthread_mutex_unlock(&child_ready_lock);
> +
> + /* Wait for the suspend and resume to finish */
> + pthread_mutex_lock(&suspend_lock);
> + pthread_cond_wait(&suspend_cond, &suspend_lock);
> + pthread_mutex_unlock(&suspend_lock);
> }
> }
>
> @@ -440,6 +463,81 @@ NULL));
> active_time);
> igt_assert(in_d3(device, d_state));
> }
> +
> + /* Tell the parent that we are ready. This should run only when the code
> + * is not supposed to suspend.
> + */
> + if (n_execs <= 1 || s_state == NO_SUSPEND) {
> + pthread_mutex_lock(&child_ready_lock);
> + child_ready = true;
> + pthread_cond_signal(&child_ready_cond);
> + pthread_mutex_unlock(&child_ready_lock);
> + }
> +}
> +
> +/* Wrap test_exec() function arguments in a struct for pthread_create */
> +static void*
> +test_exec_wrapper(void *args)
this probably deserves a better name...
> +{
> + test_exec_args *exec_args = (test_exec_args *)args;
> +
> + test_exec(exec_args->device, exec_args->eci, exec_args->n_exec_queues,
> + exec_args->n_execs, exec_args->s_state, exec_args->d_state,
> + exec_args->flags);
> +
> + return NULL;
> +}
> +
> +/* Do one suspend and resume cycle for all xe engines.
> + * - For each xe engine: Create a thread for test_exec
> + * - Pause the thread where it expects to suspend and resume
> + * - Wait for all threads to reach the pause
> + * - Run one suspend and resume cycle
> + * - Wake up all threads
> + * - Wait the threads to complete
looks a correct flow for the system suspend... something strange for the runtime pm,
although d3hot and d3cold works for me here in my DG2...
I mean, during the thread child execution we are checking if the device is in d3...
But with multiple threads executing that, we cannot guarantee that anymore that we are
in d3.... That flow is broken....
I believe it just works because in_d3 also has some sleeps and waits so all
the threads are executing and waiting... but we shouldn't rely on that.
Perhaps we should split the regular suspend and runtime_suspend tests entirely?
trying to encapsulate and reuse the exec functions...
> + */
> +static void
> +threaded_test_exec(
as I told above keep test_exec as the main function name.
> device_t device, struct drm_xe_engine_class_instance *eci,
do not declare the engine instance as argument. You are not receiving this
anymore since the xe_for_each_engine is here below.
> + int n_exec_queues, int n_execs, enum igt_suspend_state s_state,
> + enum igt_acpi_d_state d_state, unsigned int flags)
> +{
> + enum igt_suspend_test test = s_state == SUSPEND_STATE_DISK ? SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> + int active_threads = 0;
> + pthread_t threads[65]; /* MAX_ENGINES + 1 */
> + test_exec_args args;
> +
> + xe_for_each_engine(device.fd_xe, eci) {
> + args.device = device;
> + args.eci = eci;
> + args.n_exec_queues = n_exec_queues;
> + args.n_execs = n_execs;
> + args.s_state = s_state;
> + args.d_state = d_state;
> + args.flags = flags;
> +
> + pthread_create(&threads[active_threads], NULL, test_exec_wrapper, &args);
> + active_threads++;
> +
> + pthread_mutex_lock(&child_ready_lock);
> + while(!child_ready)
> + pthread_cond_wait(&child_ready_cond, &child_ready_lock);
> + child_ready = false;
> + pthread_mutex_unlock(&child_ready_lock);
> + }
> +
> + if (n_execs > 1 && s_state != NO_SUSPEND) {
> + igt_system_suspend_autoresume(s_state, test);
> +
> + sleep(2);
why?
> + pthread_mutex_lock(&suspend_lock);
> + pthread_cond_broadcast(&suspend_cond);
> + pthread_mutex_unlock(&suspend_lock);
> +
> + for (int i = 0; i < active_threads; i++)
> + pthread_join(threads[i], NULL);
> +
> + active_threads = 0;
> + }
> }
>
> /**
> @@ -718,8 +816,7 @@ igt_main
> igt_device_get_pci_slot_name(device.fd_xe, device.pci_slot_name);
>
> /* Always perform initial once-basic exec checking for health */
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 1, 1, NO_SUSPEND, NO_RPM, 0);
> + threaded_test_exec(device, hwe, 1, 1, NO_SUSPEND, NO_RPM, 0);
>
> igt_pm_get_d3cold_allowed(device.pci_slot_name, &d3cold_allowed);
> igt_assert(igt_setup_runtime_pm(device.fd_xe));
> @@ -731,14 +828,11 @@ igt_main
> igt_subtest_f("%s-basic", s->name) {
> enum igt_suspend_test test = s->state == SUSPEND_STATE_DISK ?
> SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> -
> igt_system_suspend_autoresume(s->state, test);
> }
>
> igt_subtest_f("%s-basic-exec", s->name) {
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 1, 2, s->state,
> - NO_RPM, 0);
> + threaded_test_exec(device, hwe, 1, 2, s->state, NO_RPM, 0);
> }
>
> igt_subtest_f("%s-exec-after", s->name) {
> @@ -746,31 +840,23 @@ igt_main
> SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
>
> igt_system_suspend_autoresume(s->state, test);
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 1, 2, NO_SUSPEND,
> - NO_RPM, 0);
> + threaded_test_exec(device, hwe, 1, 2, NO_SUSPEND, NO_RPM, 0);
> }
>
> igt_subtest_f("%s-multiple-execs", s->name) {
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 16, 32, s->state,
> - NO_RPM, 0);
> + threaded_test_exec(device, hwe, 16, 32, s->state, NO_RPM, 0);
> }
>
> for (const struct vm_op *op = vm_op; op->name; op++) {
> igt_subtest_f("%s-vm-bind-%s", s->name, op->name) {
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 16, 32, s->state,
> - NO_RPM, op->flags);
> + threaded_test_exec(device, hwe, 16, 32, s->state, NO_RPM, op->flags);
> }
> }
>
> for (const struct d_state *d = d_states; d->name; d++) {
> igt_subtest_f("%s-%s-basic-exec", s->name, d->name) {
> igt_assert(setup_d3(device, d->state));
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 1, 2, s->state,
> - NO_RPM, 0);
> + threaded_test_exec(device, hwe, 1, 2, s->state, NO_RPM, 0);
> cleanup_d3(device);
> }
> }
> @@ -792,17 +878,13 @@ igt_main
>
> igt_subtest_f("%s-basic-exec", d->name) {
> igt_assert(setup_d3(device, d->state));
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 1, 1,
> - NO_SUSPEND, d->state, 0);
> + threaded_test_exec(device, hwe, 1, 1, NO_SUSPEND, d->state, 0);
> cleanup_d3(device);
> }
>
> igt_subtest_f("%s-multiple-execs", d->name) {
> igt_assert(setup_d3(device, d->state));
> - xe_for_each_engine(device.fd_xe, hwe)
> - test_exec(device, hwe, 16, 32,
> - NO_SUSPEND, d->state, 0);
> + threaded_test_exec(device, hwe, 16, 32, NO_SUSPEND, d->state, 0);
> cleanup_d3(device);
> }
>
> @@ -842,7 +924,6 @@ igt_main
> test_mocs_suspend_resume(device, NO_SUSPEND, d->state);
> cleanup_d3(device);
> }
> -
> }
>
> igt_describe("Validate whether card is limited to d3hot,"
> --
> 2.34.1
>
More information about the igt-dev
mailing list