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

extension/ext: Add missing length-rounding in cmd_set_sample_mask() assert #895

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

Macdu
Copy link
Contributor

@Macdu Macdu commented Mar 28, 2024

As stated in the vulkan specification for vkCmdSetSampleMaskEXT: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkCmdSetSampleMaskEXT.html

sample_mask must be an array of size ceil(samples/32). However right now the ash wrapper for this function is checking that it is of size floor(samples/32), making this wrapper unusable for any sample count less than 32.

@Macdu
Copy link
Contributor Author

Macdu commented Mar 29, 2024

Ah, div_ceil was only stabilized in Rust 1.73. Do you want me to rewrite this function the classic way ((samples.as_raw() + 31) / 32 as usize), or keep the fix as it is as it looks like you are planning to increase the MSRV anyway?

@MarijnS95
Copy link
Collaborator

That MSRV bump has already happened, to 1.69 which the CI is enforcing. We don't have a strict (written down) MSRV policy other than trying to not bump too eagerly (i.e. at least 6 months) and keeping them to semver-breaking releases.

Having div_ceil over hand-written rounding in two instances isn't enough to warrant an MSRV bump. On the other hand the October 5th release of Rust 1.73 is nearing that same 6-month mark, and could allow us to use more features added in those 4 releases in future patch-releases that we don't yet know about.

@MarijnS95 MarijnS95 changed the title extension/ext: Fix cmd_set_sample_mask check extension/ext: Fix rounding lenght-check in cmd_set_sample_mask Mar 29, 2024
@MarijnS95 MarijnS95 changed the title extension/ext: Fix rounding lenght-check in cmd_set_sample_mask extension/ext: Fix rounding length-check in cmd_set_sample_mask Mar 29, 2024
@Macdu
Copy link
Contributor Author

Macdu commented Mar 29, 2024

I see, I changed the content of this PR so it doesn't use div_ceil anymore.

@MarijnS95 MarijnS95 changed the title extension/ext: Fix rounding length-check in cmd_set_sample_mask extension/ext: Add missing length-rounding in cmd_set_sample_mask Mar 29, 2024
@MarijnS95 MarijnS95 changed the title extension/ext: Add missing length-rounding in cmd_set_sample_mask extension/ext: Add missing length-rounding in cmd_set_sample_mask() Mar 29, 2024
@MarijnS95
Copy link
Collaborator

We can make a separate PR to preemptively bump the MSRV if deemed necessary.

This PR also reminded me of an overwritten setter for VkPipelineMultisampleStateCreateInfo::pSampleMask in:

ash/generator/src/lib.rs

Lines 2014 to 2031 in 71387e9

if name == "pSampleMask" {
return Some(quote! {
/// Sets `p_sample_mask` to `null` if the slice is empty. The mask will
/// be treated as if it has all bits set to `1`.
///
/// See <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkPipelineMultisampleStateCreateInfo.html#_description>
/// for more details.
#[inline]
pub fn sample_mask(mut self, sample_mask: &'a [SampleMask]) -> Self {
self.p_sample_mask = if sample_mask.is_empty() {
core::ptr::null()
} else {
sample_mask.as_ptr()
};
self
}
});
}

This is to treat nullability properly (where Rust actually has a non-NULL pointer for &[] with length 0) and we might use that same override to also set rasterization_samples: vk::SampleCountFlags with similar validation to these commands, but not a requirement.

Also, just to make sure, unlike VkPipelineMultisampleStateCreateInfo, neither of these two cmd_set_sample_mask() functions allow pSampleMask to be NULL to "treat the mask as if all bits were set to 1".

@MarijnS95 MarijnS95 changed the title extension/ext: Add missing length-rounding in cmd_set_sample_mask() extension/ext: Add missing length-rounding in cmd_set_sample_mask() assert Mar 29, 2024
@MarijnS95 MarijnS95 merged commit aee0c61 into ash-rs:master Mar 29, 2024
10 checks passed
@Macdu
Copy link
Contributor Author

Macdu commented Mar 29, 2024

Also, just to make sure, unlike VkPipelineMultisampleStateCreateInfo, neither of these two cmd_set_sample_mask() functions allow pSampleMask to be NULL to "treat the mask as if all bits were set to 1".

Yes, the fact that pSampleMask has slightly different semantics in VkPipelineMultisampleStateCreateInfo compared to vkCmdSetSampleMaskEXT is a bit counter-intuitive (although at least the validation layer should be able to detect easily any misuse).

@Macdu Macdu deleted the fix-sample-mask branch March 29, 2024 10:44
@MarijnS95
Copy link
Collaborator

Yeah, at least the VUID makes this clear on both cases (which is what the above statement is based on).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants