From c9482119e317141022f308be19366744af83ab4a Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Tue, 27 May 2025 15:49:06 -0700 Subject: [PATCH 1/5] Additional encoder_state testcase --- .../validation/encoding/encoder_state.spec.ts | 85 ++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/src/webgpu/api/validation/encoding/encoder_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_state.spec.ts index 3c3cde00c48d..869c8fb8ea36 100644 --- a/src/webgpu/api/validation/encoding/encoder_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_state.spec.ts @@ -19,6 +19,7 @@ TODO: import { makeTestGroup } from '../../../../common/framework/test_group.js'; import { objectEquals } from '../../../../common/util/util.js'; import { AllFeaturesMaxLimitsGPUTest } from '../../../gpu_test.js'; +import * as vtu from '../validation_test_utils.js'; class F extends AllFeaturesMaxLimitsGPUTest { beginRenderPass(commandEncoder: GPUCommandEncoder, view: GPUTextureView): GPURenderPassEncoder { @@ -154,7 +155,7 @@ g.test('call_after_successful_finish') g.test('pass_end_none') .desc( ` - Test that ending a {compute,render} pass without ending the passes generates a validation error. + Test that finishing an encoder without ending a child {compute,render} pass generates a validation error. ` ) .paramsSubcasesOnly(u => u.combine('passType', ['compute', 'render']).combine('endCount', [0, 1])) @@ -247,3 +248,85 @@ g.test('pass_end_twice,render_pass_invalid') encoder.finish(); }); }); + +g.test('pass_begin_invalid_encoder') + .desc( + ` + Test that {compute,render} passes can still be opened on an invalid encoder. + ` + ) + .params(u => + u + .combine('pass0Type', ['compute', 'render']) + .combine('pass1Type', ['compute', 'render']) + .beginSubcases() + .combine('firstPassInvalid', [false, true]) + ) + .beforeAllSubcases(t => t.usesMismatchedDevice()) + .fn(t => { + t.skipIfDeviceDoesNotSupportQueryType('timestamp'); + + const { pass0Type, pass1Type, firstPassInvalid } = t.params; + + const view = t.createAttachmentTextureView(); + const mismatchedTexture = vtu.getDeviceMismatchedRenderTexture(t, 4); + const mismatchedView = mismatchedTexture.createView(); + + const querySet = t.trackForCleanup( + t.device.createQuerySet({ + type: 'timestamp', + count: 1, + }) + ); + + const timestampWrites = { + querySet, + beginningOfPassWriteIndex: 0, + }; + + const descriptor = { + timestampWrites, + }; + + const mismatchedQuerySet = t.trackForCleanup( + t.mismatchedDevice.createQuerySet({ + type: 'timestamp', + count: 1, + }) + ); + + const mismatchedTimestampWrites = { + querySet: mismatchedQuerySet, + beginningOfPassWriteIndex: 0, + }; + + const mismatchedDescriptor = { + timestampWrites: mismatchedTimestampWrites, + }; + + const encoder = t.device.createCommandEncoder(); + + let firstPass; + if (pass0Type === 'compute') { + firstPass = firstPassInvalid + ? encoder.beginComputePass(mismatchedDescriptor) + : encoder.beginComputePass(descriptor); + } else { + firstPass = firstPassInvalid + ? t.beginRenderPass(encoder, mismatchedView) + : t.beginRenderPass(encoder, view); + } + + // Ending an invalid pass invalidates the encoder + firstPass.end(); + + // Passes can still be opened on an invalid encoder + const secondPass = + pass1Type === 'compute' ? encoder.beginComputePass() : t.beginRenderPass(encoder, view); + + secondPass.end(); + + t.expectValidationError(() => { + encoder.finish(); + }, firstPassInvalid); + }); From 8fcd343b7b4b35df2ee0c45e509ca538733397bb Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Mon, 2 Jun 2025 17:31:15 -0700 Subject: [PATCH 2/5] Test pass but not encoder ended in `encoder_open_state` --- .../encoding/encoder_open_state.spec.ts | 30 +++++++++++-------- .../validation/encoding/encoder_state.spec.ts | 6 ++-- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts index 6e9181142cdc..3a7f2cab22af 100644 --- a/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts @@ -274,15 +274,13 @@ g.test('render_pass_commands') ` Test that functions of GPURenderPassEncoder generate a validation error if the encoder or the pass is already finished. - - - TODO: Consider testing: nothing before command, end before command, end+finish before command. ` ) .params(u => u .combine('command', kRenderPassEncoderCommands) .beginSubcases() - .combine('finishBeforeCommand', [false, true]) + .combine('finishBeforeCommand', ['no', 'pass', 'encoder']) ) .fn(t => { const { command, finishBeforeCommand } = t.params; @@ -305,8 +303,10 @@ g.test('render_pass_commands') const bindGroup = t.createBindGroupForTest(); - if (finishBeforeCommand) { + if (finishBeforeCommand !== 'no') { renderPass.end(); + } + if (finishBeforeCommand === 'encoder') { encoder.finish(); } @@ -404,23 +404,25 @@ g.test('render_pass_commands') break; case 'pushDebugGroup': { - encoder.pushDebugGroup('group'); + renderPass.pushDebugGroup('group'); } break; case 'popDebugGroup': { - encoder.popDebugGroup(); + renderPass.popDebugGroup(); } break; case 'insertDebugMarker': { - encoder.insertDebugMarker('marker'); + renderPass.insertDebugMarker('marker'); } break; default: unreachable(); } - }, finishBeforeCommand); + }, finishBeforeCommand !== 'no'); + // TODO(https://github.com/gpuweb/gpuweb/issues/5207): resolve whether this + // is correct or should be changed to `finishBeforeCommand === 'encoder'`. }); g.test('render_bundle_commands') @@ -524,15 +526,13 @@ g.test('compute_pass_commands') ` Test that functions of GPUComputePassEncoder generate a validation error if the encoder or the pass is already finished. - - - TODO: Consider testing: nothing before command, end before command, end+finish before command. ` ) .params(u => u .combine('command', kComputePassEncoderCommands) .beginSubcases() - .combine('finishBeforeCommand', [false, true]) + .combine('finishBeforeCommand', ['no', 'pass', 'encoder']) ) .fn(t => { const { command, finishBeforeCommand } = t.params; @@ -549,8 +549,10 @@ g.test('compute_pass_commands') const bindGroup = t.createBindGroupForTest(); - if (finishBeforeCommand) { + if (finishBeforeCommand !== 'no') { computePass.end(); + } + if (finishBeforeCommand === 'encoder') { encoder.finish(); } @@ -594,5 +596,7 @@ g.test('compute_pass_commands') default: unreachable(); } - }, finishBeforeCommand); + }, finishBeforeCommand !== 'no'); + // TODO(https://github.com/gpuweb/gpuweb/issues/5207): resolve whether this + // is correct or should be changed to `finishBeforeCommand === 'encoder'`. }); diff --git a/src/webgpu/api/validation/encoding/encoder_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_state.spec.ts index 869c8fb8ea36..6424cfe89c38 100644 --- a/src/webgpu/api/validation/encoding/encoder_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_state.spec.ts @@ -81,6 +81,8 @@ g.test('pass_end_invalid_order') const passes = [firstPass, secondPass]; for (const index of endPasses) { + // TODO(https://github.com/gpuweb/gpuweb/issues/5207): should a validation error + // be raised here if `!firstPassEnd && endPasses = [1, 0]`? passes[index].end(); } @@ -275,7 +277,7 @@ g.test('pass_begin_invalid_encoder') const querySet = t.trackForCleanup( t.device.createQuerySet({ type: 'timestamp', - count: 1, + count: 2, }) ); @@ -291,7 +293,7 @@ g.test('pass_begin_invalid_encoder') const mismatchedQuerySet = t.trackForCleanup( t.mismatchedDevice.createQuerySet({ type: 'timestamp', - count: 1, + count: 2, }) ); From 2cf8eabdadb192d24c34d29b419892da4c05bfb7 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 18 Jun 2025 17:20:24 -0700 Subject: [PATCH 3/5] Additional call_after_successful_finish tests --- .../validation/encoding/encoder_state.spec.ts | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/src/webgpu/api/validation/encoding/encoder_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_state.spec.ts index 6424cfe89c38..c8ddb5c878b3 100644 --- a/src/webgpu/api/validation/encoding/encoder_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_state.spec.ts @@ -98,7 +98,13 @@ g.test('call_after_successful_finish') .desc(`Test that encoding command after a successful finish generates a validation error.`) .params(u => u - .combine('callCmd', ['beginComputePass', 'beginRenderPass', 'insertDebugMarker']) + .combine('callCmd', [ + 'beginComputePass', + 'beginRenderPass', + 'finishAndSubmitFirst', + 'finishAndSubmitSecond', + 'insertDebugMarker', + ]) .beginSubcases() .combine('prePassType', ['compute', 'render', 'no-op']) .combine('IsEncoderFinished', [false, true]) @@ -115,8 +121,9 @@ g.test('call_after_successful_finish') pass.end(); } + let buffer; if (IsEncoderFinished) { - encoder.finish(); + buffer = encoder.finish(); } switch (callCmd) { @@ -129,6 +136,9 @@ g.test('call_after_successful_finish') t.expectValidationError(() => { pass.end(); }, IsEncoderFinished); + if (buffer) { + t.device.queue.submit([buffer]); + } } break; case 'beginRenderPass': @@ -140,16 +150,41 @@ g.test('call_after_successful_finish') t.expectValidationError(() => { pass.end(); }, IsEncoderFinished); + if (buffer) { + t.device.queue.submit([buffer]); + } + } + break; + case 'finishAndSubmitFirst': + t.expectValidationError(() => { + encoder.finish(); + }, IsEncoderFinished); + if (buffer) { + t.device.queue.submit([buffer]); + } + break; + case 'finishAndSubmitSecond': + { + let secondBuffer: GPUCommandBuffer; + t.expectValidationError(() => { + secondBuffer = encoder.finish(); + }, IsEncoderFinished); + t.expectValidationError(() => { + t.device.queue.submit([secondBuffer]); + }, IsEncoderFinished); } break; case 'insertDebugMarker': t.expectValidationError(() => { encoder.insertDebugMarker(''); }, IsEncoderFinished); + if (buffer) { + t.device.queue.submit([buffer]); + } break; } - if (!IsEncoderFinished) { + if (!IsEncoderFinished && !callCmd.startsWith("finish")) { encoder.finish(); } }); From 674876b97c3c78002b8f2541c6f91f3edf81cfe6 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 25 Jun 2025 17:34:10 -0700 Subject: [PATCH 4/5] Lint fix --- src/webgpu/api/validation/encoding/encoder_state.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webgpu/api/validation/encoding/encoder_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_state.spec.ts index c8ddb5c878b3..1a7f04bab4b6 100644 --- a/src/webgpu/api/validation/encoding/encoder_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_state.spec.ts @@ -184,7 +184,7 @@ g.test('call_after_successful_finish') break; } - if (!IsEncoderFinished && !callCmd.startsWith("finish")) { + if (!IsEncoderFinished && !callCmd.startsWith('finish')) { encoder.finish(); } }); From 5c1716b132eae2b1d1b37adf0726eedf0172986e Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 25 Jun 2025 17:44:52 -0700 Subject: [PATCH 5/5] Move TODOs to description --- .../validation/encoding/encoder_open_state.spec.ts | 12 ++++++++---- .../api/validation/encoding/encoder_state.spec.ts | 5 +++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts index 3a7f2cab22af..06ddc88a0576 100644 --- a/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_open_state.spec.ts @@ -274,6 +274,10 @@ g.test('render_pass_commands') ` Test that functions of GPURenderPassEncoder generate a validation error if the encoder or the pass is already finished. + + TODO(https://github.com/gpuweb/gpuweb/issues/5207): Resolve whether the error condition + \`finishBeforeCommand !== 'no'\` is correct, or should be changed to + \`finishBeforeCommand === 'encoder'\`. ` ) .params(u => @@ -421,8 +425,6 @@ g.test('render_pass_commands') unreachable(); } }, finishBeforeCommand !== 'no'); - // TODO(https://github.com/gpuweb/gpuweb/issues/5207): resolve whether this - // is correct or should be changed to `finishBeforeCommand === 'encoder'`. }); g.test('render_bundle_commands') @@ -526,6 +528,10 @@ g.test('compute_pass_commands') ` Test that functions of GPUComputePassEncoder generate a validation error if the encoder or the pass is already finished. + + TODO(https://github.com/gpuweb/gpuweb/issues/5207): Resolve whether the error condition + \`finishBeforeCommand !== 'no'\` is correct, or should be changed to + \`finishBeforeCommand === 'encoder'\`. ` ) .params(u => @@ -597,6 +603,4 @@ g.test('compute_pass_commands') unreachable(); } }, finishBeforeCommand !== 'no'); - // TODO(https://github.com/gpuweb/gpuweb/issues/5207): resolve whether this - // is correct or should be changed to `finishBeforeCommand === 'encoder'`. }); diff --git a/src/webgpu/api/validation/encoding/encoder_state.spec.ts b/src/webgpu/api/validation/encoding/encoder_state.spec.ts index 1a7f04bab4b6..40e746511d18 100644 --- a/src/webgpu/api/validation/encoding/encoder_state.spec.ts +++ b/src/webgpu/api/validation/encoding/encoder_state.spec.ts @@ -52,6 +52,9 @@ g.test('pass_end_invalid_order') ` Test that beginning a {compute,render} pass before ending the previous {compute,render} pass causes an error. + + TODO(https://github.com/gpuweb/gpuweb/issues/5207): Resolve whether a validation error + should be raised immediately if '!firstPassEnd && endPasses = [1, 0]'. ` ) .params(u => @@ -81,8 +84,6 @@ g.test('pass_end_invalid_order') const passes = [firstPass, secondPass]; for (const index of endPasses) { - // TODO(https://github.com/gpuweb/gpuweb/issues/5207): should a validation error - // be raised here if `!firstPassEnd && endPasses = [1, 0]`? passes[index].end(); }