[WebGPU] WebGPUProgrammablePassEncoder::setBindGroup prototype
Created attachment 359192 [details] Patch
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.
<rdar://problem/47296678>
Very rough draft. Not ready for review-review, but comments are appreciated.
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
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.
Created attachment 359337 [details] Patch
Created attachment 359350 [details] Patch
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.
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.
(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.
Created attachment 359503 [details] Patch
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.
Created attachment 359508 [details] Patch
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.
Created attachment 359510 [details] Patch
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.
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.
(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++.
Created attachment 359523 [details] Patch for landing
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.
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.
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 commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 10 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 15.0 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 22.5 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 33.75 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 50.625 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 75.9375 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 113.90625 seconds... Logging in as commit-queue@webkit.org... Received HTTP status 500 loading "None". Retrying in 170.859375 seconds... Logging in as commit-queue@webkit.org... 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
Created attachment 359541 [details] Patch for landing
Comment on attachment 359541 [details] Patch for landing Clearing flags on attachment: 359541 Committed r240180: <https://trac.webkit.org/changeset/240180>
All reviewed patches have been landed. Closing bug.
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
Is there any new progress on this test issue?
(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.
(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.