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

Narrow down the feature sets for resource indexing #1515

Closed
kvark opened this issue Jun 18, 2021 · 9 comments
Closed

Narrow down the feature sets for resource indexing #1515

kvark opened this issue Jun 18, 2021 · 9 comments
Labels
area: api Issues related to API surface help required We need community help to make this happen. type: enhancement New feature or request

Comments

@kvark
Copy link
Member

kvark commented Jun 18, 2021

Is your feature request related to a problem? Please describe.
We have separate features for uniform buffers, storage buffers, sampled textures, storage textures, each 3 times for different indexing modes. See #1511

Describe the solution you'd like
We need to reduce this to a smaller feature set. For example, once feature for buffers, one for textures, one for buffer dynamic indexing, one for texture dynamic indexing.

Describe alternatives you've considered
there can be many schemes to split this thing

Additional context

@kvark kvark added type: enhancement New feature or request help required We need community help to make this happen. area: api Issues related to API surface labels Jun 18, 2021
@ElectronicRU
Copy link
Contributor

Unfortunately, as we've discussed with @cwfitzgerald that is not really feasible: there are platforms which support a subset of those features (ie arrays for sampled but not storage textures), both on Vulkan and on Metal. The mess in the feature flags only reflects the mess in the device support.

@ElectronicRU
Copy link
Contributor

Oh and also unless I'm mistaken, the features for storage buffers are currently not there, but yeah it's a matter of time :(

@cwfitzgerald
Copy link
Member

I suspect we will need to put the DI flags in another location and have have a broader "has a concept of DI" flag here. We may also be able to take a careful study of the ecosystem and group flags into sets that are often shared. We can probably get it down to 3-5 this way.

@kvark
Copy link
Member Author

kvark commented Jun 18, 2021

there are platforms which support a subset of those features (ie arrays for sampled but not storage textures), both on Vulkan and on Metal. The mess in the feature flags only reflects the mess in the device support.

If the fraction of "lost" features is relatively small, we might still want to do this. There is a cost to feature proliferation. It's documentation, testing, maintenance, other cots.

For example, if I search for "DynamicIndexing" features on https://vulkan.gpuinfo.org, it seems rather strongly separated in 2 groups:

  1. uniform buffer + sampled texture (96% android and full desktop)
  2. storage buffer + storage texture (75% on android and full desktop)

For this group, my suggestion would be to leave only 1 feature bit: indexing storage resources. Then make it so the whole "resource array" feature implies that uniform resources can be dynamically indexed.

I'm sure we can apply a similar logic to the other features related to resource arrays.

@ElectronicRU
Copy link
Contributor

there are platforms which support a subset of those features (ie arrays for sampled but not storage textures), both on Vulkan and on Metal. The mess in the feature flags only reflects the mess in the device support.

If the fraction of "lost" features is relatively small, we might still want to do this. There is a cost to feature proliferation. It's documentation, testing, maintenance, other cots.

For example, if I search for "DynamicIndexing" features on https://vulkan.gpuinfo.org, it seems rather strongly separated in 2 groups:

  1. uniform buffer + sampled texture (96% android and full desktop)
  2. storage buffer + storage texture (75% on android and full desktop)

For this group, my suggestion would be to leave only 1 feature bit: indexing storage resources. Then make it so the whole "resource array" feature implies that uniform resources can be dynamically indexed.

I'm sure we can apply a similar logic to the other features related to resource arrays.

Yeah, "arrays of readonly things" and "arrays of writable things" seems to be a much better divide than buffers/textures.
We could add combined flags for now and deprecate the separate ones, then?

@cwfitzgerald
Copy link
Member

We could add combined flags for now and deprecate the separate ones, then?

Every wgpu release is breaking, just yeet the old ones.

@ElectronicRU
Copy link
Contributor

Oh and also strongly agree with uniting dynamic indexing with arrays - with WGPU workflow, since there's no like specialization constants or header defines, const-indexed arrays are not much different from binding the things separately.

My proposal would be, then:

  • uniform buffer and sampled texture arrays ("the OpenGL bindless land", would be a union of those two + dynamic indexing)
  • storage buffer and storage texture arrays ("storage resource arrays", again those two + dynamic indexing)
  • nonuniform indexing (supported iff for all supported features, nonuniform indexing is also supported).

The only kinky thing I see here is Metal, as there's no support for uniform or storage buffer arrays in MSL, but there is for textures. Could also add a crisscross here, I suppose - add flags for "arrays of buffers" and "arrays of textures" too. Total of 5 flags. (Maybe also add arrays of samplers while we're at it, samplers are also a readonly thing and they fall within the same margin as uniforms and textures iirc)

@kvark
Copy link
Member Author

kvark commented Jun 20, 2021

with WGPU workflow, since there's no like specialization constants or header defines

WebGPU does have specialization constants now, and we need to catch up and implement them

bors bot added a commit that referenced this issue Jun 28, 2021
1522: Reduce feature flag surface for descriptor arrays. r=cwfitzgerald a=ElectronicRU

DYNAMIC_INDEXING is checked by default, since WGPU doesn't allow
for any kind of specialization constants and constant indexing is
nigh useless.

STORAGE_RESOURCE_BINDING_ARRAY is enabled iff the device supports:
- both or neither of uniform and buffer arrays with dynamic indexing;
- both or neither of sampled and storage images with dynamic indexing.

NONUNIFORM_INDEXING is enabled iff for ALL types of descriptor arrays
*that are supported for dynamic indexing*, nonuniform indexing is
also allowed.

These flags have a limitation in that some platforms
(eg.
https://vulkan.gpuinfo.org/displayreport.php?id=11692#features_core_12)
may support a strange subset of those features (example device
supporting non-uniform indexing for textures and storage buffers,
but NOT for storage textures or uniform buffers). In that case feature
will be reported as missing, even though it is partially present.

In the author's opinion, this flag set is convenient for the
users to query; however, due to aforementioned limitations, it would be
desirable to obtain statistics about "edge-case" platforms and what
percentage of reports they comprise.

**Connections**
Implementation of proposal outlined in #1515. 


Co-authored-by: Alex S <[email protected]>
@kvark
Copy link
Member Author

kvark commented Jul 16, 2021

I think we can consider this done by #1522

@kvark kvark closed this as completed Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface help required We need community help to make this happen. type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants