Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Push constants do not update correctly if you don't call vkCmdBindDescriptorSets after vkCmdBindPipeline #657

Closed
hrydgard opened this issue Jul 10, 2019 · 4 comments
Labels
Completed Issue has been fixed, or enhancement implemented.

Comments

@hrydgard
Copy link

hrydgard commented Jul 10, 2019

Broken out from #327.

Push constants in MoltenVK only work correctly for the first pipeline you bind in a command buffer, or after you've called vkCmdBindDescriptorSets after you've bound a later pipeline.

The following call sequence is currently broken in MoltenVK but works on other conforming Vulkan implementations:

vkCmdBindPipeline(pipe1)
vkCmdBindDescriptorSet(pipe1, a uniform buffer in set 0, binding 0)
vkCmdPushConstants(constants1)
vkCmdDraw(draw1)
vkCmdPushConstants(constants2)
vkCmdDraw(draw2)
vkCmdBindPipeline(pipe2)
vkCmdPushConstants(constants3)
vkCmdDraw(draw3)

Note that vkCmdBindDescriptorSets is not called after the second vkCmdBindPipeline.

What happens is that draw3 sees constants2 instead of constants3.

If you do call vkCmdBindDescriptorSets with zero descriptor sets after the second vkCmdBindPipeline (which is UB so validation layers don't like it) it fixes things.

The reason for this behavior appears to be that cmdEncoder->getPushConstants(...)->setMTLBufferIndex is called in vkCmdBindDescriptorSets* instead of vkCmdBindPipeline, where that code should be (push constants are entirely independent of descriptors).

*:

cmdEncoder->getPushConstants(mvkVkShaderStageFlagBitsFromMVKShaderStage(MVKShaderStage(i)))->setMTLBufferIndex(_pushConstantsMTLResourceIndexes.stages[i].bufferIndex);

@kvark
Copy link

kvark commented Jul 10, 2019

It would be good if Vulkan CTS covered more cases in this area

@hrydgard
Copy link
Author

Sure would be good, there are many things missing from the CTS.

@billhollings
Copy link
Contributor

Fixed in PR #660. Please retest and close if resolved.

@billhollings billhollings added the Completed Issue has been fixed, or enhancement implemented. label Jul 10, 2019
@hrydgard
Copy link
Author

I am happy to confirm that the bug seems to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Completed Issue has been fixed, or enhancement implemented.
Projects
None yet
Development

No branches or pull requests

3 participants