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.

Sign up for GitHub

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

extensions/nv/low_latency2: Use count parameter for p_timings "array" member #815

Closed
wants to merge 1 commit into from
Closed

extensions/nv/low_latency2: Use count parameter for p_timings "array" member #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions ash/src/extensions/nv/low_latency2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::RawPtr;
use crate::{Device, Instance};
use std::ffi::CStr;
use std::mem;
use std::ptr;

/// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VK_NV_low_latency2.html>
#[derive(Clone)]
Expand Down Expand Up @@ -53,32 +52,43 @@ impl LowLatency2 {
(self.fp.set_latency_marker_nv)(self.handle, swapchain, latency_marker_info)
}

/// Retrieve the number of elements to pass to [`get_latency_timings()`][Self::get_latency_timings()]
/// Retrieve the number of elements to pass to [`vk::GetLatencyMarkerInfoNV::timings()`] in
/// [`get_latency_timings()`][Self::get_latency_timings()]
#[inline]
pub unsafe fn get_latency_timings_len(&self, swapchain: vk::SwapchainKHR) -> usize {
pub unsafe fn get_latency_timings_len(
&self,
swapchain: vk::SwapchainKHR,
latency_marker_info: &mut vk::GetLatencyMarkerInfoNV<'_>,
) -> usize {
assert!(
latency_marker_info.p_timings.is_null(),
"latency_marker_info.p_timings must be NULL in order to query its length"
);
let mut count = 0;
(self.fp.get_latency_timings_nv)(self.handle, swapchain, &mut count, ptr::null_mut());
(self.fp.get_latency_timings_nv)(self.handle, swapchain, &mut count, latency_marker_info);
count as usize
}

/// <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetLatencyTimingsNV.html>
///
/// Call [`get_latency_timings_len()`][Self::get_latency_timings_len()] to query the number of elements to pass to `latency_marker_info`.
/// Be sure to [`Default::default()`]-initialize these elements and optionally set their `p_next` pointer.
/// Call [`get_latency_timings_len()`][Self::get_latency_timings_len()] to query the number of
/// elements to pass to [`vk::GetLatencyMarkerInfoNV::timings()`].
/// Be sure to [`Default::default()`]-initialize `timing_count` elements and optionally set their `p_next` pointer.
#[inline]
pub unsafe fn get_latency_timings(
&self,
swapchain: vk::SwapchainKHR,
latency_marker_info: &mut [vk::GetLatencyMarkerInfoNV<'_>],
timing_count: usize,
latency_marker_info: &mut vk::GetLatencyMarkerInfoNV<'_>,
) {
let mut count = latency_marker_info.len() as u32;
(self.fp.get_latency_timings_nv)(
self.handle,
swapchain,
&mut count,
latency_marker_info.as_mut_ptr(),
let mut count = timing_count as u32;
assert!(
!latency_marker_info.p_timings.is_null(),
"latency_marker_info.p_timings must be a valid pointer to an array of {} elements",
count
);
assert_eq!(count as usize, latency_marker_info.len());
(self.fp.get_latency_timings_nv)(self.handle, swapchain, &mut count, latency_marker_info);
assert_eq!(timing_count, count as usize);
}

/// <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkQueueNotifyOutOfBandNV.html>
Expand Down
21 changes: 17 additions & 4 deletions ash/src/vk/definitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4319,8 +4319,8 @@ impl<'a> PipelineMultisampleStateCreateInfo<'a> {
self.min_sample_shading = min_sample_shading;
self
}
#[doc = r" Sets `p_sample_mask` to `null` if the slice is empty. The mask will"]
#[doc = r" be treated as if it has all bits set to `1`."]
#[doc = r" Sets [`Self::p_sample_mask`] to [`std::ptr::null()`] if the slice is empty. The"]
#[doc = r" mask will be treated as if it has all bits set to `1`."]
#[doc = r""]
#[doc = r" See <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkPipelineMultisampleStateCreateInfo.html#_description>"]
#[doc = r" for more details."]
Expand Down Expand Up @@ -51890,9 +51890,22 @@ unsafe impl<'a> TaggedStructure for GetLatencyMarkerInfoNV<'a> {
const STRUCTURE_TYPE: StructureType = StructureType::GET_LATENCY_MARKER_INFO_NV;
}
impl<'a> GetLatencyMarkerInfoNV<'a> {
#[doc = r" This only sets the slice pointer. Users must manually pass the length of"]
#[doc = r" `timings` to [`crate::extensions::nv::LowLatency2::get_latency_timings()`]."]
#[doc = r""]
#[doc = r" [`Self::p_timings`] will be set to to [`std::ptr::null_mut()`] if the"]
#[doc = r" slice is empty, which is necessary to query the length of the array via"]
#[doc = r" [`crate::extensions::nv::LowLatency2::get_latency_timings_len()`]."]
#[doc = r""]
#[doc = r" See <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetLatencyTimingsNV.html#_description>"]
#[doc = r" for more details."]
#[inline]
pub fn timings(mut self, timings: &'a mut LatencyTimingsFrameReportNV<'a>) -> Self {
self.p_timings = timings;
pub fn timings(mut self, timings: &'a mut [LatencyTimingsFrameReportNV<'a>]) -> Self {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems all lifetime-fu has escaped me; I can no longer use the slice passed as timings here even after self is dropped. Notably this is the first and only mutable slice that contains objects with a lifetime parameter.

let mut info = vk::GetLatencyMarkerInfoNV::default();
let latency_timings = base
    .low_latency2_loader
    .get_latency_timings_len(base.swapchain, &mut info);
dbg!(latency_timings);
if latency_timings != 0 {
    let mut timings = vec![vk::LatencyTimingsFrameReportNV::default(); latency_timings];
    {
        // Creating a new object just for show
        let mut info = vk::GetLatencyMarkerInfoNV::default().timings(&mut timings);
        base.low_latency2_loader.get_latency_timings(
            base.swapchain,
            latency_timings,
            &mut info,
        );
        // `info` should be dropped here, removing the mutable borrow on `timings`?
    }
    dbg!(&timings); // Error: `timings` is still borrowed in `.timings(&mut timings);` above?
}
error[E0502]: cannot borrow `timings` as immutable because it is also borrowed as mutable
   --> examples/src/bin/texture.rs:800:22
    |
793 |                     let mut info = vk::GetLatencyMarkerInfoNV::default().timings(&mut timings);
    |                                                                                  ------------ mutable borrow occurs here
...
800 |                 dbg!(&timings);
    |                      ^^^^^^^^
    |                      |
    |                      immutable borrow occurs here
    |                      mutable borrow later used here

Copy link
Collaborator

@Ralith Ralith Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by constructing an &'a mut [LatencyTimingsFrameReportNV<'a>], you're saying that the slice is uniquely borrowed for (&'a mut) a lifetime that outlives it (T<'a>). Does it work better if you do fn timings<'b: 'a>(mut self, timings: &'a mut [LatencyTimingsFrameReportNV<'b>]) -> Self?

I think this might also be what you get implicitly if you write LatencyTimingsFrameReportNV<'_>.

Copy link
Collaborator Author

@MarijnS95 MarijnS95 Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ralith that is indeed the first thing I tried, making sense that 'b is a lifetime that is bigger / lives longer than 'a (because LatencyTimingsFrameReportNV and the things that it borrows can live longer than the slice and struct GetLatencyMarkerInfoNV it is eventually stored within / referenced from).

Unfortunately the compiler is not having it:

pub fn timings<'b: 'a>(mut self, timings: &'a mut [LatencyTimingsFrameReportNV<'b>]) -> Self {
    self.p_timings = if timings.is_empty() {
        std::ptr::null_mut()
    } else {
        timings.as_mut_ptr()
    };
    self
}

Results in:

error: lifetime may not live long enough
     --> ash/src/vk/definitions.rs:51910:13
      |
51892 | impl<'a> GetLatencyMarkerInfoNV<'a> {
      |      -- lifetime `'a` defined here
...
51906 |     pub fn timings<'b: 'a>(mut self, timings: &'a mut [LatencyTimingsFrameReportNV<'b>]) -> Self {
      |                    -- lifetime `'b` defined here
...
51910 |             timings.as_mut_ptr()
      |             ^^^^^^^^^^^^^^^^^^^^ argument requires that `'a` must outlive `'b`
      |
      = help: consider adding the following bound: `'a: 'b`
      = note: requirement occurs because of a mutable reference to `[vk::definitions::LatencyTimingsFrameReportNV<'_>]`
      = note: mutable references are invariant over their type parameter
      = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

Writing the bound the other way around results in the same error as before. And it doesn't make much sense for 'a to outlive 'b when 'a is referring to a thing of shorter lifetime 'b?

self.p_timings = if timings.is_empty() {
std::ptr::null_mut()
} else {
timings.as_mut_ptr()
};
self
}
}
Expand Down
30 changes: 28 additions & 2 deletions generator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,8 +1972,8 @@ fn derive_setters(

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`.
/// Sets [`Self::p_sample_mask`] to [`std::ptr::null()`] if the slice is empty. The
/// mask will be treated as if it has all bits set to `1`.
///
/// See <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkPipelineMultisampleStateCreateInfo.html#_description>
/// for more details.
Expand Down Expand Up @@ -2004,6 +2004,32 @@ fn derive_setters(
});
}

// pTimings is a dynamic array, but its length is only available as a mutable-pointer parameter in a calling function:
// https://github.com/KhronosGroup/Vulkan-Docs/issues/2269
if struct_.name == "VkGetLatencyMarkerInfoNV" && field.name.as_deref() == Some("pTimings") {
return Some(quote! {
/// This only sets the slice pointer. Users must manually pass the length of
/// `timings` to [`crate::extensions::nv::LowLatency2::get_latency_timings()`].
///
/// [`Self::p_timings`] will be set to to [`std::ptr::null_mut()`] if the
/// slice is empty, which is necessary to query the length of the array via
/// [`crate::extensions::nv::LowLatency2::get_latency_timings_len()`].
///
/// See <https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetLatencyTimingsNV.html#_description>
/// for more details.
#[inline]
#deprecated
pub fn timings(mut self, timings: &'a mut [LatencyTimingsFrameReportNV<'a>]) -> Self {
self.p_timings = if timings.is_empty() {
std::ptr::null_mut()
} else {
timings.as_mut_ptr()
};
self
}
});
}

if matches!(field.array, Some(vkxml::ArrayType::Dynamic)) {
if let Some(ref array_size) = field.size {
let mut slice_param_ty_tokens = field.safe_type_tokens(quote!('a), type_lifetime.clone(), None);
Expand Down