Summary: | [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from and GPUBindGroup validation to GPUDevice.createBindGroup | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||||
Component: | WebGPU | Assignee: | Justin Fan <justin_fan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dino, ews-watchlist, graouts, kondapallykalyan, mmaxfield, webkit-bug-importer | ||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Justin Fan
2019-03-09 04:11:15 PST
Created attachment 364287 [details]
Patch
Created attachment 364298 [details]
Patch
Comment on attachment 364298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364298&action=review > Source/WebCore/platform/graphics/gpu/GPUBindGroup.h:60 > + RetainPtr<MTLBuffer> m_vertexArgsBuffer; > + RetainPtr<MTLBuffer> m_fragmentArgsBuffer; Compute :( > Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm:66 > +static RetainPtr<MTLArgumentEncoder> tryCreateMtlArgumentEncoder(const GPUDevice& device, ArgumentArray array) Do you need to say it's Mtl? It's in a Metal file already. > Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupMetal.mm:202 > + case GPUBindGroupLayoutBinding::BindingType::StorageBuffer: { > + auto bufferResource = tryGetResourceAsBufferBinding(resourceBinding.resource, functionName); > + if (!bufferResource) > + return nullptr; > + if (layoutBinding.visibility & GPUShaderStageBit::Flags::Vertex) { > + if (!trySetBufferOnEncoder(vertexEncoder, *bufferResource, resourceBinding.binding, functionName)) > + return nullptr; > + } > + if (layoutBinding.visibility & GPUShaderStageBit::Flags::Fragment) { > + if (!trySetBufferOnEncoder(fragmentEncoder, *bufferResource, resourceBinding.binding, functionName)) > + return nullptr; > + } > + boundBuffers.append(bufferResource->buffer.copyRef()); > + break; > + } Can we collapse the cases somehow? There seems to be a lot of duplicated code. It seems wrong that we mark the buffer as bound even if visibility == NONE. > Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:60 > + if (bindGroup.vertexArgsBuffer()) > + setVertexBuffer(bindGroup.vertexArgsBuffer(), 0, index); > + if (bindGroup.fragmentArgsBuffer()) > + setFragmentBuffer(bindGroup.fragmentArgsBuffer(), 0, index); Yes! This is awesome. Created attachment 364336 [details]
Patch for landing
Comment on attachment 364336 [details] Patch for landing Clearing flags on attachment: 364336 Committed r242766: <https://trac.webkit.org/changeset/242766> All reviewed patches have been landed. Closing bug. |