Bug 193457 – [WebGPU] WebGPUProgrammablePassEncoder::setBindGroup prototype

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Justin Fan]

[WebGPU] WebGPUProgrammablePassEncoder::setBindGroup prototype

[Justin Fan]

Created attachment 359192 [details]
Patch

[EWS Watchlist]

Attachment 359192 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:59:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:64:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:120:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/47296678>

[Justin Fan]

Very rough draft. Not ready for review-review, but comments are appreciated.

[Myles C. Maxfield]

Comment on attachment 359192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359192&action=review

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:55
> +    const ArgumentEncoders& argumentEncoders() const { return m_argumentEncoders; }

I don't see this type in the tree. Does this patch depend on another patch? The bug doesn't say it depends on anything.

> Source/WebCore/platform/graphics/gpu/GPUCommandBuffer.h:39
> +class GPUDevice; // FIXME: Why is ommitting this throwing "Unknown type name 'GPUDevice'" error despite #include "GPUDevice.h"?

This symptom is usually caused by circular #includes.

> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:60
> +    virtual void useResource(MTLResource *, unsigned long) = 0;

Are you sure that this is allowed to know about Metal types? It isn't in a subdirectory for a particular platform, and doesn't have Metal in the name... Consider subclassing or wrapping in #if USE(METAL)

> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:70
> +    [argumentEncoder setBuffer:buffer offset:bufferBinding.offset atIndex:index];

Where do you call -[MTLArgumentEncoder setTexture:atIndex:] and -[MTLArgumentEncoder setSamplerState:atIndex]?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:91
> +        vertexArgumentBuffer = adoptNS([m_device->platformDevice() newBufferWithLength:encoders.vertex.get().encodedLength options:0]);

This definitely needs to be done in WebGPUDevice.createBindGroup(), not in WebGPUProgrammablePassEncoder.setBindGroup(). The argument buffers should already be created and populated before WebGPUProgrammablePassEncoder.setBindGroup() is called.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:93
> +        setVertexBuffer(vertexArgumentBuffer.get(), 0, index);

This call (and setFragmentBuffer and setComputeBuffer) should be the only things that this function does.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:103
> +        // FIXME: Finish support for compute.

👎 Compute should be a first-class citizen

> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:129
> +                LOG(WebGPU, "%s: Resource type not yet implemented.", functionName);

👎 Textures and samplers should be first-class citizens

[Justin Fan]

Comment on attachment 359192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359192&action=review

>> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:55
>> +    const ArgumentEncoders& argumentEncoders() const { return m_argumentEncoders; }
> 
> I don't see this type in the tree. Does this patch depend on another patch? The bug doesn't say it depends on anything.

It's a struct declared in this same header and already in the code.

>> Source/WebCore/platform/graphics/gpu/GPUCommandBuffer.h:39
>> +class GPUDevice; // FIXME: Why is ommitting this throwing "Unknown type name 'GPUDevice'" error despite #include "GPUDevice.h"?
> 
> This symptom is usually caused by circular #includes.

Grr. Time to do some graph traversal.

>> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:60
>> +    virtual void useResource(MTLResource *, unsigned long) = 0;
> 
> Are you sure that this is allowed to know about Metal types? It isn't in a subdirectory for a particular platform, and doesn't have Metal in the name... Consider subclassing or wrapping in #if USE(METAL)

I'll throw them in #if USE(METAL). These functions are used only to support the Metal backend for this class.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:70
>> +    [argumentEncoder setBuffer:buffer offset:bufferBinding.offset atIndex:index];
> 
> Where do you call -[MTLArgumentEncoder setTexture:atIndex:] and -[MTLArgumentEncoder setSamplerState:atIndex]?

Due to time pressure I'm not supporting textures or samplers in this patch. They will be handled by their own code that deals with resources of those types.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:91
>> +        vertexArgumentBuffer = adoptNS([m_device->platformDevice() newBufferWithLength:encoders.vertex.get().encodedLength options:0]);
> 
> This definitely needs to be done in WebGPUDevice.createBindGroup(), not in WebGPUProgrammablePassEncoder.setBindGroup(). The argument buffers should already be created and populated before WebGPUProgrammablePassEncoder.setBindGroup() is called.

a g r e e d. Thanks!

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:103
>> +        // FIXME: Finish support for compute.
> 
> 👎 Compute should be a first-class citizen

