Skip to content

vk: Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMAT is supported#4655

Closed
sethdusek wants to merge 1 commit intogfx-rs:trunkfrom
sethdusek:trunk
Closed

vk: Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMAT is supported#4655
sethdusek wants to merge 1 commit intogfx-rs:trunkfrom
sethdusek:trunk

Conversation

@sethdusek
Copy link
Copy Markdown

@sethdusek sethdusek commented Nov 8, 2023

Checklist

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

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

Description
On Intel 6th generation GPUs, a simple compute shader that reads from a bgra8unorm texture and writes to a bgra8unorm texture does not work. Upon further inspection, writes to the texture were working but reading wasn't. After running it through renderdoc with validation layers I get the following error: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkCmdDispatch-OpTypeImage-07028

It seems my Intel GPU supports VK_FORMAT_FEATURE_2_STORAGE_WRITE_WITHOUT_FORMAT but does not support VK_FORMAT_FEATURE_2_STORAGE_READ_WITHOUT_FORMAT.

This PR only enables BGRA8UNORM_STORAGE if both of these are present.

However I do not understand why wgpu requires both WRITE_WITHOUT_FORMAT and READ_WITHOUT_FORMAT in the first place. I've tried specifying the format of the texture when creating the TextureView that gets passed to the compute shader but to no avail, as I get the same validation errors (and an empty screen since nothing is being read). It seems wgpu is ignoring these view formats somehow and thus requires both of these WITHOUT_FORMAT extensions?

Testing
I've tested this patch on an Intel HD Graphics 520. Now it does not support BGRA8UNORM_STORAGE, as it should, instead of spitting out validation errors later. It works as expected on any normal GPU that supports both these features.

thread 'main' panicked at 'called Result::unwrap() on an Err value: RequestDeviceError { inner: Core(UnsupportedFeature(Features(BGRA8UNORM_STORAGE))) }', src/wgpu_renderer.rs:167:14

@sethdusek sethdusek changed the title vulkan: Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMA… Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMAT is supported Nov 8, 2023
@sethdusek sethdusek changed the title Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMAT is supported vk: Only support bgra8unorm_storage if STORAGE_READ_WITHOUT_FORMAT is supported Nov 8, 2023
@sethdusek sethdusek marked this pull request as ready for review November 8, 2023 14:12
@sethdusek sethdusek requested a review from a team as a code owner November 8, 2023 14:12
@Wumpf Wumpf self-requested a review November 8, 2023 15:03
Wumpf
Wumpf previously approved these changes Nov 8, 2023
Copy link
Copy Markdown
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

However I do not understand why wgpu requires both WRITE_WITHOUT_FORMAT and READ_WITHOUT_FORMAT in the first place.

The feature flag enabled here corresponds to https://www.w3.org/TR/webgpu/#bgra8unorm-storage which requires both read & write storage access. So I think you fix here makes perfectly sense.
If we want to break this down, we'd have to add a wgpu-native only feature for this.

I've tried specifying the format of the texture when creating the TextureView that gets passed to the compute shader but to no avail, as I get the same validation errors (and an empty screen since nothing is being read). It seems wgpu is ignoring these view formats somehow and thus requires both of these WITHOUT_FORMAT extensions?

The only "conversion" a texture view can do right now is adding/removing srgb flag https://www.w3.org/TR/webgpu/#dom-gputexturedescriptor-viewformats
I'm not 100% sure on this part, but texture views aren't really relevant for storage textures at all, only about non-storage texture access, i.e. when you access via texture_2d instead of texture_storage_2d
(worth noting that texture_2d should always be supported for bgra8)

@Wumpf
Copy link
Copy Markdown
Member

Wumpf commented Nov 8, 2023

Can you add a changelog line? That's still a bugfix worth calling out :)

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 8, 2023

Can we hold off for a bit on merging this?

bgra8unorm-storage was at first only targeted at write-only storage textures.

The PR that added read-only and read-write textures to the spec updated the table based on the findings in gpuweb/gpuweb#3838 (comment).

But the situation of bgra8unorm-storage is different from all the other formats because we need to check for STORAGE_WRITE_WITHOUT_FORMAT/STORAGE_READ_WITHOUT_FORMAT rather than STORAGE_IMAGE.

I'll look some more into what would be best here and open a spec issue.

@Wumpf
Copy link
Copy Markdown
Member

Wumpf commented Nov 8, 2023

thanks for the context @teoxoy, had no idea ofc 😄

@Wumpf Wumpf requested review from Wumpf and removed request for Wumpf November 8, 2023 16:00
@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 9, 2023

Based on the data from https://vulkan.gpuinfo.org/ it seems that requiring STORAGE_READ_WITHOUT_FORMAT on top of STORAGE_WRITE_WITHOUT_FORMAT doesn't lose additional devices.

