Add initial instance and adapter interface by Kangz · Pull Request #23 · webgpu-native/webgpu-headers

Kangz · 2019-11-12T13:28:34Z

Also moves the surface creation to the instance.

PTAL, this is meant to start the discussion on what the interface for WGPUInstance and WGPUAdapter should look like.

kainino0x · 2019-11-12T15:17:57Z

 WGPU_EXPORT void wgpuFenceOnCompletion(WGPUFence fence, uint64_t value, WGPUFenceOnCompletionCallback callback, void * userdata);

+// Methods of Instance
+WGPU_EXPORT WGPUAdapter wgpuInstanceCreateAdapter(WGPUInstance instance, WGPUAdapterDescriptor const * descriptor);


This one would also need to be async

kainino0x · 2019-11-12T15:18:18Z

 WGPU_EXPORT WGPUProc WGPUGetProcAddress(WGPUDevice device, const char* procName);

+// Methods of Adapter
+WGPU_EXPORT WGPUDevice wgpuAdapterCreateDevice(WGPUAdapter adapter, WGPUDeviceDescriptor const * descriptor);


for wasm, would need to be async (take a callback)

Agreed that we should provide an async version that 100% mirrors requestDevice, how about the following:

// Maybe we could not have the status and instead use the device lost callback, would this match WebGPU better? enum WGPUAdapterRequestDeviceCallbackStatus{ WGPUAdapterRequestDeviceCallbackStatus_Success, WGPUAdapterRequestDeviceCallbackStatus_ValidationError, WGPUAdapterRequestDeviceCallbackStatus_CreationError, WGPUAdapterRequestDeviceCallbackStatus_Unknown, }; typedef void (*WGPUAdapterRequestDeviceCallback)(WGPUAdapterRequestDeviceCallbackStatus status, WGPUDevice device); void wgpuAdapterRequestDevice(WGPUAdapter, WGPUDeviceDescriptor * const descriptor);

This would require having sorted out what webgpu.h's event loop looks like.

I still think there might be some divergence between WebGPU in JS and native for device creation, so maybe we could also have the immediate creation.

requestDevice and requestAdapter can fail (reject with DOMException) in WebGPU, so we should keep the status.

Having both async and immediate creation would be ok.

if we keep the immediate creation, how would this translate to WASM?

If we want to have a synchronous version of these, we should probably move them behind a #ifdef WGPU_NATIVE_ONLY or separate header. For example, if an application is using these and tries to compile to wasm for the web, we can't make the synchronous version work (because we can't yield to the browser), so we'd have to abort/panic whenever they're called on the web.

I'd slightly prefer to match the WebIDL – mostly to avoid applications using these and later realizing that they can't actually compile to wasm. Applications not targeting wasm could use a small helper function to block on the async call and spin if they'd like (though I can appreciate that's not ideal).

It might actually be very handy for applications to be able to pretend some operations are synchronous, and use compiler functionality like Emscripten's ASYNCIFY.

But I agree with putting any such things behind an ifdef.

I think we should have both a synchronous and asynchronous version. If we provided only the synchronous version, people would still synchronously tick the event loop until the callback is called. It adds unnecessary hassle (in native device and adapter creation is synchronous), and it wouldn't help WASM ports because without ASYNCIFY they need to return all the way to the browser, spinning doesn't work.

