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.

Sign up for GitHub

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

Expose adapter specific formats features via Extension, support for storage read+write #1112

Merged
merged 6 commits into from
Jan 6, 2021
Merged

Expose adapter specific formats features via Extension, support for storage read+write #1112

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ccad9a
wip adapter specific format features & rw storage
Wumpf Dec 26, 2020
4207e57
validation against adapter specific format features
Wumpf Dec 30, 2020
c17b2dc
Expose adapter_get_texture_format_features
Wumpf Dec 30, 2020
757d12a
map depth/stencil feature to RENDER_ATTACHMENT
Wumpf Dec 30, 2020
e29d17a
wgpu format features are again part of TextureFormatInfo
Wumpf Jan 5, 2021
30c96bf
Added note on why hal update needed for STORAGE_READ_WRITE is blocked…
Wumpf Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions wgpu-core/src/binding_model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ pub enum CreateBindGroupError {
WrongSamplerComparison,
#[error("bound texture views can not have both depth and stencil aspects enabled")]
DepthStencilAspect,
#[error("the adapter does not support simultaneous read + write storage texture access for the format {0:?}")]
StorageReadWriteNotSupported(wgt::TextureFormat),
}

#[derive(Clone, Debug, Error)]
Expand Down
54 changes: 42 additions & 12 deletions wgpu-core/src/device/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,15 +516,16 @@ impl<B: GfxBackend> Device<B> {
fn create_texture(
&self,
self_id: id::DeviceId,
adapter: &crate::instance::Adapter<B>,
desc: &resource::TextureDescriptor,
) -> Result<resource::Texture<B>, resource::CreateTextureError> {
debug_assert_eq!(self_id.backend(), B::VARIANT);

let format_desc = desc.format.describe();
let features = format_desc.features;
if !self.features.contains(features) {
let required_features = format_desc.required_features;
if !self.features.contains(required_features) {
return Err(resource::CreateTextureError::MissingFeature(
features,
required_features,
desc.format,
));
}
Expand All @@ -546,10 +547,19 @@ impl<B: GfxBackend> Device<B> {
return Err(resource::CreateTextureError::EmptyUsage);
}

let missing_features = desc.usage - format_desc.allowed_usages;
if !missing_features.is_empty() {
let format_features = if self
.features
.contains(wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES)
{
adapter.get_texture_format_features(desc.format)
} else {
format_desc.guaranteed_format_features
};

let missing_allowed_usages = desc.usage - format_features.allowed_usages;
if !missing_allowed_usages.is_empty() {
return Err(resource::CreateTextureError::InvalidUsages(
missing_features,
missing_allowed_usages,
desc.format,
));
}
Expand Down Expand Up @@ -618,6 +628,7 @@ impl<B: GfxBackend> Device<B> {
dimension: desc.dimension,
kind,
format: desc.format,
format_features,
full_range: TextureSelector {
levels: 0..desc.mip_level_count as hal::image::Level,
layers: 0..kind.num_layers(),
Expand Down Expand Up @@ -774,6 +785,7 @@ impl<B: GfxBackend> Device<B> {
},
aspects,
format: texture.format,
format_features: texture.format_features,
extent: texture.kind.extent().at_level(desc.base_mip_level as _),
samples: texture.kind.num_samples(),
selector,
Expand Down Expand Up @@ -1301,17 +1313,33 @@ impl<B: GfxBackend> Device<B> {
wgt::BindingType::Texture { .. } => {
(wgt::TextureUsage::SAMPLED, resource::TextureUse::SAMPLED)
}
wgt::BindingType::StorageTexture { access, .. } => (
wgt::TextureUsage::STORAGE,
match access {
wgt::BindingType::StorageTexture { access, .. } => {
let internal_use = match access {
wgt::StorageTextureAccess::ReadOnly => {
resource::TextureUse::STORAGE_LOAD
}
wgt::StorageTextureAccess::WriteOnly => {
resource::TextureUse::STORAGE_STORE
}
},
),
wgt::StorageTextureAccess::ReadWrite => {
if !view.format_features.flags.contains(
wgt::TextureFormatFeatureFlags::STORAGE_READ_WRITE,
) {
return Err(if self.features.contains(
wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES,
) {
Error::StorageReadWriteNotSupported(view.format)
} else {
Error::MissingFeatures(wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES)
});
}

resource::TextureUse::STORAGE_STORE
| resource::TextureUse::STORAGE_LOAD
}
};
(wgt::TextureUsage::STORAGE, internal_use)
}
_ => return Err(Error::WrongBindingType {
binding,
actual: decl.ty.clone(),
Expand Down Expand Up @@ -2692,13 +2720,15 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
let hub = B::hub(self);
let mut token = Token::root();

let (adapter_guard, mut token) = hub.adapters.read(&mut token);
let (device_guard, mut token) = hub.devices.read(&mut token);
let error = loop {
let device = match device_guard.get(device_id) {
Ok(device) => device,
Err(_) => break DeviceError::Invalid.into(),
};
let texture = match device.create_texture(device_id, desc) {
let adapter = &adapter_guard[device.adapter_id.value];
let texture = match device.create_texture(device_id, adapter, desc) {
Ok(texture) => texture,
Err(error) => break error,
};
Expand Down
96 changes: 83 additions & 13 deletions wgpu-core/src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ impl crate::hub::Resource for Surface {
pub struct Adapter<B: hal::Backend> {
pub(crate) raw: hal::adapter::Adapter<B>,
features: wgt::Features,
private_features: PrivateFeatures,
limits: wgt::Limits,
life_guard: LifeGuard,
}
Expand All @@ -132,7 +133,8 @@ impl<B: GfxBackend> Adapter<B> {

let mut features = wgt::Features::default()
| wgt::Features::MAPPABLE_PRIMARY_BUFFERS
| wgt::Features::PUSH_CONSTANTS;
| wgt::Features::PUSH_CONSTANTS
| wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we actually need this feature. I.e. can we pretend it's always supported? On the Web, we can just return the statically known features (until WebGPU gets a proper extension).

Copy link
Member Author

Choose a reason for hiding this comment

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

That thought crossed my mind as well but what kept me from it is

  • (most important) one might develop a cross platform application (e.g. web + desktop) and unknowingly break its portability if this is not an explicit opt-in. Imho a huge selling point of wgpu(-rs) is that it formulates a feature set that will work on a very wide variety of devices
  • other things in the limits struct need to be requested explicitly as well and default to the webgpu specification (e.g. how many storage bindings are supported)
  • if and when WebGPU gets an extension this may fit more nicely as a "free-for-all tier"

Copy link
Member

Choose a reason for hiding this comment

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

That's a very good point! Ideally I'd love to make it so you can't use a feature that you haven't either discovered or requested. This should apply to format features as well. So what you are saying makes total sense, and is much appreciated!

We can also go a slightly different route and consider the format features to be "basic" unless the user asks for them. This would make the same code portable across the web and non-web.

Copy link
Member Author

Choose a reason for hiding this comment

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

consider the format features to be "basic" unless the user asks for them

You're thinking that the user can request particular texture features ahead of time? That would make sense in regard to how the limit struct works and keeps all the missing-feature surprises to the device selection rather than some random texture binding.

Or did you mean something else? :)
Otherwise, the way it's done here would already be a manual opt-in by the user, only that it's a bit vague what one ends up with :/

Copy link
Member

Choose a reason for hiding this comment

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

No, I wasn't think about "ahead of time" kind of thing as we do for limits. I was thinking more simple: if the user didn't call get_texture_features (or whatever it's called), then they can only assume the base guaranteed feature set for this format specifically (and we validate that).

features.set(
wgt::Features::DEPTH_CLAMPING,
adapter_features.contains(hal::Features::DEPTH_CLAMP),
Expand Down Expand Up @@ -181,6 +183,20 @@ impl<B: GfxBackend> Adapter<B> {
//TODO: https://github.com/gfx-rs/gfx/issues/3346
features.set(wgt::Features::ADDRESS_MODE_CLAMP_TO_BORDER, true);

let private_features = PrivateFeatures {
anisotropic_filtering: adapter_features.contains(hal::Features::SAMPLER_ANISOTROPY),
texture_d24: raw
.physical_device
.format_properties(Some(hal::format::Format::X8D24Unorm))
.optimal_tiling
.contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT),
texture_d24_s8: raw
.physical_device
.format_properties(Some(hal::format::Format::D24UnormS8Uint))
.optimal_tiling
.contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT),
};

let adapter_limits = raw.physical_device.limits();

let default_limits = wgt::Limits::default();
Expand Down Expand Up @@ -228,11 +244,60 @@ impl<B: GfxBackend> Adapter<B> {
Self {
raw,
features,
private_features,
limits,
life_guard: LifeGuard::new("<Adapter>"),
}
}

pub(crate) fn get_texture_format_features(
&self,
format: wgt::TextureFormat,
) -> wgt::TextureFormatFeatures {
let texture_format_properties = self
.raw
.physical_device
.format_properties(Some(conv::map_texture_format(
format,
self.private_features,
)))
.optimal_tiling;

let mut allowed_usages = wgt::TextureUsage::empty();
if texture_format_properties.contains(hal::format::ImageFeature::SAMPLED) {
allowed_usages |= wgt::TextureUsage::SAMPLED;
}
if texture_format_properties.contains(hal::format::ImageFeature::STORAGE) {
allowed_usages |= wgt::TextureUsage::STORAGE;
}
if texture_format_properties.contains(hal::format::ImageFeature::COLOR_ATTACHMENT) {
allowed_usages |= wgt::TextureUsage::RENDER_ATTACHMENT;
}
if texture_format_properties.contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT) {
allowed_usages |= wgt::TextureUsage::RENDER_ATTACHMENT;
}
if texture_format_properties.contains(hal::format::ImageFeature::BLIT_SRC) {
allowed_usages |= wgt::TextureUsage::COPY_SRC;
}
if texture_format_properties.contains(hal::format::ImageFeature::BLIT_DST) {
allowed_usages |= wgt::TextureUsage::COPY_DST;
}

let mut flags = wgt::TextureFormatFeatureFlags::empty();
if texture_format_properties.contains(hal::format::ImageFeature::STORAGE_ATOMIC) {
flags |= wgt::TextureFormatFeatureFlags::STORAGE_ATOMICS;
}
// TODO: hal update
//if texture_format_properties.contains(hal::format::ImageFeature::STORAGE_READ_WRITE) {
Copy link
Member

Choose a reason for hiding this comment

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

can now be implemented thanks to you!

Copy link
Member Author

Choose a reason for hiding this comment

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

😊
once we managed to upgrade to newest gfx that is :)

// flags |= wgt::TextureFormatFeatureFlags::STORAGE_READ_WRITE;
//}

wgt::TextureFormatFeatures {
allowed_usages,
flags,
}
}

fn create_device(
&self,
self_id: AdapterId,
Expand Down Expand Up @@ -364,17 +429,6 @@ impl<B: GfxBackend> Adapter<B> {
}

let mem_props = phd.memory_properties();
let private_features = PrivateFeatures {
anisotropic_filtering: enabled_features.contains(hal::Features::SAMPLER_ANISOTROPY),
texture_d24: phd
.format_properties(Some(hal::format::Format::X8D24Unorm))
.optimal_tiling
.contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT),
texture_d24_s8: phd
.format_properties(Some(hal::format::Format::D24UnormS8Uint))
.optimal_tiling
.contains(hal::format::ImageFeature::DEPTH_STENCIL_ATTACHMENT),
};

Device::new(
gpu.device,
Expand All @@ -385,7 +439,7 @@ impl<B: GfxBackend> Adapter<B> {
gpu.queue_groups.swap_remove(0),
mem_props,
limits,
private_features,
self.private_features,
desc,
trace_path,
)
Expand Down Expand Up @@ -708,6 +762,22 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.map_err(|_| InvalidAdapter)
}

pub fn adapter_get_texture_format_features<B: GfxBackend>(
&self,
adapter_id: AdapterId,
format: wgt::TextureFormat,
) -> Result<wgt::TextureFormatFeatures, InvalidAdapter> {
span!(_guard, INFO, "Adapter::get_texture_format_features");

let hub = B::hub(self);
let mut token = Token::root();
let (adapter_guard, _) = hub.adapters.read(&mut token);
adapter_guard
.get(adapter_id)
.map(|adapter| adapter.get_texture_format_features(format))
.map_err(|_| InvalidAdapter)
}

pub fn adapter_features<B: GfxBackend>(
&self,
adapter_id: AdapterId,
Expand Down
2 changes: 2 additions & 0 deletions wgpu-core/src/resource.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ pub struct Texture<B: hal::Backend> {
pub(crate) dimension: wgt::TextureDimension,
pub(crate) kind: hal::image::Kind,
pub(crate) format: wgt::TextureFormat,
pub(crate) format_features: wgt::TextureFormatFeatures,
pub(crate) full_range: TextureSelector,
pub(crate) life_guard: LifeGuard,
}
Expand Down Expand Up @@ -305,6 +306,7 @@ pub struct TextureView<B: hal::Backend> {
//TODO: store device_id for quick access?
pub(crate) aspects: hal::format::Aspects,
pub(crate) format: wgt::TextureFormat,
pub(crate) format_features: wgt::TextureFormatFeatures,
pub(crate) extent: hal::image::Extent,
pub(crate) samples: hal::image::NumSamples,
pub(crate) selector: TextureSelector,
Expand Down
4 changes: 4 additions & 0 deletions wgpu-core/src/swap_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
},
aspects: hal::format::Aspects::COLOR,
format: sc.desc.format,
format_features: wgt::TextureFormatFeatures {
allowed_usages: wgt::TextureUsage::RENDER_ATTACHMENT,
flags: wgt::TextureFormatFeatureFlags::empty(),
},
extent: hal::image::Extent {
width: sc.desc.width,
height: sc.desc.height,
Expand Down
1 change: 1 addition & 0 deletions wgpu-core/src/validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ impl Resource {
let usage = match access {
wgt::StorageTextureAccess::ReadOnly => naga::GlobalUse::LOAD,
wgt::StorageTextureAccess::WriteOnly => naga::GlobalUse::STORE,
wgt::StorageTextureAccess::ReadWrite => naga::GlobalUse::all(),
};
(naga::ImageClass::Storage(naga_format), usage)
}
Expand Down
Loading