Bug 195519 - [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from and GPUBindGroup validation to GPUDevice.createBindGroup
Summary: [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from and GPUBi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-09 04:11 PST by Justin Fan
Modified: 2019-03-11 20:19 PDT (History)
7 users (show)

See Also:


Attachments
Patch (41.43 KB, patch)
2019-03-11 14:28 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (42.81 KB, patch)
2019-03-11 15:25 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (41.04 KB, patch)
2019-03-11 19:41 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-03-09 04:11:15 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-03-11 14:00:59 PDT
<rdar://problem/48781297>
Comment 2 Justin Fan 2019-03-11 14:28:32 PDT
Created attachment 364287 [details]
Patch
Comment 3 Justin Fan 2019-03-11 15:25:21 PDT
Created attachment 364298 [details]
Patch
Comment 4 Myles C. Maxfield 2019-03-11 16:44:30 PDT
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.
Comment 5 Justin Fan 2019-03-11 19:41:00 PDT
Created attachment 364336 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2019-03-11 20:19:17 PDT
Comment on attachment 364336 [details]
Patch for landing

Clearing flags on attachment: 364336

Committed r242766: <https://trac.webkit.org/changeset/242766>
Comment 7 WebKit Commit Bot 2019-03-11 20:19:19 PDT
All reviewed patches have been landed.  Closing bug.