Skip to content

Commit

Permalink
Bug 1752188: Move GPUBufferDescriptor validation to server. r=jgilbert
Browse files Browse the repository at this point in the history
Depends on [`wgpu#2673`].

WebGPU requires `GPUBufferDescriptor` validation failure to:

1) generate an error on the Device timeline, and

2) mark the new buffer as invalid.

Satisfy 1) by moving most validation to the compositor process.

Requirement 2) is harder. `wgpu_core::Global::device_create_buffer`
already takes care of some validation for us, and marks the provided
buffer id invalid when there's a problem. However, there are certain
errors the spec requires us to detect which `device_create_buffer`
cannot even see, because they are caught by standard Rust validation
steps in the process of creating the `wgpu_types::BufferDescriptor`
that one would *pass to* that function. For example, if there are
bogus bits set in the `usage` property, we can't even construct a Rust
`BufferUsages` value from that to include in the `BufferDescriptor`
that we must pass to `device_create_buffer`.

This means that we need to do some validation ourselves, in the
process of constructing that `BufferDescriptor`, and use the
`Global::create_buffer_error` method added in [`wgpu#2673`] to mark
the new buffer as invalid.

[`wgpu#2673`]: gfx-rs/wgpu#2673

Differential Revision: https://phabricator.services.mozilla.com/D146768
  • Loading branch information
jimblandy committed Jun 6, 2022
1 parent 12ca318 commit 0ad04a9
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 50 deletions.
3 changes: 3 additions & 0 deletions dom/webgpu/ipc/PWebGPU.ipdl
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ using layers::CompositableHandle from "mozilla/layers/LayersTypes.h";
using RawId from "mozilla/webgpu/WebGPUTypes.h";
using dom::GPURequestAdapterOptions from "mozilla/dom/WebGPUBinding.h";
using dom::GPUCommandBufferDescriptor from "mozilla/dom/WebGPUBinding.h";
using dom::GPUBufferDescriptor from "mozilla/dom/WebGPUBinding.h";
using webgpu::ffi::WGPUHostMap from "mozilla/webgpu/ffi/wgpu.h";
using MaybeScopedError from "mozilla/webgpu/WebGPUTypes.h";

Expand Down Expand Up @@ -38,6 +39,8 @@ parent:
async CommandEncoderAction(RawId selfId, RawId aDeviceId, ByteBuf buf);
async BumpImplicitBindGroupLayout(RawId pipelineId, bool isCompute, uint32_t index, RawId assignId);

async CreateBuffer(RawId deviceId, RawId bufferId, GPUBufferDescriptor desc);

