From 2a94f1dd86a94a8772b48d20c01a04f574cf1290 Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Thu, 18 Apr 2024 18:31:48 +1000 Subject: [PATCH 1/7] Made vulkan::CommandEncoder::discard_encoding idempotent --- wgpu-hal/src/vulkan/command.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index 43a2471954..6d874d379f 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -104,8 +104,10 @@ impl crate::CommandEncoder for super::CommandEncoder { } unsafe fn discard_encoding(&mut self) { - self.discarded.push(self.active); - self.active = vk::CommandBuffer::null(); + if self.active != vk::CommandBuffer::null() { + self.discarded.push(self.active); + self.active = vk::CommandBuffer::null(); + } } unsafe fn reset_all(&mut self, cmd_bufs: I) From 9e268c45ac848fe51aa431ae612fd2b69cd1f05f Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Thu, 18 Apr 2024 18:47:58 +1000 Subject: [PATCH 2/7] Add vulkan::CommandEncoder Drop impl that calls discard_encoding --- wgpu-hal/src/vulkan/command.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index 6d874d379f..a2afe4d152 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -45,6 +45,13 @@ impl super::DeviceShared { } } +impl Drop for super::CommandEncoder { + fn drop(&mut self) { + use crate::CommandEncoder; + unsafe { self.discard_encoding() } + } +} + impl super::CommandEncoder { fn write_pass_end_timestamp_if_requested(&mut self) { if let Some((query_set, index)) = self.end_of_pass_timer_query.take() { From cf5085af82e1e9b58f068940105a2123f9b9a4a4 Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Thu, 18 Apr 2024 23:15:26 +1000 Subject: [PATCH 3/7] Documentation and changelog --- CHANGELOG.md | 4 ++++ wgpu-hal/src/lib.rs | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a54b33d290..05ea2916cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,10 @@ Bottom level categories: - Implement the `device_set_device_lost_callback` method for `ContextWebGpu`. By @suti in [#5438](https://github.com/gfx-rs/wgpu/pull/5438) - Add support for storage texture access modes `ReadOnly` and `ReadWrite`. By @JolifantoBambla in [#5434](https://github.com/gfx-rs/wgpu/pull/5434) +#### Vulkan + +- Make `CommandEncoder::discard_encoding` do nothing if called multiple times, and implement `Drop` for `CommandEncoder` to call `discard_encoding`. By @villuna + ### Bug Fixes #### General diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 1aff081606..5574463dea 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -617,6 +617,10 @@ pub trait Queue: WasmNotSendSync { /// live `CommandBuffers` built from it. All the `CommandBuffer`s /// are destroyed, and their resources are freed. /// +/// You may want to implement the [core::ops::Drop] trait to discard +/// the current commands before the encoder is dropped (e.g. using +/// [CommandEncoder::discard_encoding]). +/// /// # Safety /// /// - The `CommandEncoder` must be in the states described above to @@ -652,6 +656,10 @@ pub trait CommandEncoder: WasmNotSendSync + fmt::Debug { /// /// This puts this `CommandEncoder` in the "closed" state. /// + /// Implementations of this function must be idempotent, i.e. + /// if the function has just been called, calling it again should + /// not do anything. + /// /// # Safety /// /// This `CommandEncoder` must be in the "recording" state. From 9be4ce4b42d46ae3e0047eac42064e487fa37428 Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Sun, 21 Apr 2024 18:27:26 +1000 Subject: [PATCH 4/7] Fixed documentation, changed discard_encoding null check to an assertion --- CHANGELOG.md | 5 +---- wgpu-hal/src/lib.rs | 12 ++++-------- wgpu-hal/src/vulkan/command.rs | 11 +++++++---- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05ea2916cd..0ac28aac07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,10 +137,6 @@ Bottom level categories: - Implement the `device_set_device_lost_callback` method for `ContextWebGpu`. By @suti in [#5438](https://github.com/gfx-rs/wgpu/pull/5438) - Add support for storage texture access modes `ReadOnly` and `ReadWrite`. By @JolifantoBambla in [#5434](https://github.com/gfx-rs/wgpu/pull/5434) -#### Vulkan - -- Make `CommandEncoder::discard_encoding` do nothing if called multiple times, and implement `Drop` for `CommandEncoder` to call `discard_encoding`. By @villuna - ### Bug Fixes #### General @@ -184,6 +180,7 @@ Bottom level categories: #### Vulkan - Set object labels when the DEBUG flag is set, even if the VALIDATION flag is disabled. By @DJMcNab in [#5345](https://github.com/gfx-rs/wgpu/pull/5345). +- Add safety check to `wgpu_hal::vulkan::CommandEncoder` to make sure `discard_encoding` is not called in the closed state. By @villuna in [#5557](https://github.com/gfx-rs/wgpu/pull/5557) #### Metal diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 5574463dea..517c0992e9 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -617,10 +617,6 @@ pub trait Queue: WasmNotSendSync { /// live `CommandBuffers` built from it. All the `CommandBuffer`s /// are destroyed, and their resources are freed. /// -/// You may want to implement the [core::ops::Drop] trait to discard -/// the current commands before the encoder is dropped (e.g. using -/// [CommandEncoder::discard_encoding]). -/// /// # Safety /// /// - The `CommandEncoder` must be in the states described above to @@ -656,13 +652,13 @@ pub trait CommandEncoder: WasmNotSendSync + fmt::Debug { /// /// This puts this `CommandEncoder` in the "closed" state. /// - /// Implementations of this function must be idempotent, i.e. - /// if the function has just been called, calling it again should - /// not do anything. - /// /// # Safety /// /// This `CommandEncoder` must be in the "recording" state. + /// + /// Callers must not assume that implementations of this + /// function is idempotent. Calling it multiple times in a + /// row might not necessarily be safe. unsafe fn discard_encoding(&mut self); /// Return a fresh [`CommandBuffer`] holding the recorded commands. diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index a2afe4d152..7df6eae2d4 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -111,10 +111,13 @@ impl crate::CommandEncoder for super::CommandEncoder { } unsafe fn discard_encoding(&mut self) { - if self.active != vk::CommandBuffer::null() { - self.discarded.push(self.active); - self.active = vk::CommandBuffer::null(); - } + // Safe use requires this is not called in the close state, so the buffer + // shouldn't be null. Assert this to make sure we're not pushing null + // buffers to the discard pile. + assert_ne!(self.active, vk::CommandBuffer::null()); + + self.discarded.push(self.active); + self.active = vk::CommandBuffer::null(); } unsafe fn reset_all(&mut self, cmd_bufs: I) From 47770870244053b0c69a80db327566dbf168f7f4 Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Sun, 21 Apr 2024 18:49:14 +1000 Subject: [PATCH 5/7] Remove drop implementation for vulkan::CommandEncoder --- wgpu-hal/src/vulkan/command.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index 7df6eae2d4..20c7a4d601 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -45,13 +45,6 @@ impl super::DeviceShared { } } -impl Drop for super::CommandEncoder { - fn drop(&mut self) { - use crate::CommandEncoder; - unsafe { self.discard_encoding() } - } -} - impl super::CommandEncoder { fn write_pass_end_timestamp_if_requested(&mut self) { if let Some((query_set, index)) = self.end_of_pass_timer_query.take() { From da4d4c1362bc3304ceb9e24cbbad6d2539cae442 Mon Sep 17 00:00:00 2001 From: Luna Borella Date: Sun, 21 Apr 2024 19:00:54 +1000 Subject: [PATCH 6/7] Rephrased and fixed a typo in the safety doc comment --- wgpu-hal/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 517c0992e9..0484437aba 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -657,8 +657,8 @@ pub trait CommandEncoder: WasmNotSendSync + fmt::Debug { /// This `CommandEncoder` must be in the "recording" state. /// /// Callers must not assume that implementations of this - /// function is idempotent. Calling it multiple times in a - /// row might not necessarily be safe. + /// function are idempotent, so should not call it + /// multiple times in a row. unsafe fn discard_encoding(&mut self); /// Return a fresh [`CommandBuffer`] holding the recorded commands. From 774ef3d792f586b393384e341f14ee250731a6e7 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 22 Apr 2024 12:18:30 -0700 Subject: [PATCH 7/7] minor copy edits --- wgpu-hal/src/lib.rs | 4 ++-- wgpu-hal/src/vulkan/command.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 0484437aba..8fffca015d 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -645,7 +645,7 @@ pub trait CommandEncoder: WasmNotSendSync + fmt::Debug { /// This `CommandEncoder` must be in the "closed" state. unsafe fn begin_encoding(&mut self, label: Label) -> Result<(), DeviceError>; - /// Discard the command list under construction, if any. + /// Discard the command list under construction. /// /// If an error has occurred while recording commands, this /// is the only safe thing to do with the encoder. @@ -657,7 +657,7 @@ pub trait CommandEncoder: WasmNotSendSync + fmt::Debug { /// This `CommandEncoder` must be in the "recording" state. /// /// Callers must not assume that implementations of this - /// function are idempotent, so should not call it + /// function are idempotent, and thus should not call it /// multiple times in a row. unsafe fn discard_encoding(&mut self); diff --git a/wgpu-hal/src/vulkan/command.rs b/wgpu-hal/src/vulkan/command.rs index 20c7a4d601..ceb44dfbe6 100644 --- a/wgpu-hal/src/vulkan/command.rs +++ b/wgpu-hal/src/vulkan/command.rs @@ -104,7 +104,7 @@ impl crate::CommandEncoder for super::CommandEncoder { } unsafe fn discard_encoding(&mut self) { - // Safe use requires this is not called in the close state, so the buffer + // Safe use requires this is not called in the "closed" state, so the buffer // shouldn't be null. Assert this to make sure we're not pushing null // buffers to the discard pile. assert_ne!(self.active, vk::CommandBuffer::null());