:( I guess I'm biased against compute. I feel bad

Compute will be tackled after the WebGPU F2F next week.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUProgrammablePassEncoderMetal.mm:129
>> +                LOG(WebGPU, "%s: Resource type not yet implemented.", functionName);
> 
> 👎 Textures and samplers should be first-class citizens

Ditto.

[Justin Fan]

Created attachment 359337 [details]
Patch

[Justin Fan]

Created attachment 359350 [details]
Patch

[EWS Watchlist]

Attachment 359350 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:59:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

TODO:

1) I'm currently creating one set of argument buffers per BindGroupLayout, when there needs to be one set per BindGroupLayoutBinding. 

2) Move MTLBuffer creation out of setBindGroup into BindGroupLayout creation to improve setBindGroup performance.

[Justin Fan]

(In reply to Justin Fan from comment #10)
> TODO:
> 
> 1) I'm currently creating one set of argument buffers per BindGroupLayout,
> when there needs to be one set per BindGroupLayoutBinding. 

Ignore this. Confused BindGroupLayout with BindGroup. A unique BindGroup should be backed by a unique (set of) argument buffers, but the whole point of BindGroupLayoutBinding is to combine an entire list of them into one argument buffer.

[Justin Fan]

Created attachment 359503 [details]
Patch

[EWS Watchlist]

Attachment 359503 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:58:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

Created attachment 359508 [details]
Patch

[EWS Watchlist]

Attachment 359508 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:58:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

Created attachment 359510 [details]
Patch

[EWS Watchlist]

Attachment 359510 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:58:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 359510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359510&action=review

> Source/WebCore/ChangeLog:11
> +        Support for compute pipelines to follow shortly. 
> +        Texture and sampler resources to follow after their requisite interfaces are implemented. 

I wouldn't worry about adding these comments.

> Source/WebCore/ChangeLog:25
> +        * platform/graphics/gpu/GPUBindGroupLayout.h: Added ArgumentEncoderBuffer struct to retain ptr tot both MTLArgumentEncoders and their argument MTLBuffers.

Typo tot

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17048
> +		1CA0C2F621EEDAD200A11860 /* AST */ = {
> +			isa = PBXGroup;
> +			children = (
> +				1C840B9021EC30F900D0500D /* WHLSLAddressSpace.h */,
> +				C21BF72521CD89E200227979 /* WHLSLArrayReferenceType.h */,
> +				C21BF70921CD89CA00227979 /* WHLSLArrayType.h */,
> +				C21BF73021CD89ED00227979 /* WHLSLAssignmentExpression.h */,
> +				C21BF70A21CD89CB00227979 /* WHLSLBaseFunctionAttribute.h */,
> +				C21BF6FA21CD89BE00227979 /* WHLSLBaseSemantic.h */,
> +				C21BF71E21CD89DC00227979 /* WHLSLBlock.h */,
> +				C21BF6F621CD89B700227979 /* WHLSLBooleanLiteral.h */,
> +				C21BF71A21CD89D800227979 /* WHLSLBreak.h */,
> +				C2138A1321DDECD300F516BA /* WHLSLBuiltInSemantic.cpp */,
> +				C21BF72221CD89DF00227979 /* WHLSLBuiltInSemantic.h */,
> +				C21BF71621CD89D500227979 /* WHLSLCallExpression.h */,

What is this change?

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:38
>  OBJC_PROTOCOL(MTLArgumentEncoder);
> +OBJC_PROTOCOL(MTLBuffer);

This should probably be inside #if USE(METAL) too

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:55
> +        bool operator!() const { return !encoder || !buffer; }

I think this would be better as an isValid() function rather than an operator.

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:64
> +    GPUBindGroupLayout(BindingsMapType&&, ArgumentEncoderBuffer&&, ArgumentEncoderBuffer&&, ArgumentEncoderBuffer&&);

You should probably give these parameters names here. The order isn't obvious.

> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63
> +    virtual void useResource(MTLResource *, unsigned long) = 0;
> +
> +    // Render command encoder methods.
> +    virtual void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) { }
> +    virtual void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) { }

MTLFoo are ObjC types, so should be MTLFoo* not MTLFoo *.

> Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:69
> +    void useResource(MTLResource *, unsigned long) final;
> +    void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) final;
> +    void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) final;

Ditto.

[Justin Fan]

