Am 23.05.24 um 17:35 schrieb Li, Yunxiang (Teddy):
[Public]
Here is taking a different lock than the reset_domain->sem. It is a seperate
reset_domain->gpu_sem that is only locked when we will actuall do reset, it is not
taken in the skip_hw_reset path.
Exactly that is what you should *not* do. Please don't add any new lock to the
code. This is already complicated enough.
If you think that adding wrappers for reset lock makes sense then we can
probably do that, bot don't add any lock for hw access.
The two lock protects very different things though. The first case is we need
to block two resets running in parallel,
No, that's not correct. Two parallel resets are prevent by using a
worker queue for the resets.
The reset lock is here exactly to provide external thread the
opportunity to make their operation mutual exclusive with the reset.
this does not only cover GPU reset itself but also any teardown that happens
before GPU reset. The second case is we need to ensure exclusive access to the
GPU between GPU reset and GPU init, concurrent access is fine before GPU is
reset.
If that is true you could in theory lower the locked area of the
existing lock, but adding a new one is strict no-go from my side.
Regards,
Christian.
Theoretically, the second case happens within the first case, so locking the first
case would protect against both. But with the current implementation this is
infeasible, all the generic functions called between
amdgpu_device_lock/unlock_reset_domain would need to be swapped out for special
versions so the reset thread does not dead lock itself. It is much simpler to have a
second, much narrower lock that only covers GPU reset<->GPU init because all
the accesses there are very low level anyway.
Teddy
