[webkit-reviews] review granted: [Bug 195519] [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from and GPUBindGroup validation to GPUDevice.createBindGroup : [Attachment 364298] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Justin Fan
<justin_fan at apple.com>'s request for review:
Bug 195519: [Web GPU] BindGroups/Argument buffers: Move MTLBuffer creation from
and GPUBindGroup validation to GPUDevice.createBindGroup
https://bugs.webkit.org/show_bug.cgi?id=195519

Attachment 364298: Patch

https://bugs.webkit.org/attachment.cgi?id=364298&action=review




--- Comment #4 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 364298
  --> https://bugs.webkit.org/attachment.cgi?id=364298
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:6
0
> +    if (bindGroup.vertexArgsBuffer())
> +	   setVertexBuffer(bindGroup.vertexArgsBuffer(), 0, index);
> +    if (bindGroup.fragmentArgsBuffer())
> +	   setFragmentBuffer(bindGroup.fragmentArgsBuffer(), 0, index);

Yes! This is awesome.