vkd3d: Leave the command queue op mutex locked after a partial flush.
All return paths in d3d12_command_queue_flush_ops_locked() must leave the op mutex locked.
Merge request reports
Activity
6800 6801 6801 6802 hr = d3d12_command_queue_record_as_blocked(queue); 6802 6803 6803 vkd3d_mutex_unlock(&queue->op_mutex); 6804 6805 6804 return hr; changed this line in version 2 of the diff
added 1 commit
- f34270da - vkd3d: Leave the command queue op mutex locked after a partial flush.
added 1 commit
- e66f0d87 - vkd3d: Leave the command queue op mutex locked after a partial flush.
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 insided3d12_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 ind3d12_command_queue_flush_ops_locked()
, and 2) to acknowledge that in principle there could be other ops thanOP_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 additionalmemmove()
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.
I guess it depends a lot on what you consider fragile or clumsy.
Well, to make it a bit more concrete, at least 3 people originally missed this issue in commit bb2fa97c, and it took me a bit more time than I would've liked to review this MR. Arguably a good chunk of that is down to my shortcomings as a reviewer, but perhaps we could try to cater to those?
Sure, and for the sake of completeness I also take the share of responsibility due to my inability to invalidate assumptions across patch rewritings, but I am not sure of what you would consider better. Does any of the two alternatives I wrote above work better for you? Or would you propose something else?
added 13 commits
-
e66f0d87...8186b752 - 12 commits from branch
wine:master
- 332fe9e7 - vkd3d: Leave the command queue op mutex locked after a partial flush.
-
e66f0d87...8186b752 - 12 commits from branch
added 16 commits
-
332fe9e7...d6d9aab3 - 15 commits from branch
wine:master
- e27ceddf - vkd3d: Leave the command queue op mutex locked after a partial flush.
-
332fe9e7...d6d9aab3 - 15 commits from branch