Skip to content

Commit fa56a58

Browse files
committed
GPU Metal: make command-buffer failures fatal
A failed Metal command buffer (e.g. killed by the macOS GPU watchdog, kIOGPUCommandBufferCallbackErrorImpactingInteractivity / ...Timeout) was only logged when the stream was drained, and the reconstruction carried on without its work: on 20 kHz Pb-Pb time frames this gave silently wrong outputs (271,967-296,957 tracks vs 297,669 on CPU for the same TF). Worse, a failed buffer never signals the events it was to signal, so later buffers waiting for them on the GPU, and the host waiting for those, blocked forever: one such run hung at 0% CPU. Signal the stream-order event and the recorded event of a buffer from its completion handler when it fails, so everything queued behind it completes, and fail in DrainStream through GPUChkErr, like the CUDA and HIP backends do on a failed synchronisation ("GPU Backend Failure"). Failures seen while the device is torn down are only logged, so the destructor does not throw. pbpb30 (M1 Max, abp.sh, 3 rounds, --runs 10): 966.7 ms/ev before, 962.5 after; Metal dumps byte-identical before and after; sim5pp dump still identical to CPU.
1 parent bc5ed44 commit fa56a58

2 files changed

Lines changed: 25 additions & 2 deletions

File tree

‎GPU/GPUTracking/Base/metal/GPUReconstructionMetal.mm‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,8 @@ uint32_t metalGPUCoreCount()
212212

213213
int32_t GPUReconstructionMetal::ExitDevice_Runtime()
214214
{
215-
// Tear down Metal
215+
// Tear down Metal, also after a failure, without failing again
216+
mInternals->exiting = true;
216217
SynchronizeGPU();
217218

218219
for (uint32_t i = 0; i < mEvents.size(); i++) {
@@ -380,7 +381,9 @@ MetalMemRef metalResolve(const GPUReconstructionMetalInternals* internals, const
380381
// Does not change the retain count, so it's important we manage
381382
// the lifetime of the events outside here.
382383
auto mtlEvent = (__bridge id<MTLSharedEvent>)(ev->get<void*>());
383-
[cb encodeSignalEvent:mtlEvent value:EventValue(*ev, true)];
384+
const uint64_t value = EventValue(*ev, true);
385+
[cb encodeSignalEvent:mtlEvent value:value];
386+
metalSignalOnError(cb, mtlEvent, value);
384387
CommitStream(stream);
385388
}
386389

@@ -392,6 +395,7 @@ MetalMemRef metalResolve(const GPUReconstructionMetalInternals* internals, const
392395
// in constant memory. So each buffer of a stream waits for the previous one.
393396
const uint64_t value = ++mInternals->streamOrderValue[stream];
394397
[cb encodeSignalEvent:mInternals->streamOrder[stream] value:value];
398+
metalSignalOnError(cb, mInternals->streamOrder[stream], value);
395399
[cb commit];
396400
// Hand the committed buffer over to the stream's submission list, which owns
397401
// it until it is waited for, and open a fresh one to encode the next work in.
@@ -420,16 +424,22 @@ MetalMemRef metalResolve(const GPUReconstructionMetalInternals* internals, const
420424
const auto t0 = std::chrono::steady_clock::now();
421425
while (!mInternals->submittedBuffers[stream].empty() && !(done = mInternals->streamOrder[stream].signaledValue >= mInternals->streamOrderValue[stream]) && std::chrono::steady_clock::now() - t0 < std::chrono::milliseconds(20)) {
422426
}
427+
int64_t error = 0;
423428
for (id<MTLCommandBuffer> cb : mInternals->submittedBuffers[stream]) {
424429
if (!done) {
425430
[cb waitUntilCompleted];
426431
}
427432
if (cb.status == MTLCommandBufferStatusError) {
428433
GPUError("Metal command buffer on stream %d failed: %s", stream, [[cb.error description] UTF8String]);
434+
error = error ? error : cb.error.code;
429435
}
430436
[cb release];
431437
}
432438
mInternals->submittedBuffers[stream].clear();
439+
// The work of a failed buffer is lost, so the results are wrong: fail like the other backends.
440+
if (error && !mInternals->exiting) {
441+
GPUChkErr(error);
442+
}
433443
}
434444

435445
void GPUReconstructionMetal::SynchronizeGPU()

‎GPU/GPUTracking/Base/metal/GPUReconstructionMetalIncludesHost.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ struct GPUReconstructionMetalInternals {
6666
// collects the stream's work until something has to wait for it.
6767
std::array<id<MTLFence>, constants::GPU_MAX_STREAMS> streamFence;
6868
std::array<bool, constants::GPU_MAX_STREAMS> streamHasWork;
69+
// Set while the device is torn down, when a failed command buffer is no longer fatal
70+
bool exiting = false;
6971

7072
std::vector<id<MTLFunction>> functions;
7173
std::vector<id<MTLComputePipelineState>> pipelines;
@@ -90,6 +92,17 @@ inline void metalBeginEncoder(GPUReconstructionMetalInternals* internals, E enco
9092
{
9193
[encoder waitForFence:internals->streamFence[stream]];
9294
}
95+
// A failed command buffer does not signal its events, and the work waiting for
96+
// them would wait forever: signal them from the host, so that everything
97+
// completes and the failure is reported when the host synchronises.
98+
inline void metalSignalOnError(id<MTLCommandBuffer> cb, id<MTLSharedEvent> event, uint64_t value)
99+
{
100+
[cb addCompletedHandler:^(id<MTLCommandBuffer> c) {
101+
if (c.status == MTLCommandBufferStatusError && event.signaledValue < value) {
102+
event.signaledValue = value;
103+
}
104+
}];
105+
}
93106
template <class E>
94107
inline void metalEndEncoder(GPUReconstructionMetalInternals* internals, E encoder, int32_t stream)
95108
{

0 commit comments

Comments
 (0)