From ca41b006917e80081d1392fa53a08dd20cf93e91 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Mon, 7 Jun 2021 18:23:52 +0000 Subject: [PATCH] Triage Dawn TODOs Change-Id: Ia049d5a03d0e251531f71def525492403588fd74 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/53460 Commit-Queue: Austin Eng Reviewed-by: Corentin Wallez --- src/dawn_native/BUILD.gn | 2 +- .../CopyTextureForBrowserHelper.cpp | 6 ++--- src/dawn_native/RingBufferAllocator.cpp | 1 - src/dawn_native/Texture.h | 2 +- .../d3d12/BindGroupLayoutD3D12.cpp | 1 - src/dawn_native/d3d12/BufferD3D12.cpp | 4 +-- src/dawn_native/d3d12/CommandBufferD3D12.cpp | 8 +----- src/dawn_native/d3d12/HeapAllocatorD3D12.cpp | 2 +- .../d3d12/ResourceAllocatorManagerD3D12.cpp | 2 +- src/dawn_native/d3d12/SwapChainD3D12.cpp | 4 +-- src/dawn_native/d3d12/TextureD3D12.cpp | 4 +-- src/dawn_native/metal/BufferMTL.mm | 2 -- src/dawn_native/metal/CommandBufferMTL.mm | 6 ++--- src/dawn_native/metal/DeviceMTL.mm | 4 +-- src/dawn_native/metal/SwapChainMTL.mm | 2 +- src/dawn_native/null/DeviceNull.cpp | 2 +- src/dawn_native/opengl/BufferGL.cpp | 6 +---- src/dawn_native/opengl/CommandBufferGL.cpp | 8 ++---- src/dawn_native/opengl/TextureGL.cpp | 4 +-- src/dawn_native/vulkan/BindGroupVk.cpp | 2 +- src/dawn_native/vulkan/BufferVk.cpp | 8 +++--- src/dawn_native/vulkan/BufferVk.h | 2 +- src/dawn_native/vulkan/CommandBufferVk.cpp | 4 +-- src/dawn_native/vulkan/DeviceVk.cpp | 2 +- .../vulkan/NativeSwapChainImplVk.cpp | 6 ++--- src/dawn_native/vulkan/PipelineLayoutVk.cpp | 2 +- src/dawn_native/vulkan/RenderPipelineVk.cpp | 1 - .../vulkan/ResourceMemoryAllocatorVk.cpp | 6 ++--- src/dawn_native/vulkan/SwapChainVk.cpp | 26 +++++++++---------- src/dawn_native/vulkan/TextureVk.cpp | 8 +++--- src/dawn_native/vulkan/TextureVk.h | 14 +++++----- src/utils/TerribleCommandBuffer.cpp | 4 +-- src/utils/TestUtils.cpp | 2 +- 33 files changed, 65 insertions(+), 92 deletions(-) diff --git a/src/dawn_native/BUILD.gn b/src/dawn_native/BUILD.gn index 982e80daec..e4aad1db41 100644 --- a/src/dawn_native/BUILD.gn +++ b/src/dawn_native/BUILD.gn @@ -63,7 +63,7 @@ config("dawn_native_internal") { # aren't on 10.11 it means we are on 10.11 and above, and Metal is available. # Skipping this check on 10.11 and above is important as it allows getting # proper compilation warning when using 10.12 and above feature for example. - # TODO(cwallez@chromium.org): Consider using API_AVAILABLE annotations on all + # TODO(crbug.com/1004024): Consider using API_AVAILABLE annotations on all # metal code in dawn once crbug.com/1004024 is sorted out if Chromium still # supports 10.10 then. if (is_mac && mac_deployment_target == "10.10.0") { diff --git a/src/dawn_native/CopyTextureForBrowserHelper.cpp b/src/dawn_native/CopyTextureForBrowserHelper.cpp index 50248462a4..6f4510104e 100644 --- a/src/dawn_native/CopyTextureForBrowserHelper.cpp +++ b/src/dawn_native/CopyTextureForBrowserHelper.cpp @@ -32,7 +32,7 @@ namespace dawn_native { namespace { - // TODO(shaobo.yan@intel.com) : Support premultiplay-alpha + // TODO(crbug.com/dawn/856) : Support premultiply-alpha static const char sCopyTextureForBrowserVertex[] = R"( [[block]] struct Uniforms { u_scale : vec2; @@ -101,7 +101,7 @@ namespace dawn_native { } )"; - // TODO(shaobo.yan@intel.com): Expand copyTextureForBrowser to support any + // TODO(crbug.com/dawn/856): Expand copyTextureForBrowser to support any // non-depth, non-stencil, non-compressed texture format pair copy. Now this API // supports CopyImageBitmapToTexture normal format pairs. MaybeError ValidateCopyTextureFormatConversion(const wgpu::TextureFormat srcFormat, @@ -281,7 +281,7 @@ namespace dawn_native { const ImageCopyTexture* destination, const Extent3D* copySize, const CopyTextureForBrowserOptions* options) { - // TODO(shaobo.yan@intel.com): In D3D12 and Vulkan, compatible texture format can directly + // TODO(crbug.com/dawn/856): In D3D12 and Vulkan, compatible texture format can directly // copy to each other. This can be a potential fast path. // Noop copy diff --git a/src/dawn_native/RingBufferAllocator.cpp b/src/dawn_native/RingBufferAllocator.cpp index 12fbe59026..c77f0a58ee 100644 --- a/src/dawn_native/RingBufferAllocator.cpp +++ b/src/dawn_native/RingBufferAllocator.cpp @@ -26,7 +26,6 @@ // only two indices that keep increasing (unbounded) but can be still indexed using bit masks. // However, this 1) requires the size to always be a power-of-two and 2) remove tests that check // used bytes. -// TODO(bryan.bernhart@intel.com): Follow-up with ringbuffer optimization. namespace dawn_native { RingBufferAllocator::RingBufferAllocator(uint64_t maxSize) : mMaxBlockSize(maxSize) { diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h index 43076efb47..d981acc5bf 100644 --- a/src/dawn_native/Texture.h +++ b/src/dawn_native/Texture.h @@ -106,7 +106,7 @@ namespace dawn_native { wgpu::TextureUsage mUsage = wgpu::TextureUsage::None; TextureState mState; - // TODO(natlee@microsoft.com): Use a more optimized data structure to save space + // TODO(crbug.com/dawn/845): Use a more optimized data structure to save space std::vector mIsSubresourceContentInitializedAtIndex; }; diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index 8557b44ce1..ce84faa6ab 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp @@ -137,7 +137,6 @@ namespace dawn_native { namespace d3d12 { continue; } - // TODO(shaobo.yan@intel.com): Implement dynamic buffer offset. // TODO(dawn:728) In the future, special handling will be needed here for external // textures because they encompass multiple views. DescriptorType descriptorType = WGPUBindingInfoToDescriptorType(bindingInfo); diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index b5e17f7780..0223111b01 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -422,7 +422,7 @@ namespace dawn_native { namespace d3d12 { ASSERT(GetDevice()->IsToggleEnabled(Toggle::LazyClearResourceOnFirstUse)); ASSERT(!IsDataInitialized()); - // TODO(jiawei.shao@intel.com): skip initializing the buffer when it is created on a heap + // TODO(crbug.com/dawn/484): skip initializing the buffer when it is created on a heap // that has already been zero initialized. DAWN_TRY(ClearBuffer(commandContext, uint8_t(0u))); SetIsDataInitialized(); @@ -441,7 +441,7 @@ namespace dawn_native { namespace d3d12 { memset(mMappedData, clearValue, GetSize()); UnmapImpl(); } else { - // TODO(jiawei.shao@intel.com): use ClearUnorderedAccessView*() when the buffer usage + // TODO(crbug.com/dawn/852): use ClearUnorderedAccessView*() when the buffer usage // includes STORAGE. DynamicUploader* uploader = device->GetDynamicUploader(); UploadHandle uploadHandle; diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 107e70e898..a2dca9ca00 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -208,7 +208,7 @@ namespace dawn_native { namespace d3d12 { for (size_t i = 0; i < usages.buffers.size(); ++i) { Buffer* buffer = ToBackend(usages.buffers[i]); - // TODO(jiawei.shao@intel.com): clear storage buffers with + // TODO(crbug.com/dawn/852): clear storage buffers with // ClearUnorderedAccessView*(). buffer->GetDevice()->ConsumedError(buffer->EnsureDataInitialized(commandContext)); @@ -285,7 +285,6 @@ namespace dawn_native { namespace d3d12 { // the signal to change the bounded heaps. // Re-populating all bindgroups after the last one fails causes duplicated allocations // to occur on overflow. - // TODO(bryan.bernhart@intel.com): Consider further optimization. bool didCreateBindGroupViews = true; bool didCreateBindGroupSamplers = true; for (BindGroupIndex index : IterateBitSet(mDirtyBindGroups)) { @@ -793,9 +792,6 @@ namespace dawn_native { namespace d3d12 { cmd->firstQuery, cmd->queryCount, destination->GetD3D12Resource(), cmd->destinationOffset); - // TODO(hao.x.li@intel.com): Add compute shader to convert the query result - // (ticks) to timestamp (ns) - break; } @@ -1109,8 +1105,6 @@ namespace dawn_native { namespace d3d12 { ->StencilBeginningAccess.Clear.ClearValue.DepthStencil.Stencil; } - // TODO(kainino@chromium.org): investigate: should the Dawn clear - // stencil type be uint8_t? if (clearFlags) { commandList->ClearDepthStencilView( renderPassBuilder->GetRenderPassDepthStencilDescriptor()->cpuDescriptor, diff --git a/src/dawn_native/d3d12/HeapAllocatorD3D12.cpp b/src/dawn_native/d3d12/HeapAllocatorD3D12.cpp index 756c9071cf..2c9f3778e0 100644 --- a/src/dawn_native/d3d12/HeapAllocatorD3D12.cpp +++ b/src/dawn_native/d3d12/HeapAllocatorD3D12.cpp @@ -42,7 +42,7 @@ namespace dawn_native { namespace d3d12 { // It is preferred to use a size that is a multiple of the alignment. // However, MSAA heaps are always aligned to 4MB instead of 64KB. This means // if the heap size is too small, the VMM would fragment. - // TODO(bryan.bernhart@intel.com): Consider having MSAA vs non-MSAA heaps. + // TODO(crbug.com/dawn/849): Consider having MSAA vs non-MSAA heaps. heapDesc.Alignment = D3D12_DEFAULT_MSAA_RESOURCE_PLACEMENT_ALIGNMENT; heapDesc.Flags = mHeapFlags; diff --git a/src/dawn_native/d3d12/ResourceAllocatorManagerD3D12.cpp b/src/dawn_native/d3d12/ResourceAllocatorManagerD3D12.cpp index a9010b691b..166d59f817 100644 --- a/src/dawn_native/d3d12/ResourceAllocatorManagerD3D12.cpp +++ b/src/dawn_native/d3d12/ResourceAllocatorManagerD3D12.cpp @@ -194,7 +194,7 @@ namespace dawn_native { namespace d3d12 { optimizedClearValue = &zero; } - // TODO(bryan.bernhart@intel.com): Conditionally disable sub-allocation. + // TODO(crbug.com/dawn/849): Conditionally disable sub-allocation. // For very large resources, there is no benefit to suballocate. // For very small resources, it is inefficent to suballocate given the min. heap // size could be much larger then the resource allocation. diff --git a/src/dawn_native/d3d12/SwapChainD3D12.cpp b/src/dawn_native/d3d12/SwapChainD3D12.cpp index 98388da0c7..d78c63d440 100644 --- a/src/dawn_native/d3d12/SwapChainD3D12.cpp +++ b/src/dawn_native/d3d12/SwapChainD3D12.cpp @@ -289,7 +289,7 @@ namespace dawn_native { namespace d3d12 { Device* device = ToBackend(GetDevice()); // Transition the texture to the present state as required by IDXGISwapChain1::Present() - // TODO(cwallez@chromium.org): Remove the need for this by eagerly transitioning the + // TODO(crbug.com/dawn/269): Remove the need for this by eagerly transitioning the // presentable texture to present at the end of submits that use them. CommandRecordingContext* commandContext; DAWN_TRY_ASSIGN(commandContext, device->GetPendingCommandContext()); @@ -320,7 +320,7 @@ namespace dawn_native { namespace d3d12 { // Synchronously wait until previous operations on the next swapchain buffer are finished. // This is the logic that performs frame pacing. - // TODO(cwallez@chromium.org): Consider whether this should be lifted for Mailbox so that + // TODO(crbug.com/dawn/269): Consider whether this should be lifted for Mailbox so that // there is not frame pacing. mCurrentBuffer = mDXGISwapChain->GetCurrentBackBufferIndex(); DAWN_TRY(device->WaitForSerial(mBufferLastUsedSerials[mCurrentBuffer])); diff --git a/src/dawn_native/d3d12/TextureD3D12.cpp b/src/dawn_native/d3d12/TextureD3D12.cpp index ee4ee14ebd..b39e88d2e5 100644 --- a/src/dawn_native/d3d12/TextureD3D12.cpp +++ b/src/dawn_native/d3d12/TextureD3D12.cpp @@ -499,7 +499,6 @@ namespace dawn_native { namespace d3d12 { resourceDescriptor.MipLevels = static_cast(GetNumMipLevels()); resourceDescriptor.Format = dxgiFormat; resourceDescriptor.SampleDesc.Count = GetSampleCount(); - // TODO(bryan.bernhart@intel.com): investigate how to specify standard MSAA sample pattern. resourceDescriptor.SampleDesc.Quality = 0; resourceDescriptor.Layout = D3D12_TEXTURE_LAYOUT_UNKNOWN; resourceDescriptor.Flags = @@ -652,7 +651,6 @@ namespace dawn_native { namespace d3d12 { ExecutionSerial pendingCommandSerial) const { // Reuse the subresource(s) directly and avoid transition when it isn't needed, and // return false. - // TODO(cwallez@chromium.org): Need some form of UAV barriers at some point. if (state->lastState == newState) { return; } @@ -1102,7 +1100,7 @@ namespace dawn_native { namespace d3d12 { // D3D12_SRV_DIMENSION_TEXTURE2DMS. // https://docs.microsoft.com/en-us/windows/desktop/api/d3d12/ns-d3d12-d3d12_tex2d_srv // https://docs.microsoft.com/en-us/windows/desktop/api/d3d12/ns-d3d12-d3d12_tex2d_array_srv - // TODO(jiawei.shao@intel.com): support more texture view dimensions. + // TODO(crbug.com/dawn/814): support 1D textures. if (GetTexture()->IsMultisampledTexture()) { switch (descriptor->dimension) { case wgpu::TextureViewDimension::e2DArray: diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index 568430d446..f28021e896 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -45,8 +45,6 @@ namespace dawn_native { namespace metal { storageMode = MTLResourceStorageModePrivate; } - // TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead - // of creating a new 4-byte buffer? if (GetSize() > std::numeric_limits::max()) { return DAWN_OUT_OF_MEMORY_ERROR("Buffer allocation is too large"); } diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index e7da83951b..a9f1896a94 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -359,7 +359,7 @@ namespace dawn_native { namespace metal { PipelineLayout* pipelineLayout) { uint32_t currentDynamicBufferIndex = 0; - // TODO(kainino@chromium.org): Maintain buffers and offsets arrays in BindGroup + // TODO(crbug.com/dawn/854): Maintain buffers and offsets arrays in BindGroup // so that we only have to do one setVertexBuffers and one setFragmentBuffers // call here. for (BindingIndex bindingIndex{0}; @@ -398,7 +398,7 @@ namespace dawn_native { namespace metal { const id buffer = ToBackend(binding.buffer)->GetMTLBuffer(); NSUInteger offset = binding.offset; - // TODO(shaobo.yan@intel.com): Record bound buffer status to use + // TODO(crbug.com/dawn/854): Record bound buffer status to use // setBufferOffset to achieve better performance. if (bindingInfo.buffer.hasDynamicOffset) { offset += dynamicOffsets[currentDynamicBufferIndex]; @@ -823,7 +823,7 @@ namespace dawn_native { namespace metal { EnsureDestinationTextureInitialized(commandContext, dstTexture, copy->destination, copy->copySize); - // TODO(jiawei.shao@intel.com): support copies with 1D textures. + // TODO(crbug.com/dawn/814): support copies with 1D textures. ASSERT(srcTexture->GetDimension() != wgpu::TextureDimension::e1D && dstTexture->GetDimension() != wgpu::TextureDimension::e1D); diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 45dce66ca4..0320887cc5 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -189,10 +189,10 @@ namespace dawn_native { namespace metal { SetToggle(Toggle::DisableBaseInstance, !haveBaseVertexBaseInstance); } - // TODO(jiawei.shao@intel.com): tighten this workaround when the driver bug is fixed. + // TODO(crbug.com/dawn/846): tighten this workaround when the driver bug is fixed. SetToggle(Toggle::AlwaysResolveIntoZeroLevelAndLayer, true); - // TODO(hao.x.li@intel.com): Use MTLStorageModeShared instead of MTLStorageModePrivate when + // TODO(crbug.com/dawn/847): Use MTLStorageModeShared instead of MTLStorageModePrivate when // creating MTLCounterSampleBuffer in QuerySet on Intel platforms, otherwise it fails to // create the buffer. Change to use MTLStorageModePrivate when the bug is fixed. if (@available(macOS 10.15, iOS 14.0, *)) { diff --git a/src/dawn_native/metal/SwapChainMTL.mm b/src/dawn_native/metal/SwapChainMTL.mm index b878893928..d49ed702dc 100644 --- a/src/dawn_native/metal/SwapChainMTL.mm +++ b/src/dawn_native/metal/SwapChainMTL.mm @@ -80,7 +80,7 @@ namespace dawn_native { namespace metal { ASSERT(GetSurface()->GetType() == Surface::Type::MetalLayer); if (previousSwapChain != nullptr) { - // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by + // TODO(crbug.com/dawn/269): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. if (previousSwapChain->GetBackendType() != wgpu::BackendType::Metal) { diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 9ffb21ddbb..7024c7a1cd 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -369,7 +369,7 @@ namespace dawn_native { namespace null { MaybeError SwapChain::Initialize(NewSwapChainBase* previousSwapChain) { if (previousSwapChain != nullptr) { - // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by + // TODO(crbug.com/dawn/269): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. if (previousSwapChain->GetBackendType() != wgpu::BackendType::Null) { diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp index f01c1b8fca..e6b9507e19 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -35,8 +35,6 @@ namespace dawn_native { namespace opengl { Buffer::Buffer(Device* device, const BufferDescriptor* descriptor) : BufferBase(device, descriptor) { - // TODO(cwallez@chromium.org): Have a global "zero" buffer instead of creating a new 4-byte - // buffer? uint64_t size = GetAppliedSize(); device->gl.GenBuffers(1, &mBuffer); @@ -69,8 +67,6 @@ namespace dawn_native { namespace opengl { } uint64_t Buffer::GetAppliedSize() const { - // TODO(cwallez@chromium.org): Have a global "zero" buffer instead of creating a new 4-byte - // buffer? return std::max(GetSize(), uint64_t(4u)); } @@ -151,7 +147,7 @@ namespace dawn_native { namespace opengl { EnsureDataInitialized(); - // TODO(cwallez@chromium.org): this does GPU->CPU synchronization, we could require a high + // This does GPU->CPU synchronization, we could require a high // version of OpenGL that would let us map the buffer unsynchronized. gl.BindBuffer(GL_ARRAY_BUFFER, mBuffer); void* mappedData = nullptr; diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 0d8599f969..40eb26b249 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -774,7 +774,7 @@ namespace dawn_native { namespace opengl { auto& src = copy->source; auto& dst = copy->destination; - // TODO(jiawei.shao@intel.com): add workaround for the case that imageExtentSrc + // TODO(crbug.com/dawn/817): add workaround for the case that imageExtentSrc // is not equal to imageExtentDst. For example when copySize fits in the virtual // size of the source image but does not fit in the one of the destination // image. @@ -805,7 +805,7 @@ namespace dawn_native { namespace opengl { } case Command::ResolveQuerySet: { - // TODO(hao.x.li@intel.com): Resolve non-precise occlusion query. + // TODO(crbug.com/dawn/434): Resolve non-precise occlusion query. SkipCommand(&mCommands, type); break; } @@ -849,7 +849,6 @@ namespace dawn_native { namespace opengl { bindGroupTracker.Apply(gl); gl.DispatchCompute(dispatch->x, dispatch->y, dispatch->z); - // TODO(cwallez@chromium.org): add barriers to the API gl.MemoryBarrier(GL_ALL_BARRIER_BITS); break; } @@ -863,7 +862,6 @@ namespace dawn_native { namespace opengl { gl.BindBuffer(GL_DISPATCH_INDIRECT_BUFFER, indirectBuffer->GetHandle()); gl.DispatchComputeIndirect(static_cast(indirectBufferOffset)); - // TODO(cwallez@chromium.org): add barriers to the API gl.MemoryBarrier(GL_ALL_BARRIER_BITS); break; } @@ -964,8 +962,6 @@ namespace dawn_native { namespace opengl { // Attach depth/stencil buffer. GLenum glAttachment = 0; - // TODO(kainino@chromium.org): it may be valid to just always use - // GL_DEPTH_STENCIL_ATTACHMENT here. if (format.aspects == (Aspect::Depth | Aspect::Stencil)) { glAttachment = GL_DEPTH_STENCIL_ATTACHMENT; } else if (format.aspects == Aspect::Depth) { diff --git a/src/dawn_native/opengl/TextureGL.cpp b/src/dawn_native/opengl/TextureGL.cpp index 8b97064ea7..dc4cddfa25 100644 --- a/src/dawn_native/opengl/TextureGL.cpp +++ b/src/dawn_native/opengl/TextureGL.cpp @@ -210,7 +210,7 @@ namespace dawn_native { namespace opengl { MaybeError Texture::ClearTexture(const SubresourceRange& range, TextureBase::ClearValue clearValue) { - // TODO(jiawei.shao@intel.com): initialize the textures with compressed formats. + // TODO(crbug.com/dawn/850): initialize the textures with compressed formats. if (GetFormat().isCompressed) { return {}; } @@ -539,7 +539,7 @@ namespace dawn_native { namespace opengl { mHandle = ToBackend(texture)->GetHandle(); } else { // glTextureView() is supported on OpenGL version >= 4.3 - // TODO(jiawei.shao@intel.com): support texture view on OpenGL version <= 4.2 + // TODO(crbug.com/dawn/593): support texture view on OpenGL version <= 4.2 and ES const OpenGLFunctions& gl = ToBackend(GetDevice())->gl; mHandle = GenTexture(gl); const Texture* textureGL = ToBackend(texture); diff --git a/src/dawn_native/vulkan/BindGroupVk.cpp b/src/dawn_native/vulkan/BindGroupVk.cpp index 251e4ff737..eec2c3d39a 100644 --- a/src/dawn_native/vulkan/BindGroupVk.cpp +++ b/src/dawn_native/vulkan/BindGroupVk.cpp @@ -138,7 +138,7 @@ namespace dawn_native { namespace vulkan { numWrites++; } - // TODO(cwallez@chromium.org): Batch these updates + // TODO(crbug.com/dawn/855): Batch these updates device->fn.UpdateDescriptorSets(device->GetVkDevice(), numWrites, writes.data(), 0, nullptr); } diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index cc2ebb6c2c..efe26c6b7f 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -153,8 +153,6 @@ namespace dawn_native { namespace vulkan { createInfo.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO; createInfo.pNext = nullptr; createInfo.flags = 0; - // TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead - // of creating a new 4-byte buffer? createInfo.size = std::max(GetSize(), uint64_t(4u)); // Add CopyDst for non-mappable buffer initialization with mappedAtCreation // and robust resource initialization. @@ -263,7 +261,7 @@ namespace dawn_native { namespace vulkan { CommandRecordingContext* recordingContext = device->GetPendingRecordingContext(); - // TODO(jiawei.shao@intel.com): initialize mapped buffer in CPU side. + // TODO(crbug.com/dawn/852): initialize mapped buffer in CPU side. EnsureDataInitialized(recordingContext); if (mode & wgpu::MapMode::Read) { @@ -352,8 +350,8 @@ namespace dawn_native { namespace vulkan { TransitionUsageNow(recordingContext, wgpu::BufferUsage::CopyDst); Device* device = ToBackend(GetDevice()); - // TODO(jiawei.shao@intel.com): find out why VK_WHOLE_SIZE doesn't work on old Windows Intel - // Vulkan drivers. + // This code is fine. According to jiawei.shao@intel.com, VK_WHOLE_SIZE doesn't work + // on old Windows Intel Vulkan drivers, so we don't use it. device->fn.CmdFillBuffer(recordingContext->commandBuffer, mHandle, 0, GetSize(), clearValue); } diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h index b55cf9b737..51c6993caa 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -35,7 +35,7 @@ namespace dawn_native { namespace vulkan { // Transitions the buffer to be used as `usage`, recording any necessary barrier in // `commands`. - // TODO(cwallez@chromium.org): coalesce barriers and do them early when possible. + // TODO(crbug.com/dawn/851): coalesce barriers and do them early when possible. void TransitionUsageNow(CommandRecordingContext* recordingContext, wgpu::BufferUsage usage); bool TransitionUsageAndGetResourceBarrier(wgpu::BufferUsage usage, VkBufferMemoryBarrier* barrier, diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index bbdcf6c3b9..2383e90972 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -91,7 +91,7 @@ namespace dawn_native { namespace vulkan { region.srcOffset.z = srcCopy.origin.z; break; case wgpu::TextureDimension::e1D: - // TODO(jiawei.shao@intel.com): support 1D textures + // TODO(crbug.com/dawn/814): support 1D textures UNREACHABLE(); } @@ -110,7 +110,7 @@ namespace dawn_native { namespace vulkan { region.dstOffset.z = dstCopy.origin.z; break; case wgpu::TextureDimension::e1D: - // TODO(jiawei.shao@intel.com): support 1D textures + // TODO(crbug.com/dawn/814): support 1D textures UNREACHABLE(); } diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 9e8ad22134..cc69fa96b9 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -449,7 +449,7 @@ namespace dawn_native { namespace vulkan { } void Device::InitTogglesFromDriver() { - // TODO(jiawei.shao@intel.com): tighten this workaround when this issue is fixed in both + // TODO(crbug.com/dawn/857): tighten this workaround when this issue is fixed in both // Vulkan SPEC and drivers. SetToggle(Toggle::UseTemporaryBufferInCompressedTextureToTextureCopy, true); diff --git a/src/dawn_native/vulkan/NativeSwapChainImplVk.cpp b/src/dawn_native/vulkan/NativeSwapChainImplVk.cpp index 54dd06614a..9782a2bfbd 100644 --- a/src/dawn_native/vulkan/NativeSwapChainImplVk.cpp +++ b/src/dawn_native/vulkan/NativeSwapChainImplVk.cpp @@ -48,13 +48,13 @@ namespace dawn_native { namespace vulkan { if (!chooseSwapPresentMode(info.presentModes, turnOffVsync, &presentMode)) { return false; } - // TODO(cwallez@chromium.org): For now this is hardcoded to what works with one NVIDIA + // TODO(crbug.com/dawn/269): For now this is hardcoded to what works with one NVIDIA // driver. Need to generalize config->nativeFormat = VK_FORMAT_B8G8R8A8_UNORM; config->colorSpace = VK_COLOR_SPACE_SRGB_NONLINEAR_KHR; config->format = wgpu::TextureFormat::BGRA8Unorm; config->minImageCount = 3; - // TODO(cwallez@chromium.org): This is upside down compared to what we want, at least + // TODO(crbug.com/dawn/269): This is upside down compared to what we want, at least // on Linux config->preTransform = info.capabilities.currentTransform; config->presentMode = presentMode; @@ -109,7 +109,7 @@ namespace dawn_native { namespace vulkan { ASSERT(mInfo.capabilities.maxImageExtent.height >= height); ASSERT(format == static_cast(GetPreferredFormat())); - // TODO(cwallez@chromium.org): need to check usage works too + // TODO(crbug.com/dawn/269): need to check usage works too // Create the swapchain with the configuration we chose VkSwapchainKHR oldSwapchain = mSwapChain; diff --git a/src/dawn_native/vulkan/PipelineLayoutVk.cpp b/src/dawn_native/vulkan/PipelineLayoutVk.cpp index 2aff50d921..6f7ab54115 100644 --- a/src/dawn_native/vulkan/PipelineLayoutVk.cpp +++ b/src/dawn_native/vulkan/PipelineLayoutVk.cpp @@ -33,7 +33,7 @@ namespace dawn_native { namespace vulkan { MaybeError PipelineLayout::Initialize() { // Compute the array of VkDescriptorSetLayouts that will be chained in the create info. - // TODO(cwallez@chromium.org) Vulkan doesn't allow holes in this array, should we expose + // TODO(crbug.com/dawn/277) Vulkan doesn't allow holes in this array, should we expose // this constraints at the Dawn level? uint32_t numSetLayouts = 0; std::array setLayouts; diff --git a/src/dawn_native/vulkan/RenderPipelineVk.cpp b/src/dawn_native/vulkan/RenderPipelineVk.cpp index fd29af15e9..1a2c84e0e8 100644 --- a/src/dawn_native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn_native/vulkan/RenderPipelineVk.cpp @@ -457,7 +457,6 @@ namespace dawn_native { namespace vulkan { // LogicOp isn't supported so we disable it. colorBlend.logicOpEnable = VK_FALSE; colorBlend.logicOp = VK_LOGIC_OP_CLEAR; - // TODO(cwallez@chromium.org): Do we allow holes in the color attachments? colorBlend.attachmentCount = static_cast(GetColorAttachmentsMask().count()); colorBlend.pAttachments = colorBlendAttachments.data(); // The blend constant is always dynamic so we fill in a dummy value diff --git a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp index 13ea9bedb9..0f4d050e7e 100644 --- a/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp +++ b/src/dawn_native/vulkan/ResourceMemoryAllocatorVk.cpp @@ -26,7 +26,7 @@ namespace dawn_native { namespace vulkan { namespace { - // TODO(cwallez@chromium.org): This is a hardcoded heurstic to choose when to + // TODO(crbug.com/dawn/849): This is a hardcoded heurstic to choose when to // suballocate but it should ideally depend on the size of the memory heaps and other // factors. constexpr uint64_t kMaxSizeForSubAllocation = 4ull * 1024ull * 1024ull; // 4MiB @@ -134,7 +134,7 @@ namespace dawn_native { namespace vulkan { // Sub-allocate non-mappable resources because at the moment the mapped pointer // is part of the resource and not the heap, which doesn't match the Vulkan model. - // TODO(cwallez@chromium.org): allow sub-allocating mappable resources, maybe. + // TODO(crbug.com/dawn/849): allow sub-allocating mappable resources, maybe. if (requirements.size < kMaxSizeForSubAllocation && !mappable) { ResourceMemoryAllocation subAllocation; DAWN_TRY_ASSIGN(subAllocation, @@ -185,7 +185,7 @@ namespace dawn_native { namespace vulkan { // Suballocations aren't freed immediately, otherwise another resource allocation could // happen just after that aliases the old one and would require a barrier. - // TODO(cwallez@chromium.org): Maybe we can produce the correct barriers to reduce the + // TODO(crbug.com/dawn/851): Maybe we can produce the correct barriers to reduce the // latency to reclaim memory. case AllocationMethod::kSubAllocated: mSubAllocationsToDelete.Enqueue(*allocation, mDevice->GetPendingCommandSerial()); diff --git a/src/dawn_native/vulkan/SwapChainVk.cpp b/src/dawn_native/vulkan/SwapChainVk.cpp index 29c2454d04..933f7b4d99 100644 --- a/src/dawn_native/vulkan/SwapChainVk.cpp +++ b/src/dawn_native/vulkan/SwapChainVk.cpp @@ -233,19 +233,19 @@ namespace dawn_native { namespace vulkan { VkSwapchainKHR previousVkSwapChain = VK_NULL_HANDLE; if (previousSwapChain != nullptr) { - // TODO(cwallez@chromium.org): The first time a surface is used with a Device, check + // TODO(crbug.com/dawn/269): The first time a surface is used with a Device, check // it is supported with vkGetPhysicalDeviceSurfaceSupportKHR. - // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by + // TODO(crbug.com/dawn/269): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. if (previousSwapChain->GetBackendType() != wgpu::BackendType::Vulkan) { return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between APIs"); } - // TODO(cwallez@chromium.org): use ToBackend once OldSwapChainBase is removed. + // TODO(crbug.com/dawn/269): use ToBackend once OldSwapChainBase is removed. SwapChain* previousVulkanSwapChain = static_cast(previousSwapChain); - // TODO(cwallez@chromium.org): Figure out switching a single surface between multiple + // TODO(crbug.com/dawn/269): Figure out switching a single surface between multiple // Vulkan devices on different VkInstances. Probably needs to block too! VkInstance previousInstance = ToBackend(previousSwapChain->GetDevice())->GetVkInstance(); @@ -447,7 +447,7 @@ namespace dawn_native { namespace vulkan { config.extent = surfaceInfo.capabilities.currentExtent; } - // TODO(cwallez@chromium.org): If the swapchain image doesn't support TRANSFER_DST + // TODO(crbug.com/dawn/269): If the swapchain image doesn't support TRANSFER_DST // then we'll need to have a second fallback that uses a blit shader :( if ((supportedUsages & VK_IMAGE_USAGE_TRANSFER_DST_BIT) == 0) { return DAWN_INTERNAL_ERROR( @@ -492,14 +492,14 @@ namespace dawn_native { namespace vulkan { mTexture->GetHandle(), mTexture->GetCurrentLayoutForSwapChain(), 1, ®ion, VK_FILTER_LINEAR); - // TODO(cwallez@chromium.org): Find a way to reuse the blit texture between frames + // TODO(crbug.com/dawn/269): Find a way to reuse the blit texture between frames // instead of creating a new one every time. This will involve "un-destroying" the // texture or making the blit texture "external". mBlitTexture->APIDestroy(); mBlitTexture = nullptr; } - // TODO(cwallez@chromium.org): Remove the need for this by eagerly transitioning the + // TODO(crbug.com/dawn/269): Remove the need for this by eagerly transitioning the // presentable texture to present at the end of submits that use them and ideally even // folding that in the free layout transition at the end of render passes. mTexture->TransitionUsageNow(recordingContext, kPresentTextureUsage, @@ -510,7 +510,7 @@ namespace dawn_native { namespace vulkan { // Assuming that the present queue is the same as the graphics queue, the proper // synchronization has already been done on the queue so we don't need to wait on any // semaphores. - // TODO(cwallez@chromium.org): Support the present queue not being the main queue. + // TODO(crbug.com/dawn/269): Support the present queue not being the main queue. VkPresentInfoKHR presentInfo; presentInfo.sType = VK_STRUCTURE_TYPE_PRESENT_INFO_KHR; presentInfo.pNext = nullptr; @@ -541,7 +541,7 @@ namespace dawn_native { namespace vulkan { case VK_ERROR_OUT_OF_DATE_KHR: return Initialize(this); - // TODO(cwallez@chromium.org): Allow losing the surface at Dawn's API level? + // TODO(crbug.com/dawn/269): Allow losing the surface at Dawn's API level? case VK_ERROR_SURFACE_LOST_KHR: default: return CheckVkSuccess(::VkResult(result), "QueuePresent"); @@ -572,7 +572,7 @@ namespace dawn_native { namespace vulkan { VkFence{}, &mLastImageIndex)); if (result == VK_SUCCESS) { - // TODO(cwallez@chromium.org) put the semaphore on the texture so it is waited on when + // TODO(crbug.com/dawn/269) put the semaphore on the texture so it is waited on when // used instead of directly on the recording context? device->GetPendingRecordingContext()->waitSemaphores.push_back(semaphore); } else { @@ -582,7 +582,7 @@ namespace dawn_native { namespace vulkan { } switch (result) { - // TODO(cwallez@chromium.org): Introduce a mechanism to notify the application that + // TODO(crbug.com/dawn/269): Introduce a mechanism to notify the application that // the swapchain is in a suboptimal state? case VK_SUBOPTIMAL_KHR: case VK_SUCCESS: @@ -592,7 +592,7 @@ namespace dawn_native { namespace vulkan { // Prevent infinite recursive calls to GetCurrentTextureViewInternal when the // swapchains always return that they are out of date. if (isReentrant) { - // TODO(cwallez@chromium.org): Allow losing the surface instead? + // TODO(crbug.com/dawn/269): Allow losing the surface instead? return DAWN_INTERNAL_ERROR( "Wasn't able to recuperate the surface after a VK_ERROR_OUT_OF_DATE_KHR"); } @@ -602,7 +602,7 @@ namespace dawn_native { namespace vulkan { return GetCurrentTextureViewInternal(true); } - // TODO(cwallez@chromium.org): Allow losing the surface at Dawn's API level? + // TODO(crbug.com/dawn/269): Allow losing the surface at Dawn's API level? case VK_ERROR_SURFACE_LOST_KHR: default: DAWN_TRY(CheckVkSuccess(::VkResult(result), "AcquireNextImage")); diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index def417ab64..48eea440f2 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -55,7 +55,7 @@ namespace dawn_native { namespace vulkan { } // Computes which vulkan access type could be required for the given Dawn usage. - // TODO(cwallez@chromium.org): We shouldn't need any access usages for srcAccessMask when + // TODO(crbug.com/dawn/269): We shouldn't need any access usages for srcAccessMask when // the previous usage is readonly because an execution dependency is sufficient. VkAccessFlags VulkanAccessFlags(wgpu::TextureUsage usage, const Format& format) { VkAccessFlags flags = 0; @@ -119,7 +119,7 @@ namespace dawn_native { namespace vulkan { flags |= VK_PIPELINE_STAGE_TRANSFER_BIT; } if (usage & (wgpu::TextureUsage::Sampled | kReadOnlyStorageTexture)) { - // TODO(cwallez@chromium.org): Only transition to the usage we care about to avoid + // TODO(crbug.com/dawn/851): Only transition to the usage we care about to avoid // introducing FS -> VS dependencies that would prevent parallelization on tiler // GPUs flags |= VK_PIPELINE_STAGE_VERTEX_SHADER_BIT | @@ -134,7 +134,7 @@ namespace dawn_native { namespace vulkan { if (format.HasDepthOrStencil()) { flags |= VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT | VK_PIPELINE_STAGE_LATE_FRAGMENT_TESTS_BIT; - // TODO(cwallez@chromium.org): This is missing the stage where the depth and + // TODO(crbug.com/dawn/853): This is missing the stage where the depth and // stencil values are written, but it isn't clear which one it is. } else { flags |= VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT; @@ -410,7 +410,7 @@ namespace dawn_native { namespace vulkan { // or a combination with something else, the texture could be in a combination of // GENERAL and TRANSFER_SRC_OPTIMAL. This would be a problem, so we make CopySrc use // GENERAL. - // TODO(cwallez@chromium.org): We no longer need to transition resources all at + // TODO(crbug.com/dawn/851): We no longer need to transition resources all at // once and can instead track subresources so we should lift this limitation. case wgpu::TextureUsage::CopySrc: // Read-only and write-only storage textures must use general layout because load diff --git a/src/dawn_native/vulkan/TextureVk.h b/src/dawn_native/vulkan/TextureVk.h index 908e468792..e985c55071 100644 --- a/src/dawn_native/vulkan/TextureVk.h +++ b/src/dawn_native/vulkan/TextureVk.h @@ -66,17 +66,10 @@ namespace dawn_native { namespace vulkan { // Transitions the texture to be used as `usage`, recording any necessary barrier in // `commands`. - // TODO(cwallez@chromium.org): coalesce barriers and do them early when possible. + // TODO(crbug.com/dawn/851): coalesce barriers and do them early when possible. void TransitionUsageNow(CommandRecordingContext* recordingContext, wgpu::TextureUsage usage, const SubresourceRange& range); - // TODO(cwallez@chromium.org): This function should be an implementation detail of - // vulkan::Texture but it is currently used by the barrier tracking for compute passes. - void TransitionUsageAndGetResourceBarrier(wgpu::TextureUsage usage, - const SubresourceRange& range, - std::vector* imageBarriers, - VkPipelineStageFlags* srcStages, - VkPipelineStageFlags* dstStages); void TransitionUsageForPass(CommandRecordingContext* recordingContext, const TextureSubresourceUsage& textureUsages, std::vector* imageBarriers, @@ -115,6 +108,11 @@ namespace dawn_native { namespace vulkan { TextureBase::ClearValue); // Implementation details of the barrier computations for the texture. + void TransitionUsageAndGetResourceBarrier(wgpu::TextureUsage usage, + const SubresourceRange& range, + std::vector* imageBarriers, + VkPipelineStageFlags* srcStages, + VkPipelineStageFlags* dstStages); void TransitionUsageForPassImpl( CommandRecordingContext* recordingContext, const SubresourceStorage& subresourceUsages, diff --git a/src/utils/TerribleCommandBuffer.cpp b/src/utils/TerribleCommandBuffer.cpp index c6f1ac3f27..0c06ab1036 100644 --- a/src/utils/TerribleCommandBuffer.cpp +++ b/src/utils/TerribleCommandBuffer.cpp @@ -34,9 +34,7 @@ namespace utils { } void* TerribleCommandBuffer::GetCmdSpace(size_t size) { - // TODO(kainino@chromium.org): Should we early-out if size is 0? - // (Here and/or in the caller?) It might be good to make the wire receiver get a nullptr - // instead of pointer to zero-sized allocation in mBuffer. + // Note: This returns non-null even if size is zero. if (size > sizeof(mBuffer)) { return nullptr; } diff --git a/src/utils/TestUtils.cpp b/src/utils/TestUtils.cpp index 69b75ca122..cac3a1ac9c 100644 --- a/src/utils/TestUtils.cpp +++ b/src/utils/TestUtils.cpp @@ -36,7 +36,7 @@ namespace utils { wgpu::Extent3D textureSizeAtLevel0, uint32_t mipmapLevel, uint32_t rowsPerImage) { - // TODO(jiawei.shao@intel.com): support compressed texture formats + // Compressed texture formats not supported in this function yet. ASSERT(utils::GetTextureFormatBlockWidth(format) == 1); TextureDataCopyLayout layout;