Re: [i-g-t PATCH V2] tests/intel/xe_pm: one suspend/resume cycle for all xe engines

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

Hi Rodrigo,

Thank you very much for the review! I addressed most of your comments in V3, and I will skip the comments I fixed in V3.


On 27.09.2024 19:31, Rodrigo Vivi wrote:
> 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...

[...]

>> +}
>> +
>> +/* 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
Updated to state which code paths run concurrently:

/* Do one suspend and resume cycle for all xe engines.
 *  - Create a child_exec() thread for each xe engine. Run only one thread
 *    at a time. The parent will wait for the child to signal it is ready
 *    to sleep before creating a new thread.
 *  - Put child_exec() to sleep where it expects to suspend and resume
 *  - Wait for all child_exec() threads to sleep
 *  - Run one suspend and resume cycle
 *  - Wake up all child_exec() threads at once. They will run concurrently.
 *  - Wait for all child_exec() 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....

Yep, one thread per engine execute that, but only one at a time. There is no concurrency between threads until all threads go to sleep. Does it make a difference?

> 
> 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...

I will be happy to make the change with a little bit of guidance.


[...]


Thank you again Rodrigo!