(In reply to Dean Jackson from comment #18)
> Comment on attachment 359510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359510&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:17048
> > +		1CA0C2F621EEDAD200A11860 /* AST */ = {
> > +			isa = PBXGroup;
> > +			children = (
> > +				1C840B9021EC30F900D0500D /* WHLSLAddressSpace.h */,
> > +				C21BF72521CD89E200227979 /* WHLSLArrayReferenceType.h */,
> > +				C21BF70921CD89CA00227979 /* WHLSLArrayType.h */,
> > +				C21BF73021CD89ED00227979 /* WHLSLAssignmentExpression.h */,
> > +				C21BF70A21CD89CB00227979 /* WHLSLBaseFunctionAttribute.h */,
> > +				C21BF6FA21CD89BE00227979 /* WHLSLBaseSemantic.h */,
> > +				C21BF71E21CD89DC00227979 /* WHLSLBlock.h */,
> > +				C21BF6F621CD89B700227979 /* WHLSLBooleanLiteral.h */,
> > +				C21BF71A21CD89D800227979 /* WHLSLBreak.h */,
> > +				C2138A1321DDECD300F516BA /* WHLSLBuiltInSemantic.cpp */,
> > +				C21BF72221CD89DF00227979 /* WHLSLBuiltInSemantic.h */,
> > +				C21BF71621CD89D500227979 /* WHLSLCallExpression.h */,
> 
> What is this change?

I think a WHLSL patch didn't get processed by an upload script to order those in the project file. Wenson came across the same diff when he tried making a WebCore patch earlier.

> > Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63
> > +    virtual void useResource(MTLResource *, unsigned long) = 0;
> > +
> > +    // Render command encoder methods.
> > +    virtual void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) { }
> > +    virtual void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) { }
> 
> MTLFoo are ObjC types, so should be MTLFoo* not MTLFoo *.
> 
> > Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:69
> > +    void useResource(MTLResource *, unsigned long) final;
> > +    void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) final;
> > +    void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) final;
> 
> Ditto.

I think you have this jumbled with C++.

[Justin Fan]

Created attachment 359523 [details]
Patch for landing

[Dean Jackson]

Comment on attachment 359510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359510&action=review

>>> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63
>>> +    virtual void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) { }
>> 
>> MTLFoo are ObjC types, so should be MTLFoo* not MTLFoo *.
> 
> I think you have this jumbled with C++.

I do. 100% wrong.

[EWS Watchlist]

Attachment 359523 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:60:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 359523 [details]
Patch for landing

Rejecting attachment 359523 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 359523, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 10 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 15.0 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 22.5 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 33.75 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 50.625 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 75.9375 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 113.90625 seconds...
Logging in as [e-mail]...
Received HTTP status 500 loading "None".  Retrying in 170.859375 seconds...
Logging in as [e-mail]...
Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/download.py", line 180, in execute
    patches = self._fetch_list_of_patches_to_process(options, args, tool)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/download.py", line 224, in _fetch_list_of_patches_to_process
    patch = tool.bugs.fetch_attachment(patch_id, throw_on_access_error=True)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 552, in fetch_attachment
    bug_id = self.bug_id_for_attachment_id(attachment_id, throw_on_access_error)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 530, in bug_id_for_attachment_id
    return NetworkTransaction().run(lambda: self.get_bug_id_for_attachment_id(attachment_id, throw_on_access_error))
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/networktransaction.py", line 57, in run
    self._check_for_timeout()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/net/networktransaction.py", line 67, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout: NetworkTimeout

Full output: https://webkit-queues.webkit.org/results/10800438

[Justin Fan]

Created attachment 359541 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359541 [details]
Patch for landing

Clearing flags on attachment: 359541

Committed r240180: <https://trac.webkit.org/changeset/240180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

looks like the new test added in https://trac.webkit.org/changeset/240180/webkit 

webgpu/buffer-resource-triangles.html

is failing constantly. diff shows the drawn square is red. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgpu%2Fbuffer-resource-triangles.html

Diff:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r240251%20(8970)/webgpu/buffer-resource-triangles-diffs.html

[Truitt Savell]

Is there any new progress on this test issue?

[Justin Fan]

(In reply to Truitt Savell from comment #28)
> Is there any new progress on this test issue?

I haven't reproduced the failure on either of my Mac devices, but I have one more thing I want to try. Unfortunately it took me a while to update my laptop (the only device I own with an intel gpu) back to building ToT.

[Ryan Haddad]

(In reply to Justin Fan from comment #29)
> (In reply to Truitt Savell from comment #28)
> > Is there any new progress on this test issue?
> 
> I haven't reproduced the failure on either of my Mac devices, but I have one
> more thing I want to try. Unfortunately it took me a while to update my
> laptop (the only device I own with an intel gpu) back to building ToT.
Filed https://bugs.webkit.org/show_bug.cgi?id=194182 to track the test failure since the change cannot be rolled out cleanly anymore.