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

Introduce ExternalStagingBuffer for staging buffer uses outside of wgpu-core #6090

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 11 additions & 8 deletions wgpu-core/src/device/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use crate::{
lock::RwLockWriteGuard,
resource::{
Buffer, BufferAccessError, BufferMapState, DestroyedBuffer, DestroyedResourceError,
DestroyedTexture, FlushedStagingBuffer, Labeled, ParentDevice, ResourceErrorIdent,
StagingBuffer, Texture, TextureInner, Trackable,
DestroyedTexture, ExternalStagingBuffer, FlushedStagingBuffer, Labeled, ParentDevice,
ResourceErrorIdent, StagingBuffer, Texture, TextureInner, Trackable,
},
resource_log,
track::{self, Tracker, TrackerIndex},
Expand Down Expand Up @@ -438,8 +438,8 @@ impl Global {
&self,
queue_id: QueueId,
buffer_size: wgt::BufferSize,
id_in: Option<id::StagingBufferId>,
) -> Result<(id::StagingBufferId, NonNull<u8>), QueueWriteError> {
id_in: Option<id::ExternalStagingBufferId>,
) -> Result<(id::ExternalStagingBufferId, NonNull<u8>), QueueWriteError> {
profiling::scope!("Queue::create_staging_buffer");
let hub = A::hub(self);

Expand All @@ -450,11 +450,10 @@ impl Global {

let device = &queue.device;

let staging_buffer = StagingBuffer::new(device, buffer_size)?;
let ptr = unsafe { staging_buffer.ptr() };
let (staging_buffer, ptr) = ExternalStagingBuffer::new(device, buffer_size)?;

let fid = hub.staging_buffers.prepare(id_in);
let id = fid.assign(Arc::new(staging_buffer));
let id = fid.assign(staging_buffer);
resource_log!("Queue::create_staging_buffer {id:?}");

Ok((id, ptr))
Expand All @@ -465,7 +464,7 @@ impl Global {
queue_id: QueueId,
buffer_id: id::BufferId,
buffer_offset: wgt::BufferAddress,
staging_buffer_id: id::StagingBufferId,
staging_buffer_id: id::ExternalStagingBufferId,
) -> Result<(), QueueWriteError> {
profiling::scope!("Queue::write_staging_buffer");
let hub = A::hub(self);
Expand All @@ -483,6 +482,10 @@ impl Global {
.and_then(Arc::into_inner)
.ok_or_else(|| QueueWriteError::Transfer(TransferError::InvalidBufferId(buffer_id)))?;

// TODO: bubble up safety requirements, they currently hold but we should make functions
Copy link
Member

Choose a reason for hiding this comment

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

question(blocking): Should this be resolved before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't intend to resolve this in this PR. After degenerification and removal of registries we might not even need to resolve it since we can make the API safe by not giving users the pointer at all.

Copy link
Member

@ErichDonGubler ErichDonGubler Aug 13, 2024

Choose a reason for hiding this comment

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

Ah, shall we file an issue, then, and remove this comment? If we don't want an issue to track it, then I wonder if we should even include the comment.

Will downgrade to non-blocking with a response to the above, trusting that you've thought this through.

// unsafe and add safety comments
let staging_buffer = unsafe { staging_buffer.into_inner() };

let mut pending_writes = device.pending_writes.lock();

// At this point, we have taken ownership of the staging_buffer from the
Expand Down
4 changes: 2 additions & 2 deletions wgpu-core/src/hub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ use crate::{
instance::{Adapter, Surface},
pipeline::{ComputePipeline, PipelineCache, RenderPipeline, ShaderModule},
registry::{Registry, RegistryReport},
resource::{Buffer, QuerySet, Sampler, StagingBuffer, Texture, TextureView},
resource::{Buffer, ExternalStagingBuffer, QuerySet, Sampler, Texture, TextureView},
storage::{Element, Storage},
};
use std::fmt::Debug;
Expand Down Expand Up @@ -184,7 +184,7 @@ pub struct Hub<A: HalApi> {
pub(crate) pipeline_caches: Registry<PipelineCache<A>>,
pub(crate) query_sets: Registry<QuerySet<A>>,
pub(crate) buffers: Registry<Buffer<A>>,
pub(crate) staging_buffers: Registry<StagingBuffer<A>>,
pub(crate) staging_buffers: Registry<ExternalStagingBuffer<A>>,
pub(crate) textures: Registry<Texture<A>>,
pub(crate) texture_views: Registry<TextureView<A>>,
pub(crate) samplers: Registry<Sampler<A>>,
Expand Down
2 changes: 1 addition & 1 deletion wgpu-core/src/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ ids! {
pub type DeviceId Device;
pub type QueueId Queue;
pub type BufferId Buffer;
pub type StagingBufferId StagingBuffer;
pub type ExternalStagingBufferId ExternalStagingBuffer;
pub type TextureViewId TextureView;
pub type TextureId Texture;
pub type SamplerId Sampler;
Expand Down
43 changes: 34 additions & 9 deletions wgpu-core/src/resource.rs
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,37 @@ impl<A: HalApi> Drop for DestroyedBuffer<A> {
}
}

/// A temporary buffer, that can be created with [`queue_create_staging_buffer`]
/// and used with [`queue_write_staging_buffer`].
///
/// [`queue_create_staging_buffer`]: Global::queue_create_staging_buffer
/// [`queue_write_staging_buffer`]: Global::queue_write_staging_buffer
#[derive(Debug)]
pub struct ExternalStagingBuffer<A: HalApi> {
inner: StagingBuffer<A>,
}

impl<A: HalApi> ExternalStagingBuffer<A> {
pub(crate) fn new(
device: &Arc<Device<A>>,
size: wgt::BufferSize,
) -> Result<(Arc<Self>, NonNull<u8>), DeviceError> {
let inner = StagingBuffer::new(device, size)?;
let ptr = unsafe { inner.ptr() };
let staging_buffer = Self { inner };
let staging_buffer = Arc::new(staging_buffer);
Ok((staging_buffer, ptr))
}

/// SAFETY: You must have stopped using the pointer returned by [`Self::new`].
pub(crate) unsafe fn into_inner(self) -> StagingBuffer<A> {
self.inner
}
}

crate::impl_resource_type!(ExternalStagingBuffer);
crate::impl_storage_item!(ExternalStagingBuffer);

#[cfg(send_sync)]
unsafe impl<A: HalApi> Send for StagingBuffer<A> {}
#[cfg(send_sync)]
Expand All @@ -801,17 +832,14 @@ unsafe impl<A: HalApi> Sync for StagingBuffer<A> {}
/// is always created mapped, and the command that uses it destroys the buffer
/// when it is done.
///
/// [`StagingBuffer`]s can be created with [`queue_create_staging_buffer`] and
/// used with [`queue_write_staging_buffer`]. They are also used internally by
/// operations like [`queue_write_texture`] that need to upload data to the GPU,
/// but that don't belong to any particular wgpu command buffer.
/// They are used internally by operations like [`queue_write_texture`]
/// that need to upload data to the GPU, but that don't belong to any
/// particular wgpu command buffer.
///
/// Used `StagingBuffer`s are accumulated in [`Device::pending_writes`], to be
/// freed once their associated operation's queue submission has finished
/// execution.
///
/// [`queue_create_staging_buffer`]: Global::queue_create_staging_buffer
/// [`queue_write_staging_buffer`]: Global::queue_write_staging_buffer
/// [`queue_write_texture`]: Global::queue_write_texture
/// [`Device::pending_writes`]: crate::device::Device
#[derive(Debug)]
Expand Down Expand Up @@ -913,9 +941,6 @@ impl<A: HalApi> StagingBuffer<A> {
}
}

crate::impl_resource_type!(StagingBuffer);
crate::impl_storage_item!(StagingBuffer);

#[derive(Debug)]
pub struct FlushedStagingBuffer<A: HalApi> {
raw: ManuallyDrop<A::Buffer>,
Expand Down
2 changes: 1 addition & 1 deletion wgpu/src/backend/wgpu_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3435,7 +3435,7 @@ impl From<CreateShaderModuleError> for CompilationInfo {

#[derive(Debug)]
pub struct QueueWriteBuffer {
buffer_id: wgc::id::StagingBufferId,
buffer_id: wgc::id::ExternalStagingBufferId,
mapping: BufferMappedRange,
}

Expand Down
Loading