So we should have createDevice/Adapter and requestDevice/Adapter. And I don't think it should be behind an ifdef because the synchronous versions are still useful in emscripten so the loader JS outside of the module can pass an adapter or device that's the one to be returned by createAdapter/Device. I feel less strongly about this (-3 because our code generator will doesn't support such ifdefs, -2 otherwise).

If the preload JS sets a "preinitialized" device, I think we should just provide an emscripten_* entry point to grab that, instead of doing it through createDevice, because the C code calling createDevice would have to be modified anyway to not do createAdapter.

For adapter creation, it might be useful for createAdapter to return a preinitialized adapter, because the C code might not have to be modified. Plus, returning a preinitialized adapter is much more straightforward: it's valid to ignore all of the GPURequestAdapterOptions, which is not true of GPUDeviceDescriptor.

If we provided only the synchronous version, people would still synchronously tick the event loop until the callback is called. It adds unnecessary hassle (in native device and adapter creation is synchronous), and it wouldn't help WASM ports because without ASYNCIFY they need to return all the way to the browser, spinning doesn't work.

FWIW this is basically how the SDL main loop works with Emscripten – it just means the adapter and device are also initialized as part of the main loop. In Rust I was thinking we could return Futures to make it easy to do async initialization and thought we could do something similar in C++/other languages that have some concept of tasks/futures.

So we should have createDevice/Adapter and requestDevice/Adapter. And I don't think it should be behind an ifdef because the synchronous versions are still useful in emscripten

How could we expose createDevice and createAdapter meaningfully for wasm targets when Emscripten isn't used though? It seems like we would have to panic depending on which target+compiler combination is used. If we do expose both versions I'd prefer to feature-gate the synchronous versions somehow (ifdef/separate header/etc.).

kvark · 2019-11-12T21:39:35Z

 typedef WGPUProc (*WGPUProcGetProcAddress)(WGPUDevice device, const char* procName);

+// Procs of Adapter
+typedef WGPUDevice (*WGPUProcAdapterCreateDevice)(WGPUAdapter adapter, WGPUDeviceDescriptor const * descriptor);


Are you intentionally calling it different from the upstream spec, which has RequestXxx for both device and adapter?
We'd need to talk a bit (on Thursday?) about async stuff in the native headers

This was intentional because it is synchronous so it doesn't match the spec. createXXX is synchronous while requestXXX is asynchronous. Happy to chat about it next time we talk!

kvark · 2019-11-12T21:45:44Z

 WGPU_EXPORT WGPUProc WGPUGetProcAddress(WGPUDevice device, const char* procName);

+// Methods of Adapter
+WGPU_EXPORT WGPUDevice wgpuAdapterCreateDevice(WGPUAdapter adapter, WGPUDeviceDescriptor const * descriptor);


if we keep the immediate creation, how would this translate to WASM?

kvark · 2019-11-12T21:47:05Z


+// Methods of Instance
+WGPU_EXPORT WGPUAdapter wgpuInstanceCreateAdapter(WGPUInstance instance, WGPUAdapterDescriptor const * descriptor);
+WGPU_EXPORT WGPUSurface wgpuInstanceCreateSurface(WGPUInstance instance, WGPUSurfaceDescriptor const * descriptor);


I wonder if this needs to be async as well...

I don't think there will be anything async in here. Just getContext (or maybe the surface descriptor gives us the context) and configureSwapChain (or some future equivalent thereof).

getSwapChainPreferredFormat is async, but not called inside here.

grovesNL · 2019-11-13T01:23:39Z

 typedef struct WGPUComputePipelineImpl* WGPUComputePipeline;
 typedef struct WGPUDeviceImpl* WGPUDevice;
 typedef struct WGPUFenceImpl* WGPUFence;
+typedef struct WGPUInstanceImpl* WGPUInstance;


What's the benefit of having the separate Instance struct on native vs. just creating the adapter (without the adapter being attached to anything)?

Some reasons off the top of my head:

provides a place to map adapter ids to adapter objects

provides a place for an event loop (however that ends up working)

The Instance is the global object through which wgpu is used, and avoids the need for global static variables (unfortunately we have differing designs here). Global variables like these are useful for:

Dependency injection before the implementation does anything.

Caching queried adapters etc.

Setting global options (like enable validation, or start capture) that are sometimes needed by backends when we create adapters. (could be in descriptors but we don't want to expose that in webgpu.h, these are dawn-specific concerns)

Oh, sorry for the race conditions

grovesNL · 2019-11-13T01:42:47Z

 WGPU_EXPORT WGPUProc WGPUGetProcAddress(WGPUDevice device, const char* procName);

+// Methods of Adapter
+WGPU_EXPORT WGPUDevice wgpuAdapterCreateDevice(WGPUAdapter adapter, WGPUDeviceDescriptor const * descriptor);


If we want to have a synchronous version of these, we should probably move them behind a #ifdef WGPU_NATIVE_ONLY or separate header. For example, if an application is using these and tries to compile to wasm for the web, we can't make the synchronous version work (because we can't yield to the browser), so we'd have to abort/panic whenever they're called on the web.

I'd slightly prefer to match the WebIDL – mostly to avoid applications using these and later realizing that they can't actually compile to wasm. Applications not targeting wasm could use a small helper function to block on the async call and spin if they'd like (though I can appreciate that's not ideal).

Kangz · 2019-11-15T16:14:04Z

Had a chat about this with @kvark, though it was mostly focused on the event loop. Summary is:

the instance object is useful and should have the ProcessEvent call.
createDevice and createAdapter can just be helper functions around requestAdapter, requestDevice and ProcessEvent. It's bad to loop on ProcessEvent so we could produce an error if compiling for emscripten. Also emscripten could have a way to inject devices and adapters that's not in webgpu.h.
Applications in native will want to have access to the full list of adapters but we didn't discuss of a design for that.

Kangz · 2019-11-21T15:01:34Z

PTAL again:

Updated for asynchronous WGPUDevice and WGPUAdapter creation
Added a WGPUInstanceDescriptor
Added wgpuInstanceProcessEvents
Added callback support to our generator so the callback definitions were rewritten to be on one line.

Also moves the surface creation to the instance.

kvark

I think we can proceed with this, thanks for the update!

Kangz requested review from grovesNL and kvark November 12, 2019 13:28

kainino0x reviewed Nov 12, 2019

View reviewed changes

kvark reviewed Nov 12, 2019

View reviewed changes

grovesNL reviewed Nov 13, 2019

View reviewed changes

Kangz force-pushed the instance branch from 998c142 to f8ec405 Compare November 21, 2019 14:59

Add initial instance and adapter interface

cc1ffaa

Also moves the surface creation to the instance.

Kangz force-pushed the instance branch from f8ec405 to cc1ffaa Compare November 21, 2019 15:04

kvark approved these changes Nov 22, 2019

View reviewed changes

kvark merged commit 7e91852 into webgpu-native:master Nov 22, 2019

grovesNL mentioned this pull request Jan 18, 2020

Case for using wgpu-core gfx-rs/wgpu-rs#156

Closed

Kangz deleted the instance branch September 30, 2022 12:51

Uh oh!

Conversation

Kangz commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kangz commented Nov 15, 2019

Uh oh!

Kangz commented Nov 21, 2019

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants