Bug 195519 – [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from and GPUBindGroup validation to GPUDevice.createBindGroup

• Component: WebGPU
• Status: RESOLVED FIXED

[Justin Fan]

The MTLBuffers that serve as argument buffers in the GPUBindGroup backend shouldn't be created until the GPUBindGroup is created. Each GPUBindGroup requires its own unique MTLBuffer(s), but a GPUBindGroupLayout can be used by different GPUBindGroups or GPUPipelineLayouts.

[Radar WebKit Bug Importer]

<rdar://problem/48781297>

[Justin Fan]

Created attachment 364287 [details]
Patch

[Justin Fan]

Created attachment 364298 [details]
Patch

[Myles C. Maxfield]

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.

[Justin Fan]

Created attachment 364336 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364336 [details]
Patch for landing

Clearing flags on attachment: 364336

Committed r242766: <https://trac.webkit.org/changeset/242766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.