async InstanceRequestAdapter(GPURequestAdapterOptions options, RawId[] ids) returns (ByteBuf byteBuf);
async AdapterRequestDevice(RawId selfId, ByteBuf buf, RawId newId) returns (bool success);
async AdapterDestroy(RawId selfId);
Expand Down
18 changes: 3 additions & 15 deletions dom/webgpu/ipc/WebGPUChild.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -352,23 +352,11 @@ Maybe<DeviceRequest> WebGPUChild::AdapterRequestDevice(

RawId WebGPUChild::DeviceCreateBuffer(RawId aSelfId,
const dom::GPUBufferDescriptor& aDesc) {
ffi::WGPUBufferDescriptor desc = {};
nsCString label;
if (aDesc.mLabel.WasPassed()) {
LossyCopyUTF16toASCII(aDesc.mLabel.Value(), label);
desc.label = label.get();
}
desc.size = aDesc.mSize;
desc.usage = aDesc.mUsage;
desc.mapped_at_creation = aDesc.mMappedAtCreation;

ByteBuf bb;
RawId id =
ffi::wgpu_client_create_buffer(mClient.get(), aSelfId, &desc, ToFFI(&bb));
if (!SendDeviceAction(aSelfId, std::move(bb))) {
RawId bufferId = ffi::wgpu_client_make_buffer_id(mClient.get(), aSelfId);
if (!SendCreateBuffer(aSelfId, bufferId, aDesc)) {
MOZ_CRASH("IPC failure");
}
return id;
return bufferId;
}

RawId WebGPUChild::DeviceCreateTexture(RawId aSelfId,
Expand Down
22 changes: 18 additions & 4 deletions dom/webgpu/ipc/WebGPUParent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,22 @@ ipc::IPCResult WebGPUParent::RecvDeviceDestroy(RawId aSelfId) {
return IPC_OK();
}

ipc::IPCResult WebGPUParent::RecvCreateBuffer(
RawId aSelfId, RawId aBufferId, dom::GPUBufferDescriptor&& aDesc) {
nsCString label;
const char* labelOrNull = nullptr;
if (aDesc.mLabel.WasPassed()) {
LossyCopyUTF16toASCII(aDesc.mLabel.Value(), label);
labelOrNull = label.get();
}
ErrorBuffer error;
ffi::wgpu_server_device_create_buffer(mContext.get(), aSelfId, aBufferId,
labelOrNull, aDesc.mSize, aDesc.mUsage,
aDesc.mMappedAtCreation, error.ToFFI());
ForwardError(aSelfId, error);
return IPC_OK();
}

ipc::IPCResult WebGPUParent::RecvBufferReturnShmem(RawId aSelfId,
Shmem&& aShmem) {
MOZ_LOG(sLogger, LogLevel::Info,
Expand Down Expand Up @@ -744,13 +760,11 @@ ipc::IPCResult WebGPUParent::RecvSwapChainPresent(

ffi::WGPUBufferUsages usage =
WGPUBufferUsages_COPY_DST | WGPUBufferUsages_MAP_READ;
ffi::WGPUBufferDescriptor desc = {};
desc.size = bufferSize;
desc.usage = usage;

ErrorBuffer error;
ffi::wgpu_server_device_create_buffer(mContext.get(), data->mDeviceId,
&desc, bufferId, error.ToFFI());
bufferId, nullptr, bufferSize,
usage, false, error.ToFFI());
if (ForwardError(data->mDeviceId, error)) {
return IPC_OK();
}
Expand Down
2 changes: 2 additions & 0 deletions dom/webgpu/ipc/WebGPUParent.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class WebGPUParent final : public PWebGPUParent {
AdapterRequestDeviceResolver&& resolver);
ipc::IPCResult RecvAdapterDestroy(RawId aSelfId);
ipc::IPCResult RecvDeviceDestroy(RawId aSelfId);
ipc::IPCResult RecvCreateBuffer(RawId aSelfId, RawId aBufferId,
dom::GPUBufferDescriptor&& aDesc);
ipc::IPCResult RecvBufferReturnShmem(RawId aSelfId, Shmem&& aShmem);
ipc::IPCResult RecvBufferMap(RawId aSelfId, ffi::WGPUHostMap aHostMap,
uint64_t aOffset, uint64_t size,
Expand Down
3 changes: 3 additions & 0 deletions dom/webgpu/ipc/WebGPUSerialize.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ DEFINE_IPC_SERIALIZER_WITHOUT_FIELDS(mozilla::dom::GPUCommandBufferDescriptor);
DEFINE_IPC_SERIALIZER_WITH_FIELDS(mozilla::dom::GPURequestAdapterOptions,
mPowerPreference, mForceFallbackAdapter);

DEFINE_IPC_SERIALIZER_WITH_FIELDS(mozilla::dom::GPUBufferDescriptor, mSize,
mUsage, mMappedAtCreation);

DEFINE_IPC_SERIALIZER_WITH_FIELDS(mozilla::webgpu::ScopedError, operationError,
validationMessage);

Expand Down
1 change: 1 addition & 0 deletions gfx/wgpu_bindings/cbindgen.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ exclude = [
"Option_BufferSize", "Option_NonZeroU32", "Option_NonZeroU8",
"ANativeWindow_setBuffersGeometry",
]
include = ["BufferUsages"]

[export.rename]
"BufferDescriptor_RawString" = "BufferDescriptor"
Expand Down
20 changes: 0 additions & 20 deletions gfx/wgpu_bindings/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,26 +475,6 @@ pub extern "C" fn wgpu_client_make_buffer_id(
.alloc(backend)
}

#[no_mangle]
pub extern "C" fn wgpu_client_create_buffer(
client: &Client,
device_id: id::DeviceId,
desc: &wgt::BufferDescriptor<RawString>,
bb: &mut ByteBuf,
) -> id::BufferId {
let backend = device_id.backend();
let id = client
.identities
.lock()
.select(backend)
.buffers
.alloc(backend);

let action = DeviceAction::CreateBuffer(id, desc.map_label(cow_label));
*bb = make_byte_buf(&action);
id
}

#[no_mangle]
pub extern "C" fn wgpu_client_create_texture(
client: &Client,
Expand Down
1 change: 0 additions & 1 deletion gfx/wgpu_bindings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ struct ImplicitLayout<'a> {

#[derive(serde::Serialize, serde::Deserialize)]
enum DeviceAction<'a> {
CreateBuffer(id::BufferId, wgc::resource::BufferDescriptor<'a>),
CreateTexture(id::TextureId, wgc::resource::TextureDescriptor<'a>),
CreateSampler(id::SamplerId, wgc::resource::SamplerDescriptor<'a>),
CreateBindGroupLayout(
Expand Down
32 changes: 22 additions & 10 deletions gfx/wgpu_bindings/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,12 +211,30 @@ pub extern "C" fn wgpu_server_device_drop(global: &Global, self_id: id::DeviceId
pub extern "C" fn wgpu_server_device_create_buffer(
global: &Global,
self_id: id::DeviceId,
desc: &wgt::BufferDescriptor<RawString>,
new_id: id::BufferId,
buffer_id: id::BufferId,
label_or_null: RawString,
size: wgt::BufferAddress,
usage: u32,
mapped_at_creation: bool,
mut error_buf: ErrorBuffer,
) {
let desc = desc.map_label(cow_label);
let (_, error) = gfx_select!(self_id => global.device_create_buffer(self_id, &desc, new_id));
let label = cow_label(&label_or_null);
let usage = match wgt::BufferUsages::from_bits(usage) {
Some(usage) => usage,
None => {
error_buf.init_str("GPUBufferDescriptor's 'usage' includes invalid unimplemented bits \
or unimplemented usages");
gfx_select!(self_id => global.create_buffer_error(buffer_id, label));
return;
}
};
let desc = wgc::resource::BufferDescriptor {
label,
size,
usage,
mapped_at_creation,
};
let (_, error) = gfx_select!(self_id => global.device_create_buffer(self_id, &desc, buffer_id));
if let Some(err) = error {
error_buf.init(err);
}
Expand Down Expand Up @@ -285,12 +303,6 @@ impl Global {
mut error_buf: ErrorBuffer,
) {
match action {
DeviceAction::CreateBuffer(id, desc) => {
let (_, error) = self.device_create_buffer::<A>(self_id, &desc, id);
if let Some(err) = error {
error_buf.init(err);
}
}
DeviceAction::CreateTexture(id, desc) => {
let (_, error) = self.device_create_texture::<A>(self_id, &desc, id);
if let Some(err) = error {
Expand Down

0 comments on commit 0ad04a9

Please sign in to comment.