Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 162 additions & 0 deletions spec/index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -1828,6 +1828,12 @@ A <dfn dfn>supported limits</dfn> object has a value for every limit defined by
<tr class=row-continuation><td colspan=4>
The maximum value for the arguments of
{{GPUComputePassEncoder/dispatchWorkgroups(workgroupCountX, workgroupCountY, workgroupCountZ)}}.

<tr><td><dfn>maxMultiDrawIndirectCount</dfn>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts here:

  • Shouldn't this also apply to the value in the drawCountBuffer?
  • Does this mean that after enabling the feature you still need to request a higher limit or else will be stuck to 1? It's workable but seems a bit weird.
  • Seems a bit weird that this could be 1 when the feature is not enabled. In fact it seems weird the limit exists at all when the feature isn't enabled...I don't think there precedent for feature related limits though.
  • @kvark had suggested in a previous comment to " lift the limit entirely...I.e. it becomes limited in the same way as maximum index count is limited." It's not clear to me how "maximum index count is limited", other than it's tied to the max buffer size. Maybe the feature should only be available when the implementation max count is >= maxStorageBufferBindingSize and then this limit goes away? Not sure how reasonable that is.

@rconde01 rconde01 Oct 26, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also apply to the value in the drawCountBuffer?

Realized this is already covered transitively since drawCountBuffer is already limited by maxDrawCount. Nm.

@kainino0x kainino0x Jul 22, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirtsu55 is working on this feature in Dawn and we got to talking about the limit. I am not sure we need one - if it's guaranteed to be large on all systems then we could just hardcode a limit, or derive a limit from maxBufferSize.

  • Vulkan: has a limit but it's always either 1 or huge, so when the feature is available we could just have a hardcoded limit as high as 2**30.
  • D3D12: not sure yet, currently investingating.
  • Metal: There is already a maxBufferSize which imposes some limit. On weaker devices like old iPhones (the ones where maxBufferSize is 256MiB) there's a good chance this is the limiting factor. However the limiting factor wouldn't be the size on the indirect buffer (256MiB/16B=16.6M draws or 256MiB/20B=13.4M indexed draws) but the size of the indirect command buffer (ICB) that we generate internally in our draw call validation/rerecording shader. There is a maxCommandCount when you allocate an ICB, but we haven't found any documented limit on how large this can be. We don't know what the size of a draw command is so we can't seem to derive it from maxBufferLength - maybe in practice it's something like maxBufferLength / 32 bytes (256MiB/32B = 8.3M). We will probably try probing the Metal validation layer, but @mwyrzykowski @litherum would you be able to tell us how to determine this limit - maybe what the validation layer checks and/or whether it would be large enough on all target systems (#1069) that we can hardcode it?

@mwyrzykowski mwyrzykowski Jul 23, 2024 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kainino0x The metal validation layer does not appear to validate maxCommandCount specifically, though you may run into validation errors due to the maxBufferLength.

It appears we need the following to hold device.maxBufferLength <= 40 + maxCommandCount * ((2 + 2 * desc.maxVertexBufferBindCount + desc.maxFragmentBufferBindCount) * 8), where desc is the ICB descriptor.

I will need to check if that is strictly for validation or applies in general, we could start there as a limit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwyrzykowski thanks! Since multidraw will always inherit vertex/fragment buffers, those should be 0, simplifying to 40 + maxCommandCount * 2 * 8. (Also, did you mean the reverse, ... <= maxBufferLength?)

Just want to verify that 16 bytes per command is correct. That is too small for an indexed indirect call (20 bytes) so I'm guessing it is 2 pointers, one which points to the indexed indirect parameters allocated in some other buffer. If so, does the size of that buffer impose a limit? Or will it split into multiple buffers if it doesn't fit?

I'm also concerned that there will be three huge allocations, and we'll run out of memory anyway: the user's draw params, the ICB full of pointers, and Metal's internal allocation(s) that hold draw params.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great question and I don't think we have an answer for it. Maybe it would be possible to expand multidraw calls in bundles out, into allocation space for many single-draw calls, which we can write to later? But that would leave (maxDrawCount - actual draw count) of unused draw calls in the bundle which is probably very inefficient.

Maybe we have to say multidraw is not supported in bundles to start.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would certainly be easier, limiting this functionality to GPURenderPassEncoder

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kainino0x What should we do about the maxMultiDrawIndirectCount limit?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to put it out there:

From my investigation of D3D12 in WebGPU the limiting factor is the buffer size. I tried counts up to 2^26 in a native D3D12 app, it does not work. Vulkan has a property and Metal is untested.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I'd propose giving a hardcoded value of 10M or something to punt again on adding limits that are part of features (#4613). Unless we think that will be too small.

Of course we need to test that, and we also need to check that setting maxDrawCount=10M and having a small draw count in the drawCountBuffer can perform passably well. (It should, though in Dawn we need to fix https://crbug.com/367394543.)

<td>{{GPUSize32}} <td>[=limit class/maximum=] <td>1
<tr class=row-continuation><td colspan=4>
The maximum value for the `maxDrawCount` argument in
{{GPURenderCommandsMixin/multiDrawIndirect()}} and {{GPURenderCommandsMixin/multiDrawIndexedIndirect()}}.
</table>

<h5 id=gpusupportedlimits data-dfn-type=interface>`GPUSupportedLimits`
Expand Down Expand Up @@ -1874,6 +1880,7 @@ interface GPUSupportedLimits {
readonly attribute unsigned long maxComputeWorkgroupSizeY;
readonly attribute unsigned long maxComputeWorkgroupSizeZ;
readonly attribute unsigned long maxComputeWorkgroupsPerDimension;
readonly attribute unsigned long maxIndirectDrawCount;
};
</script>

Expand Down Expand Up @@ -2847,6 +2854,7 @@ enum GPUFeatureName {
"rg11b10ufloat-renderable",
"bgra8unorm-storage",
"float32-filterable",
"multi-draw-indirect",
};
</script>

Expand Down Expand Up @@ -11516,6 +11524,15 @@ interface mixin GPURenderCommandsMixin {

undefined drawIndirect(GPUBuffer indirectBuffer, GPUSize64 indirectOffset);
undefined drawIndexedIndirect(GPUBuffer indirectBuffer, GPUSize64 indirectOffset);

undefined multiDrawIndirect(GPUBuffer indirectBuffer, GPUSize64 indirectOffset,
GPUSize32 maxDrawCount,
optional GPUBuffer drawCountBuffer = null,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from @kainino0x: nit for editors: need to check if these GPUBuffers need to be nullable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To match feature parity with D3D12 and Vulkan the drawCountBuffer should be nullable, in which case maxDrawCount becomes the draw count.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question was about optional vs nullable

@kainino0x kainino0x Jul 22, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer this old question I think we should make it optional and not nullable. That should still allow null.

Suggested change
optional GPUBuffer drawCountBuffer = null,
optional GPUBuffer drawCountBuffer,

Allowing any of these (I believe) and combinations thereof:

pass.multiDrawIndirect(indirectBuffer, 0, 100);
pass.multiDrawIndirect(indirectBuffer, 0, 100, null);
pass.multiDrawIndirect(indirectBuffer, 0, 100, null, 0);
pass.multiDrawIndirect(indirectBuffer, 0, 100, null, null);
pass.multiDrawIndirect(indirectBuffer, 0, 100, undefined);
pass.multiDrawIndirect(indirectBuffer, 0, 100, undefined, 0);
pass.multiDrawIndirect(indirectBuffer, 0, 100, undefined, undefined);

optional GPUSize64 drawCountOffset = 0);
undefined multiDrawIndexedIndirect(GPUBuffer indirectBuffer, GPUSize64 indirectOffset,
GPUSize32 maxDrawCount,
optional GPUBuffer drawCountBuffer = null,
optional GPUSize64 drawCountOffset = 0);
};
</script>

Expand Down Expand Up @@ -11985,6 +12002,132 @@ It must only be included by interfaces which also include those mixins.
with the states from |passState| and |renderState|.
</div>
</div>

: <dfn>multiDrawIndirect(indirectBuffer, indirectOffset, maxDrawCount, drawCountBuffer, drawCountOffset)</dfn>
::
Draws primitives using an array of parameters read from a {{GPUBuffer}}.
See [[#rendering-operations]] for the detailed specification.

This method is defined if and only if the {{GPUFeatureName/"multi-draw-indirect"}} [=feature=] is enabled.

@kainino0x kainino0x Oct 24, 2023 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the check I was missing above.

Just an editorial thing but we'll need to move this to a content-timeline check that throws "TypeError" if the feature isn't enabled, as WebIDL doesn't let us dynamically decide whether to define something or not.


The <dfn dfn for=>indirect multiDraw parameters</dfn> encoded in the |indirectBuffer| must be zero or more
tightly packed blocks of **four 32-bit unsigned integer values (16 bytes total)**, given in the same
order as the arguments for {{GPURenderCommandsMixin/draw()}}. For example:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should call out here that indirect-first-instance is not required to use firstInstance here, since it is not obvious.

<pre highlight="js">
let drawIndirectParameters = new Uint32Array(8);
// first draw
drawIndirectParameters[0] = vertexCount0;
drawIndirectParameters[1] = instanceCount0;
drawIndirectParameters[2] = firstVertex0;
drawIndirectParameters[3] = firstInstance0;
// second draw
drawIndirectParameters[4] = vertexCount1;
drawIndirectParameters[5] = instanceCount1;
drawIndirectParameters[6] = firstVertex1;
drawIndirectParameters[7] = firstInstance1;
</pre>

The <dfn dfn for=>indirect multiDraw count</dfn> encoded in the |drawCountBuffer| must be a **32-bit unsigned
integer** giving the number of draws to issue from the |indirectBuffer|.

<div algorithm="GPURenderCommandsMixin.multiDrawIndirect">
**Called on:** {{GPURenderCommandsMixin}} this.

**Arguments:**
<pre class=argumentdef for="GPURenderCommandsMixin/multiDrawIndirect(indirectBuffer, indirectOffset, maxDrawCount, drawCountBuffer, drawCountOffset)">
|indirectBuffer|: Buffer containing the [=indirect multiDraw parameters=].
|indirectOffset|: Offset in bytes into |indirectBuffer| where the drawing data begins.
|maxDrawCount|: Maximum number of draws or exact number of draws if |drawCountBuffer| is `null`.
|drawCountBuffer|: Buffer containing the [=indirect multiDraw count=].
|drawCountOffset|: Offset in bytes into |drawCountBuffer| where the [=indirect multiDraw count=] is located.
</pre>

**Returns:** {{undefined}}

Issue the following steps on the [=Device timeline=] of |this|.{{GPUObjectBase/[[device]]}}:
<div class=device-timeline>
1. If any of the following conditions are unsatisfied, make |this| [=invalid=] and stop.
<div class=validusage>
- It is [$valid to draw$] with |this|.
- |indirectBuffer| is [$valid to use with$] |this|.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Opening a thread here because the threads on this issue still track all the things we need to do before landing this feature in the spec.)

According to #1354 (comment) there may be limitations on what can be done on a command buffer in Metal if supportIndirectCommandBuffers is set to true. If so we may need to account for them in the spec. When we get a chance we'll look into the issue/repro on the Chromium issue and see if we think it needs to be accounted for in the spec. (Or maybe someone can find out whether wgpu has this problem and has already found a limitation or workaround.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kainino0x WebKit always sets supportIndirectCommandBuffers to YES for all PSOs and we have no such issue problem running three.js's examples - https://github.com/search?q=repo%3AWebKit%2FWebKit%20supportIndirectCommandBuffers&type=code

I think it is because our implementation is based on Argument Buffers, if Chrome is planning to adopt that then that would be an option without changing the specification.

But setting supportIndirectCommandBuffers on the PSO does have a performance penalty, so I wouldn't rule out a spec change if we don't want to always enable this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's super useful info, thank you!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One such option would be extending GPUShaderModuleCompilationHint to indicate whether or not an entry point from a given shader module will be used with MTLIndirectCommandBuffer

If we want to explore that route, it would be nice to apply this to GPURenderBundle as well. I.e., if you set the flag to 0 indicating you will not use ICBs with a given PSO then you can not call executeBundles(...) from the RenderPassEncoder which uses that given entry point.

Then at GPURenderPipelineCreation, an implementation can lookup if the given entry point needs to set supportIndirectCommandBuffers to YES or not and potentially impact codegen as well

- |indirectBuffer|.{{GPUBuffer/[[usage]]}} contains {{GPUBufferUsage/INDIRECT}}.
- |indirectOffset| + sizeof([=indirect multiDraw parameters=]) &le;
|indirectBuffer|.{{GPUBuffer/[[size]]}}.
- |indirectOffset| is a multiple of 4.
- |maxDrawCount| &le; limits.{{supported limits/maxMultiDrawIndirectCount}}.
- |drawCountBuffer| is [$valid to use with$] |this|.
- |drawCountBuffer|.{{GPUBuffer/[[usage]]}} contains {{GPUBufferUsage/INDIRECT}}.
- |drawCountOffset| + sizeof([=indirect multiDraw count=]) &le;
|drawCountBuffer|.{{GPUBuffer/[[size]]}}.
- |drawCountOffset| is a multiple of 4.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to allow any value of drawCountOffset if drawCountBuffer is not set?

@kainino0x kainino0x Sep 24, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just keep the validation, but we could also consider using overloads instead so that we can never have a drawCountOffset when there is no drawCountBuffer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what we'll do here but two assorted notes:

  • setVertexBuffer still validates the buffer offset when buffer is null
  • I like the overload solution best but we should think about what it will look like in languages without overloads (C webgpu.h). I think we currently only have one overloaded method in the API (setBindGroup) and that overload doesn't apply to C at all.
    • Two functions? Merge into one function where if drawCountBuffer is null, drawCountOffset {is ignored,must be 0,etc}

</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When implementing this for D3D12, the driver / validation layer checks if maxDrawCount exceeds the range of the indirect buffer. Specifically it returns E_INVALIDARG when closing the command list. We should add this to the validation. Something along the lines of: indirectOffset + sizeof(indirectDrawParams) * maxDrawCount should not be out of bounds of indirectBuffer

1. Add |indirectBuffer| and |drawCountBuffer| (if given) to the [=usage scope=] as [=internal usage/input=].
</div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
</div>
`1. Increment |this|.{{GPURenderCommandsMixin/[[drawCount]]}} by |maxDrawCount|.`
</div>

We won't know the exact number of draw calls issued but maxDrawCount is guaranteed to be the maximum so drawCount should be incremented by it.

</div>

: <dfn>multiDrawIndexedIndirect(indirectBuffer, indirectOffset, maxDrawCount, drawCountBuffer, drawCountOffset)</dfn>
::
Draws indexed primitives using an array of parameters read from a {{GPUBuffer}}.
See [[#rendering-operations]] for the detailed specification.

The <dfn dfn for=>indirect multiDrawIndexed parameters</dfn> encoded in the |indirectBuffer| must be zero or more
tightly packed blocks of **five 32-bit unsigned integer values (20 bytes total)**, given in
the same order as the arguments for {{GPURenderCommandsMixin/drawIndexed()}}. For example:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

<pre highlight="js">
let drawIndexedIndirectParameters = new Uint32Array(10);
// first draw
drawIndexedIndirectParameters[0] = indexCount0;
drawIndexedIndirectParameters[1] = instanceCount0;
drawIndexedIndirectParameters[2] = firstIndex0;
drawIndexedIndirectParameters[3] = baseVertex0;
drawIndexedIndirectParameters[4] = firstInstance0;
// second draw
drawIndexedIndirectParameters[5] = indexCount1;
drawIndexedIndirectParameters[6] = instanceCount1;
drawIndexedIndirectParameters[7] = firstIndex1;
drawIndexedIndirectParameters[8] = baseVertex1;
drawIndexedIndirectParameters[9] = firstInstance1;
</pre>

The <dfn dfn for=>indirect multiDrawIndexed count</dfn> encoded in the |drawCountBuffer| must be a **32-bit unsigned
integer** giving the number of draws to issue from the |indirectBuffer|.

<div algorithm="GPURenderCommandsMixin.multiDrawIndexedIndirect">
**Called on:** {{GPURenderCommandsMixin}} this.

**Arguments:**
<pre class=argumentdef for="GPURenderCommandsMixin/multiDrawIndexedIndirect(indirectBuffer, indirectOffset, maxDrawCount, drawCountBuffer, drawCountOffset)">
|indirectBuffer|: Buffer containing the [=indirect multiDrawIndexed parameters=].
|indirectOffset|: Offset in bytes into |indirectBuffer| where the drawing data begins.
|maxDrawCount|: Maximum number of draws or exact number of draws if |drawCountBuffer| is `null`.
|drawCountBuffer|: Buffer containing the [=indirect multiDrawIndexed count=].
|drawCountOffset|: Offset in bytes into |drawCountBuffer| where the [=indirect multiDrawIndexed count=] is located.
</pre>

**Returns:** {{undefined}}

Issue the following steps on the [=Device timeline=] of |this|.{{GPUObjectBase/[[device]]}}:
<div class=device-timeline>
1. If any of the following conditions are unsatisfied, make |this| [=invalid=] and stop.
<div class=validusage>
- It is [$valid to draw indexed$] with |this|.
- |indirectBuffer| is [$valid to use with$] |this|.
- |indirectBuffer|.{{GPUBuffer/[[usage]]}} contains {{GPUBufferUsage/INDIRECT}}.
- |indirectOffset| + sizeof([=indirect multiDrawIndexed parameters=]) &le;
|indirectBuffer|.{{GPUBuffer/[[size]]}}.
- |indirectOffset| is a multiple of 4.
- |maxDrawCount| &le; limits.{{supported limits/maxMultiDrawIndirectCount}}.
- |drawCountBuffer| is [$valid to use with$] |this|.
- |drawCountBuffer|.{{GPUBuffer/[[usage]]}} contains {{GPUBufferUsage/INDIRECT}}.
- |drawCountOffset| + sizeof([=indirect multiDrawIndexed count=]) &le;
|drawCountBuffer|.{{GPUBuffer/[[size]]}}.
- |drawCountOffset| is a multiple of 4.
</div>
1. Add |indirectBuffer| and |drawCountBuffer| (if given) to the [=usage scope=] as [=internal usage/input=].
</div>
</div>
</dl>

<div algorithm>
Expand Down Expand Up @@ -15350,6 +15493,25 @@ This feature adds no [=optional API surfaces=].
Makes textures with formats {{GPUTextureFormat/"r32float"}}, {{GPUTextureFormat/"rg32float"}}, and
{{GPUTextureFormat/"rgba32float"}} [=filterable=].

<h3 id=multi-draw-indirect data-dfn-type=enum-value data-dfn-for=GPUFeatureName>`"multi-draw-indirect"`
<span id=dom-gpufeaturename-multi-draw-indirect></span>
</h3>

Allows the use of the {{GPURenderCommandsMixin/multiDrawIndirect()}} and
{{GPURenderCommandsMixin/multiDrawIndexedIndirect()}} methods.

**Feature Methods**

The following methods are supported if and only if the {{GPUFeatureName/"multi-draw-indirect"}}
[=feature=] is enabled.

<dl>
: {{GPURenderCommandsMixin}}
::
* {{GPURenderCommandsMixin/multiDrawIndirect()}}
* {{GPURenderCommandsMixin/multiDrawIndexedIndirect()}}
</dl>

# Appendices # {#appendices}

## Texture Format Capabilities ## {#texture-format-caps}
Expand Down