https://github.com/teoxoy/gpuinfo-vulkan-query/blob/87708bea8170b9fcf9dec0afb5f25c9e5b302f48/bgra8unorm-storage.txt#L1348

Which contradicts what @sethdusek experienced (Intel HD Graphics 520 supporting STORAGE_WRITE_WITHOUT_FORMAT but not STORAGE_READ_WITHOUT_FORMAT).

Not sure what to make of this.

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 9, 2023

However I do not understand why wgpu requires both WRITE_WITHOUT_FORMAT and READ_WITHOUT_FORMAT in the first place. I've tried specifying the format of the texture when creating the TextureView that gets passed to the compute shader but to no avail, as I get the same validation errors (and an empty screen since nothing is being read). It seems wgpu is ignoring these view formats somehow and thus requires both of these WITHOUT_FORMAT extensions?

We only require STORAGE_WRITE_WITHOUT_FORMAT/STORAGE_READ_WITHOUT_FORMAT for bgra8unorm because we must use SPIR-V's Unknown format (since there is no bgra8 format in SPIR-V).

For more background on this see: KhronosGroup/Vulkan-Docs#2027 (comment).

@sethdusek
Copy link
Copy Markdown
Author

sethdusek commented Nov 9, 2023

Which contradicts what @sethdusek experienced (Intel HD Graphics 520 supporting STORAGE_WRITE_WITHOUT_FORMAT but not STORAGE_READ_WITHOUT_FORMAT).
Not sure what to make of this.

From what I can tell reading ANV source code (Intel driver from Mesa), it only enables support for READ_WITHOUT_FORMAT if isl_format_supports_typed_reads is true for said format. isl_format_typed_reads and isl_format_typed_writes both read from a table which indicates which versions of Intel GPUs support reading/writing from this format

The table says that versions of Intel GPUs > 125 support "typed reads" for B8R8G8A8_UNORM but versions of Intel GPUs > 70 support typed writes (I'm not sure what exactly these version numbers correspond to among generations of Intel GPUs), but this indicates there are certain GPUs that can write to a B8G8R8A8_Unorm image but can't read from them. Which would explain why I don't get validation errors binding the image as write-only but only get it when binding it read-only.

Relevant files in mesa:

Output of vulkaninfo --show-formats: https://gist.github.com/SethDusek/a2ac08483670f7019db350ec1e51923d

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 10, 2023

That makes sense, thanks for looking into it and sharing those resources!

The vulkaninfo CLI also correctly (based on the driver src code) reports it https://gist.github.com/SethDusek/a2ac08483670f7019db350ec1e51923d#file-vulkaninfo-L2370.

The remaining mystery is why does the vulkaninfo DB only contain reports where either both or none of those capabilities show up.

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 10, 2023

Could you share the output of vulkaninfo --json?

@sethdusek
Copy link
Copy Markdown
Author

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 10, 2023

It turns out that the reports at https://vulkan.gpuinfo.org/ don't contain VkFormatFeatureFlagBits2 info SaschaWillems/VulkanCapsViewer#194.

I got tripped up by SaschaWillems/vulkan.gpuinfo.org#50 (comment).

Because bit 31 (STORAGE_READ_WITHOUT_FORMAT) was originally set, it was causing all other bits after it to also be set, which included bit 32 (STORAGE_WRITE_WITHOUT_FORMAT).

That's why we were seeing either both or none of them set.

That was quite the rabbit hole.

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 10, 2023

Given the current requirements for bgra8unorm with STORAGE_BINDING usage on underlying APIs:

Vulkan D3D12 FL11_0 Metal
write-only STORAGE_WRITE_WITHOUT_FORMAT "UAV Typed Store"
read-only STORAGE_READ_WITHOUT_FORMAT ✅ (via SRV)
read-write STORAGE_WRITE_WITHOUT_FORMAT & STORAGE_READ_WITHOUT_FORMAT "UAV Typed Store" & "UAV Typed Load"

I think the bgra8unorm-storage feature should only allow write-only access. Since read-write access is not possible on Metal and requiring STORAGE_READ_WITHOUT_FORMAT & "UAV Typed Load" might lower the availability of the feature on some hardware/drivers.

I'll open an issue on the spec to see what the others think.

We also need to add some more validation for this.

Copy link
Copy Markdown
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Requesting changes per above comments.

Please re-request a review from teo once the changes are addressed to make sure they see it!

@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Nov 16, 2023

bgra8unorm-storage will most likely only cover write-only access (gpuweb/gpuweb#4377) and the spec needs updating.

I filed #4704 to track the new changes to the spec (which should cover the validation I was mentioning before).

@teoxoy teoxoy closed this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants