Skip to content
New issue

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

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

Already on GitHub? # to your account

Added overlapping_push_constant_ranges to PipelineLayout #1820

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

Amjad50
Copy link
Contributor

@Amjad50 Amjad50 commented Feb 9, 2022

Fixes #1819

Added overlapping_push_constant_ranges in PipelineLayout, which helps us track overlapping ranges, and what stages belong into them. It will be used in the push_constants command to help fixing some issues regarding:

  • VUID-vkCmdPushConstants-offset-01795
  • VUID-vkCmdPushConstants-offset-01796
    And also solidify the implementation.

CHANGELOG:

- Added `overlapping_push_constant_ranges` in `PipelineLayout` to keep track of overlapping ranges.
- Added tests for the algorithm on how to compute the `overlapping_push_constant_ranges` in `PipelineLayout`.
- Fixed buffer overflow bug in `AutoCommandBufferBuilder::push_constants`.
- Edited `AutoCommandBufferBuilder::push_constants` to push multiple times in case of range overlap (to accommodate VUIDs 01795 and 01796) 

This helps us track overlapping ranges, and what stages belong into
them.

This will be used in the `push_constants` command to help fixing some
issues regarding:
- VUID-vkCmdPushConstants-offset-01795
- VUID-vkCmdPushConstants-offset-01796
And also solidify the implementation.
@Rua
Copy link
Contributor

Rua commented Feb 9, 2022

It looks good, thank you. I think the name of the function is a bit of a misnomer though. I would expect the ranges that it returns to not overlap. So maybe push_constant_ranges_non_overlapping (I prefer the name in this order, but it's fine either way).

After this edit, `push_constants` will go through all
`overlapping_push_constant_ranges` from `PipelineLayout` and pushes the
overlapping subset of the data with the correct stages flags to satisfy
VUID-vkCmdPushConstants-offset-01795 and VUID-vkCmdPushConstants-offset-01796.

This commit also fixes buffer overflow bug when using offset that is not
zero.
@Amjad50 Amjad50 force-pushed the push_constants_fixes branch from dc3c332 to b6bfb68 Compare February 9, 2022 11:22
@Amjad50 Amjad50 marked this pull request as ready for review February 9, 2022 11:32
@Rua
Copy link
Contributor

Rua commented Feb 9, 2022

Looks good and it runs without errors. Thank you!

@Rua Rua merged commit d2f9f0c into vulkano-rs:master Feb 9, 2022
Rua added a commit that referenced this pull request Feb 9, 2022
@Amjad50 Amjad50 deleted the push_constants_fixes branch February 9, 2022 12:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsound push_constants command implementation
2 participants