Device valid

[bradwerth]

Checklist

• Run cargo clippy.
• Run cargo clippy --target wasm32-unknown-unknown if applicable.
• Add change to CHANGELOG.md. See simple instructions inside file.

Connections
None.

Description
This implements a simple version of "lose the device", which marks a Device as invalid, causing all subsequent operations to return an error. Nothing actually calls this yet.

Testing
This adds an integration test device_lose_then_more that attempts various operations on a lost device, which are all expected to fail.

[Wumpf]

Thank you for your contribution! But I fear this might be the wrong general direction for implementing device loss 😟

According to spec, entering the device-lost state is not directly exposed other than via a destroy call, but otherwise is an error state the device can enter.
https://www.w3.org/TR/webgpu/#lose-the-device

There is however a lost promise that we need to implement here, MDN has some example code on this what it should look like from JS https://developer.mozilla.org/en-US/docs/Web/API/GPUDevice/lost

I think we should try to handle device lost states through the HAL layer - that's where they typically occur. Device loss via destroy is harder to map to that ofc, but we should investigate if we can "poison" the hal device so that this actually has the desired effect through the entire stack.

[nical]

I think we should try to handle device lost states through the HAL layer - that's where they typically occur. Device loss via destroy is harder to map to that ofc, but we should investigate if we can "poison" the hal device so that this actually has the desired effect through the entire stack.

I think that we should be able to implement most of device.destroy and device loss via the same infrastructure in wgpu-core, it would be simple to keep the associated validation there. Also, all of the asynchronous deallocation logic is in core so I don't think we can escape having the destroyed state handle there.

What advantages do you expect from moving it in hal?

[Wumpf]

Generally, a device loss event is an actual thing that happens to the hal device; different implementations have different error states. Relatedly, I think destroying/loosing a device should actually have an actual effect that is communicated with the driver.

[Wumpf]

I fear if we put too much logic in wgpu-core, we have two sources of device-lost state: The hal device that is actually in a bad state / deallocated / etc. and some flag on wgpu-core. So to keep this in sync we could instead solely rely on hal owning this state.

[nical]

It looks like you are looking at this primarily through the lense of device loss, while Brad's priority here is to get device.destroy working (we direly need it for CTS runs to not randomly OOM). The effect of that will be to clean up all of the device's allocated resources (while the handles are still alive, hence the need for validation everywhere which is in core) but that's not reflected in the PR yet.

I haven't looked at the PR in details yet but perhaps some of the "lose" terminology in there could be replaced by destroy to make that clearer.

We could store the device's state in the hal structure, but it would still be mostly accessed by the validation in wgpu-core.

[bradwerth]

This is definitely a partial implementation of "lose the device" which is itself a partial implementation of device.destroy. From my read of the spec, we lose the device in 3 ways:

1. It's the final steps of device.destroy.
2. As needed from each backend.
3. As part of the error response in adapter.request_device. This case seems least important since it already returns an error and not a device, so there's nothing for the caller to check a lost promise against. But if there's internal state cleanup associated with "lose the device" it happens in this case, too.

In this patch, my goal was to support these cases at least in theory, but since the integration test is checking the outcome of lose the device, it makes it look like a new callable function on the device object itself. It also doesn't attempt to handle the lost promise resolution for two reasons: because of my lack of familiarity with the direct and web protocols, and the implementation in Firefox will be external to wgpu. Regarding the direct and web implementations of the lost promises, I should either add much better TODOs to document the gaps, or fill the gaps with an implementation.

[jimblandy]

@Wumpf I think you're right to ask how this code is going to interact with device loss reported from the underlying platform APIs. This PR needs to include a clear plan for how that is going to be incorporated.

As @nical said, our immediate goal is to implement something corresponding to the spec's GPUDevice.destroy.

However, in doing so we've run into an unfortunate part of wgpu's current architecture.

When the hal reports a lost device, WebGPU says we should execute the "lose the device" steps you mentioned above. The first step there is "Make device invalid."

Everywhere else in wgpu-core treats WebGPU invalidity by placing an Element::Error in the appropriate Registry field of the Hub. However, that was designed for objects that are invalid from the point of creation, not objects that become invalid after having previously been valid. If we were to implement GPUDevice.destroy by removing the Device from the Hub, it would be impossible for wgpu-core to continue to process the queue, as required by steps 4 and 5 of "lose the device":

4. Complete any outstanding mapAsync() steps.

5. Complete any outstanding onSubmittedWorkDone() steps.

In gpuweb/gpuweb#4294 I confirmed with @Kangz that we're not supposed to be able to make further requests of the device after losing it, so it does seem important that the device be marked invalid at the indicated point in execution.

@bradwerth explained this to me, and said that adding the valid flag to the wgpu_core Device seemed like it would be required in order to prevent the Device from handling new user requests, while still being able to process the queue. We agreed it was ugly and unfortunate, but we couldn't see a better way to do it.

I believe #3626 will change wpgu-core to use Arc for all internal references to devices, so when that lands we may be able to remove the valid flag, and implement "lose the device" simply by removing the Device's entry from its Registry. Then all API requests will automatically be unable to access the Device, and we can remove the valid flag.

[bradwerth]

Presuming that the wgpu-core implementation of "lose the device" can be correctly signaled by the driver, then I don't think there is any danger of inconsistent state. Since wgpu-core device has the valid/invalid state, and prevents further requests from reaching the hal device, then I don't believe there is double-state that can become inconsistent. The hal device is never again accessed.

[jimblandy]

Presuming that the wgpu-core implementation of "lose the device" can be correctly signaled by the driver, then I don't think there is any danger of inconsistent state. Since wgpu-core device has the valid/invalid state, and prevents further requests from reaching the hal device, then I don't believe there is double-state that can become inconsistent. The hal device is never again accessed.

This isn't quite right. For example, Vulkan requires that all the resources created from a device be freed before the device itself is freed, and this requirement holds even if vkQueueSubmit returns VK_ERROR_DEVICE_LOST. The architecture of wgpu is that wgpu-hal is not responsible for tracking a device's resources and cleaning them up; enforcing anything but the simplest invariants is wgpu-core's role. So when wgpu_hal::vulkan::Queue::submit calls ash::device::Device::queue_submit (the Rust binding for vkQueueSubmit) and that returns vk::Result::ERROR_DEVICE_LOST, it just returns it directly. Then it's up to wgpu_core to go about cleaning up the resources - and that will definitely entail continuing to use the hal device.

It looks to me like wgpu_core doesn't implement this cleanup, however. EDIT: Operations will continue to fail after device loss, and resources get cleaned up when the user drops the device.

[Wumpf]

All very good arguments why we should get this started with a validity flag. Alright, let's do it then and evolve it as we add lost device handling on HAL!

However, (digging a bit more into the code now) we should try a bit harder not to expose device_lose on the user facing wgpu layer as the spec doesn't include it. I'm also very skeptical of having it as a non public function on wgpu-core (wgpu-core is usually just an implementation of the public interface), but I can see how that might be useful in some places.
It seems pretty clear that destroy is what serves the actual purpose for the user. If the desire is to move fast on giving the user the ability to cause a device loss, we should instead expose destroy already and note down that its implementation is still work in progress - causing the device invalidation but no other sideeffects like resource destruction yet :)
Hope that makes sense!

[Wumpf]

nice! Thanks for adding all those verbose & helpful comments in there!