On Tue Apr 4 12:56:22 2023 +0000, Henri Verbeet wrote:
The patch looks correct, but the locking here also seems fragile and clumsy. Can we do better?
I guess it depends a lot on what you consider fragile or clumsy. In principle `d3d12_command_queue_fixup_and_lock_after_flush()` could be inlined inside `d3d12_command_queue_flush_ops_locked()`, which would "rebalance" the locking and unlocking operations. The reason why I avoided that are because 1) for code readability, in order to avoid overloading the switch statement in `d3d12_command_queue_flush_ops_locked()`, and 2) to acknowledge that in principle there could be other ops than `OP_WAIT` that can block, and in that case it would be nice to factor the queue fixup together. I personally like the way the code is structured and I don't feel too hard about imbalanced lock/unlock operations, so the current organization suits me, but I will admit none of my reasons is currently too compelling.
Another option would be to move the locking outside of `d3d12_command_queue_fixup_and_lock_after_flush()`, which would mean that an additional `memmove()` would (uselessly) happen inside of the critical region. I doubt in practice much will change, but I'm not a fan of that.
All in all, I don't currently see options what I would consider better, but that of course also depends on my tastes.