diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 6078eaa530..a51b9ff1ce 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -54,6 +54,10 @@ namespace dawn_native { requiredUsage = wgpu::BufferUsage::Storage; maxBindingSize = std::numeric_limits::max(); break; + case kInternalStorageBufferBinding: + requiredUsage = kInternalStorageBuffer; + maxBindingSize = std::numeric_limits::max(); + break; case wgpu::BufferBindingType::Undefined: UNREACHABLE(); } diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index 0f322af91b..875d940438 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -85,9 +85,15 @@ namespace dawn_native { if (entry.buffer.type != wgpu::BufferBindingType::Undefined) { bindingMemberCount++; const BufferBindingLayout& buffer = entry.buffer; - DAWN_TRY(ValidateBufferBindingType(buffer.type)); - if (buffer.type == wgpu::BufferBindingType::Storage) { + // The kInternalStorageBufferBinding is used internally and not a value + // in wgpu::BufferBindingType. + if (buffer.type != kInternalStorageBufferBinding) { + DAWN_TRY(ValidateBufferBindingType(buffer.type)); + } + + if (buffer.type == wgpu::BufferBindingType::Storage || + buffer.type == kInternalStorageBufferBinding) { allowedStages &= ~wgpu::ShaderStage::Vertex; } @@ -96,6 +102,7 @@ namespace dawn_native { if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) && buffer.hasDynamicOffset && (buffer.type == wgpu::BufferBindingType::Storage || + buffer.type == kInternalStorageBufferBinding || buffer.type == wgpu::BufferBindingType::ReadOnlyStorage)) { return DAWN_VALIDATION_ERROR( "Dynamic storage buffers are disallowed because they aren't secure yet. " diff --git a/src/dawn_native/BindingInfo.cpp b/src/dawn_native/BindingInfo.cpp index e2facc48a2..4b6516f5eb 100644 --- a/src/dawn_native/BindingInfo.cpp +++ b/src/dawn_native/BindingInfo.cpp @@ -40,6 +40,7 @@ namespace dawn_native { break; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: case wgpu::BufferBindingType::ReadOnlyStorage: if (buffer.hasDynamicOffset) { ++bindingCounts->dynamicStorageBufferCount; diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index cdd9f05b59..56c6ea66a9 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -138,12 +138,11 @@ namespace dawn_native { mUsage |= kReadOnlyStorageBuffer; } - // TODO(crbug.com/dawn/434): This is just a workaround to make QueryResolve buffer pass the - // binding group validation when used as an internal resource. Instead the buffer made with - // QueryResolve usage would implicitly get StorageInternal usage which is only compatible - // with StorageBufferInternal binding type in BGL, not StorageBuffer binding type. + // The buffer made with QueryResolve usage implicitly get InternalStorage usage which is + // only compatible with InternalStorageBuffer binding type in BGL, not StorageBuffer binding + // type. if (mUsage & wgpu::BufferUsage::QueryResolve) { - mUsage |= wgpu::BufferUsage::Storage; + mUsage |= kInternalStorageBuffer; } } diff --git a/src/dawn_native/PassResourceUsageTracker.cpp b/src/dawn_native/PassResourceUsageTracker.cpp index 18fd319ed6..3aa90bf6b3 100644 --- a/src/dawn_native/PassResourceUsageTracker.cpp +++ b/src/dawn_native/PassResourceUsageTracker.cpp @@ -80,6 +80,9 @@ namespace dawn_native { case wgpu::BufferBindingType::Storage: BufferUsedAs(buffer, wgpu::BufferUsage::Storage); break; + case kInternalStorageBufferBinding: + BufferUsedAs(buffer, kInternalStorageBuffer); + break; case wgpu::BufferBindingType::ReadOnlyStorage: BufferUsedAs(buffer, kReadOnlyStorageBuffer); break; diff --git a/src/dawn_native/QueryHelper.cpp b/src/dawn_native/QueryHelper.cpp index 179db7b4df..ca989845af 100644 --- a/src/dawn_native/QueryHelper.cpp +++ b/src/dawn_native/QueryHelper.cpp @@ -121,10 +121,33 @@ namespace dawn_native { DAWN_TRY_ASSIGN(store->timestampCS, device->CreateShaderModule(&descriptor)); } + // Create binding group layout + std::array entries = {}; + for (uint32_t i = 0; i < entries.size(); i++) { + entries[i].binding = i; + entries[i].visibility = wgpu::ShaderStage::Compute; + } + entries[0].buffer.type = kInternalStorageBufferBinding; + entries[1].buffer.type = wgpu::BufferBindingType::ReadOnlyStorage; + entries[2].buffer.type = wgpu::BufferBindingType::Uniform; + + BindGroupLayoutDescriptor bglDesc; + bglDesc.entryCount = static_cast(entries.size()); + bglDesc.entries = entries.data(); + Ref bgl; + DAWN_TRY_ASSIGN(bgl, device->CreateBindGroupLayout(&bglDesc)); + + // Create pipeline layout + PipelineLayoutDescriptor plDesc; + plDesc.bindGroupLayoutCount = 1; + plDesc.bindGroupLayouts = &bgl.Get(); + Ref layout; + DAWN_TRY_ASSIGN(layout, device->CreatePipelineLayout(&plDesc)); + // Create ComputePipeline. ComputePipelineDescriptor computePipelineDesc = {}; // Generate the layout based on shader module. - computePipelineDesc.layout = nullptr; + computePipelineDesc.layout = layout.Get(); computePipelineDesc.compute.module = store->timestampCS.Get(); computePipelineDesc.compute.entryPoint = "main"; diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index 930fcf4acc..7a5e14991b 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -594,10 +594,14 @@ namespace dawn_native { // Binding mismatch between shader and bind group is invalid. For example, a // writable binding in the shader with a readonly storage buffer in the bind // group layout is invalid. However, a readonly binding in the shader with a - // writable storage buffer in the bind group layout is valid. + // writable storage buffer in the bind group layout is valid, a storage + // binding in the shader with an internal storage buffer in the bind group + // layout is also valid. bool validBindingConversion = - layoutInfo.buffer.type == wgpu::BufferBindingType::Storage && - shaderInfo.buffer.type == wgpu::BufferBindingType::ReadOnlyStorage; + (layoutInfo.buffer.type == wgpu::BufferBindingType::Storage && + shaderInfo.buffer.type == wgpu::BufferBindingType::ReadOnlyStorage) || + (layoutInfo.buffer.type == kInternalStorageBufferBinding && + shaderInfo.buffer.type == wgpu::BufferBindingType::Storage); if (layoutInfo.buffer.type != shaderInfo.buffer.type && !validBindingConversion) { diff --git a/src/dawn_native/d3d12/BindGroupD3D12.cpp b/src/dawn_native/d3d12/BindGroupD3D12.cpp index 2857d2df56..a9f27314e3 100644 --- a/src/dawn_native/d3d12/BindGroupD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupD3D12.cpp @@ -78,7 +78,8 @@ namespace dawn_native { namespace d3d12 { bindingOffsets[bindingIndex])); break; } - case wgpu::BufferBindingType::Storage: { + case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: { // Since SPIRV-Cross outputs HLSL shaders with RWByteAddressBuffer, // we must use D3D12_BUFFER_UAV_FLAG_RAW when making the // UNORDERED_ACCESS_VIEW_DESC. Using D3D12_BUFFER_UAV_FLAG_RAW requires diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index ce84faa6ab..28719de8b7 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp @@ -30,6 +30,7 @@ namespace dawn_native { namespace d3d12 { case wgpu::BufferBindingType::Uniform: return BindGroupLayout::DescriptorType::CBV; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: return BindGroupLayout::DescriptorType::UAV; case wgpu::BufferBindingType::ReadOnlyStorage: return BindGroupLayout::DescriptorType::SRV; diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 0223111b01..5f73b7e8b0 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -31,7 +31,7 @@ namespace dawn_native { namespace d3d12 { D3D12_RESOURCE_FLAGS D3D12ResourceFlags(wgpu::BufferUsage usage) { D3D12_RESOURCE_FLAGS flags = D3D12_RESOURCE_FLAG_NONE; - if (usage & wgpu::BufferUsage::Storage) { + if (usage & (wgpu::BufferUsage::Storage | kInternalStorageBuffer)) { flags |= D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS; } diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 29b3a98775..706ab523f2 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -392,6 +392,7 @@ namespace dawn_native { namespace d3d12 { } break; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: if (mInCompute) { commandList->SetComputeRootUnorderedAccessView(parameterIndex, bufferLocation); diff --git a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp index 913c845ff8..8565eb0df7 100644 --- a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp @@ -45,6 +45,7 @@ namespace dawn_native { namespace d3d12 { case wgpu::BufferBindingType::Uniform: return D3D12_ROOT_PARAMETER_TYPE_CBV; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: return D3D12_ROOT_PARAMETER_TYPE_UAV; case wgpu::BufferBindingType::ReadOnlyStorage: return D3D12_ROOT_PARAMETER_TYPE_SRV; diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index f7a5a917a4..ddf4cacb3c 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp @@ -229,10 +229,13 @@ namespace dawn_native { namespace d3d12 { // storage buffer in the BGL produces the wrong output. // Force read-only storage buffer bindings to be treated as UAV // instead of SRV. + // Internal storage buffer is a storage buffer used in the internal pipeline. const bool forceStorageBufferAsUAV = (bindingInfo.buffer.type == wgpu::BufferBindingType::ReadOnlyStorage && - bgl->GetBindingInfo(bindingIndex).buffer.type == - wgpu::BufferBindingType::Storage); + (bgl->GetBindingInfo(bindingIndex).buffer.type == + wgpu::BufferBindingType::Storage || + bgl->GetBindingInfo(bindingIndex).buffer.type == + kInternalStorageBufferBinding)); if (forceStorageBufferAsUAV) { accessControls.emplace(srcBindingPoint, tint::ast::Access::kReadWrite); } diff --git a/src/dawn_native/dawn_platform.h b/src/dawn_native/dawn_platform.h index 4a0ba09b5e..9e75de4469 100644 --- a/src/dawn_native/dawn_platform.h +++ b/src/dawn_native/dawn_platform.h @@ -35,6 +35,13 @@ namespace dawn_native { // This currently aliases wgpu::TextureUsage::Present, we would assign it // some bit when wgpu::TextureUsage::Present is removed. static constexpr wgpu::TextureUsage kPresentTextureUsage = wgpu::TextureUsage::Present; + + // Add an extra buffer usage and an extra binding type for binding the buffers with QueryResolve + // usage as storage buffer in the internal pipeline. + static constexpr wgpu::BufferUsage kInternalStorageBuffer = + static_cast(0x40000000); + static constexpr wgpu::BufferBindingType kInternalStorageBufferBinding = + static_cast(0xFFFFFFFF); } // namespace dawn_native #endif // DAWNNATIVE_DAWNPLATFORM_H_ diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index f28021e896..f7a59a0451 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -53,7 +53,8 @@ namespace dawn_native { namespace metal { // Metal validation layer requires the size of uniform buffer and storage buffer to be no // less than the size of the buffer block defined in shader, and the overall size of the // buffer must be aligned to the largest alignment of its members. - if (GetUsage() & (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage)) { + if (GetUsage() & + (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | kInternalStorageBuffer)) { if (currentSize > std::numeric_limits::max() - kMinUniformOrStorageBufferAlignment) { // Alignment would overlow. diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 085a8f015b..b4e98fe03c 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -265,6 +265,7 @@ namespace dawn_native { namespace opengl { target = GL_UNIFORM_BUFFER; break; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: case wgpu::BufferBindingType::ReadOnlyStorage: target = GL_SHADER_STORAGE_BUFFER; break; diff --git a/src/dawn_native/opengl/PipelineLayoutGL.cpp b/src/dawn_native/opengl/PipelineLayoutGL.cpp index ef2cf7c6ab..a1742a54b5 100644 --- a/src/dawn_native/opengl/PipelineLayoutGL.cpp +++ b/src/dawn_native/opengl/PipelineLayoutGL.cpp @@ -43,6 +43,7 @@ namespace dawn_native { namespace opengl { uboIndex++; break; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: case wgpu::BufferBindingType::ReadOnlyStorage: mIndexInfo[group][bindingIndex] = ssboIndex; ssboIndex++; diff --git a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp index 6ee1d49275..1f8514d692 100644 --- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp +++ b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp @@ -56,6 +56,7 @@ namespace dawn_native { namespace vulkan { } return VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER; case wgpu::BufferBindingType::Storage: + case kInternalStorageBufferBinding: case wgpu::BufferBindingType::ReadOnlyStorage: if (bindingInfo.buffer.hasDynamicOffset) { return VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC; diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index efe26c6b7f..15d43a5127 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -74,7 +74,7 @@ namespace dawn_native { namespace vulkan { flags |= VK_PIPELINE_STAGE_VERTEX_INPUT_BIT; } if (usage & (wgpu::BufferUsage::Uniform | wgpu::BufferUsage::Storage | - kReadOnlyStorageBuffer)) { + kInternalStorageBuffer | kReadOnlyStorageBuffer)) { flags |= VK_PIPELINE_STAGE_VERTEX_SHADER_BIT | VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; @@ -113,7 +113,7 @@ namespace dawn_native { namespace vulkan { if (usage & wgpu::BufferUsage::Uniform) { flags |= VK_ACCESS_UNIFORM_READ_BIT; } - if (usage & wgpu::BufferUsage::Storage) { + if (usage & (wgpu::BufferUsage::Storage | kInternalStorageBuffer)) { flags |= VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT; } if (usage & kReadOnlyStorageBuffer) { diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index 69d890f7b7..759b2659c2 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -544,8 +544,7 @@ TEST_F(BindGroupValidationTest, BufferUsageReadonlySSBO) { } // Check that a resolve buffer with internal storge usage cannot be used as SSBO -// TODO(hao.x.li@intel.com): Disable until internal storage usage is implemented -TEST_F(BindGroupValidationTest, DISABLED_BufferUsageQueryResolve) { +TEST_F(BindGroupValidationTest, BufferUsageQueryResolve) { wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout( device, {{0, wgpu::ShaderStage::Fragment, wgpu::BufferBindingType::Storage}}); diff --git a/src/tests/white_box/QueryInternalShaderTests.cpp b/src/tests/white_box/QueryInternalShaderTests.cpp index 56e88f9349..4930f1e378 100644 --- a/src/tests/white_box/QueryInternalShaderTests.cpp +++ b/src/tests/white_box/QueryInternalShaderTests.cpp @@ -122,7 +122,7 @@ TEST_P(QueryInternalShaderTests, TimestampComputeShader) { wgpu::BufferDescriptor timestampsDesc; timestampsDesc.size = kTimestampCount * sizeof(uint64_t); timestampsDesc.usage = - wgpu::BufferUsage::Storage | wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst; + wgpu::BufferUsage::QueryResolve | wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst; wgpu::Buffer timestampsBuffer = device.CreateBuffer(×tampsDesc); auto PrepareExpectedResults = [&](uint32_t first, uint32_t count,