Skip to content
Snippets Groups Projects

vkd3d: Leave the command queue op mutex locked after a partial flush.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:ops_lock into master
3 unresolved threads

All return paths in d3d12_command_queue_flush_ops_locked() must leave the op mutex locked.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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;
  • Conor McCarthy added 1 commit

    added 1 commit

    • f34270da - vkd3d: Leave the command queue op mutex locked after a partial flush.

    Compare with previous version

  • Conor McCarthy added 1 commit

    added 1 commit

    • e66f0d87 - vkd3d: Leave the command queue op mutex locked after a partial flush.

    Compare with previous version

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

    • Please register or sign in to reply
  • Henri Verbeet approved this merge request

    approved this merge request

    • 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?

    • Please register or sign in to reply
  • Conor McCarthy added 13 commits

    added 13 commits

    • e66f0d87...8186b752 - 12 commits from branch wine:master
    • 332fe9e7 - vkd3d: Leave the command queue op mutex locked after a partial flush.

    Compare with previous version

  • Author Developer

    Is this an improvement?

  • Henri Verbeet unapproved this merge request

    unapproved this merge request

  • Is this an improvement?

    I think that's better, yes.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 16 commits

    added 16 commits

    • 332fe9e7...d6d9aab3 - 15 commits from branch wine:master
    • e27ceddf - vkd3d: Leave the command queue op mutex locked after a partial flush.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading