Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 6dca6330dba27ff589985db50e5e259f71a44f61
https://github.com/WebKit/WebKit/commit/6dca6330dba27ff589985db50e5e259f71a44f61
Author: Ahmad Saleem <[email protected]>
Date: 2026-08-17 (Mon, 17 Aug 2026)
Changed paths:
A
LayoutTests/fast/webgpu/draw-indirect-repeated-shared-args-buffer-expected.txt
A LayoutTests/fast/webgpu/draw-indirect-repeated-shared-args-buffer.html
M Source/WebGPU/WebGPU/Buffer.h
M Source/WebGPU/WebGPU/Buffer.mm
M Source/WebGPU/WebGPU/RenderPassEncoder.h
M Source/WebGPU/WebGPU/RenderPassEncoder.mm
Log Message:
-----------
[WebGPU] drawIndirect/drawIndexedIndirect clamp into a single per-Buffer
scratch slot, racing every other indirect draw in the same render pass
https://bugs.webkit.org/show_bug.cgi?id=321876
rdar://185057290
Reviewed by Mike Wyrzykowski.
Repeated drawIndirect/drawIndexedIndirect in one render pass could hang the GPU
unrecoverably, because every clamping draw against a given arguments buffer
wrote
its clamped arguments into the same scratch slot and then fetched them from it
with no ordering against the neighbouring draws doing the same.
RenderPassEncoder::clampIndirectBufferToValidValues and
clampIndirectIndexBufferToValidValues wrote into Buffer::indirectBuffer() /
Buffer::indirectIndexedBuffer(): one scratch slot per Buffer, sized for exactly
one arguments struct, returned at offset 0. Every clamping draw was therefore
issued as
drawPrimitives:indirectBuffer:<sameSlot> indirectBufferOffset:0
The clamp path is entered whenever computeMininumVertexInstanceCount can lower
either minimum, i.e. whenever the pipeline declares a required vertex buffer
layout with a positive stride and at least one attribute -- whether or not the
shader ever reads it. Zero-attribute and undefined-stride layouts are dropped in
createVertexDescriptor before requiredBufferIndices is populated and cannot
lower
a minimum, so they take the early-out that returns the application's own buffer
and offset.
emitMemoryBarrier() emits a vertex->vertex MTLBarrierScopeBuffers barrier
between
a draw's clamp dispatch and that same draw's indirect draw, which covers the
read-after-write within one draw. Nothing is emitted between draw N's indirect
argument fetch and draw N+1's clamp dispatch, and on Apple8 and later
memoryBarrierLimit is UINT32_MAX unless shader validation is enabled
(HardwareCapabilities.mm:264), so splitRenderPass() never fires and never
separates them either. Draw N+1's clamp vertex shader therefore stores to the
same bytes draw N is fetching its arguments from, with no ordering between them,
and MTLRenderStages has no way to name the argument fetch so no barrier can
express the constraint. Draw N can observe draw N+1's counts, or a torn mixture
of the two, since the clamp shader stores vertexCount, instanceCount,
vertexStart
and baseInstance as four separate stores. A large-instance-count draw issued
with
arguments its own bounds no longer describe is consistent with the reported AGX
tiler progress timeout. Apple7 gets a memoryBarrierLimit of 512 from mac2() and
capabilities merge with std::min, so splitRenderPass() does eventually fire
there, which bounds the window rather than closing it: the slot is still shared
by every draw between two splits.
Buffer::m_indirectCache was a single struct, not a map, keyed on
(indirectOffset,
minVertexCount, minInstanceCount, drawType), so consecutive draws at different
offsets out of one arguments buffer missed it on every draw and every one of
them
re-ran the clamp into the shared slot.
The fix is to allocate per-draw scratch, which is what the two other clamp sites
in this file already do: clampIndexBufferToValidValues (the non-indirect indexed
path) uses Queue::newTemporaryBufferWithBytes directly, and the batched
executeBundles helpers added in 59b6d9a2d027 use newZeroedIndirectScratch.
newTemporaryBufferWithBytes bump-allocates a distinct 64-byte-aligned offset out
of a pooled buffer. Both direct-path clamps now do the same and return
(scratch, offset); the per-Buffer m_indirectBuffer / m_indirectIndexedBuffer
slots are removed. Metal requires indirectBufferOffset to be a multiple of 4, so
64-byte-aligned offsets satisfy it with headroom.
m_indirectCache is removed with them. A cache hit would have to return the same
scratch it validated, and the pooled allocator recycles, so the skip cannot be
kept as-is; every clamping indirect draw now runs one point dispatch, which is
the cost the batched path already accepts. Removing the skip also makes
Buffer::skippedDrawIndirectValidation, skippedDrawIndirectIndexedValidation,
takeSlowIndirectValidationPath and takeSlowIndirectIndexValidationPath dead:
they existed only to re-verify on the CPU at commit time when a skipped clamp
might have gone stale. The GPU clamp now always runs in the same command buffer
as the draw and always reads the current arguments. The non-indirect index
validation cache and takeSlowIndexValidationPath are untouched, and
m_mustTakeSlowIndexValidationPath still has a live reader in CommandEncoder.mm.
Because the recomputation cache no longer suppresses most of them, the
device-loss readback would otherwise run once per clamping indirect draw, each
time installing an addCompletedHandler plus two Queue::scheduleWork main-thread
hops (releaseCounterSampleBuffer schedules its own). The encoderHandle is
CommandEncoder::uniqueId(), identical for every draw in the pass, so the
per-draw retain/release of the counter sample buffer was redundant as well.
RenderPassEncoder::trackIndirectDeviceLostCheck now accumulates the scratch
records in a ThreadSafeRefCounted holder and installs a single completion
handler
on the first clamp of the pass, so N clamping draws cost one handler and one
scheduleWork pair instead of N and 2N. Appends can never race that handler:
RETURN_IF_FINISHED() rejects every draw once the parent encoder is finished, and
CommandEncoder::finish is the only commit path, so all appends precede commit.
Entries hold RetainPtr, keeping scratch alive independently of
CommandEncoder::addBuffer (which is a no-op unless shader validation is on,
since m_retainedBuffers is only allocated in that case).
drawIndirect and drawIndexedIndirect bounded the arguments struct against the
whole buffer length, which is no longer sufficient once the arguments live at a
non-zero offset in a pooled buffer; both now bound offset + sizeof(args) against
the length. A nil buffer from a failed clamp has length 0 and is still rejected,
which also replaces the removed !indirectBuffer early-out.
drawIndexedIndirect was not covered by the original report but shares the same
defect, and consumed both scratch slots.
The new layout test is a correctness test for the shape in question -- two
drawIndirect calls in one pass out of one arguments buffer, each selecting a
differently coloured triangle via firstVertex and scissored to its own pixel, so
a clobbered record renders the wrong colour. It is not a deterministic pre-patch
failure, because the underlying defect is a race; the reproduction attached to
the bug is the stronger signal.
* Source/WebGPU/WebGPU/Buffer.h:
(WebGPU::Buffer::buffer const):
(WebGPU::Buffer::indirectIndexedBuffer const): Deleted.
* Source/WebGPU/WebGPU/Buffer.mm:
(WebGPU::Buffer::Buffer):
(WebGPU::Buffer::indirectBufferInvalidated):
(WebGPU::Buffer::indirectBuffer const): Deleted.
(WebGPU::Buffer::takeSlowIndirectIndexValidationPath): Deleted.
(WebGPU::verifyIndirectBufferData): Deleted.
(WebGPU::Buffer::takeSlowIndirectValidationPath): Deleted.
(WebGPU::Buffer::skippedDrawIndirectIndexedValidation): Deleted.
(WebGPU::Buffer::skippedDrawIndirectValidation): Deleted.
(WebGPU::Buffer::indirectBufferRequiresRecomputation const): Deleted.
(WebGPU::Buffer::indirectIndexedBufferRequiresRecomputation const): Deleted.
(WebGPU::Buffer::indirectBufferRecomputed): Deleted.
(WebGPU::Buffer::indirectIndexedBufferRecomputed): Deleted.
* Source/WebGPU/WebGPU/RenderPassEncoder.h:
* Source/WebGPU/WebGPU/RenderPassEncoder.mm:
(WebGPU::RenderPassEncoder::trackIndirectDeviceLostCheck):
(WebGPU::RenderPassEncoder::newZeroedIndirectScratch):
(WebGPU::RenderPassEncoder::clampIndirectIndexBufferToValidValues):
(WebGPU::RenderPassEncoder::clampIndirectBufferToValidValues):
(WebGPU::RenderPassEncoder::clampIndirectBufferDispatchBatched):
(WebGPU::RenderPassEncoder::clampIndirectIndexBufferDispatch1Batched):
(WebGPU::RenderPassEncoder::drawIndexedIndirect):
(WebGPU::RenderPassEncoder::drawIndirect):
(WebGPU::checkForIndirectDrawDeviceLost): Deleted.
Canonical link: https://commits.webkit.org/319317@main
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
