OpenVX 1.3.2: implement pipelining, streaming, and event-queue extension - #1722
OpenVX 1.3.2: implement pipelining, streaming, and event-queue extension#1722simonCatBot wants to merge 11 commits into
Conversation
Implements the OpenVX KHR pipelining extension (vx_khr_pipelining.h): - Graph scheduling modes (QUEUE_AUTO / QUEUE_MANUAL / streaming) - Graph-parameter enqueue/dequeue and done-reference tracking - Event registration/delivery (node completed, graph completed, graph-parameter consumed, user events) - Source/sink node pipeup pre-fill and node-state tracking - Replicated graph-parameter handling via object-array/pyramid siblings - Feature flag OPENVX_USE_PIPELINING, default ON Conformance results: - GraphPipeline.*: 109/109 pass - GraphStreaming.*: 24/24 pass
…nd HIP - Build the Khronos OpenVX-CTS with OPENVX_USE_PIPELINING=ON and OPENVX_USE_STREAMING=ON in both conformance.yml and conformance-hip.yml. - Add pipelining-cpu / streaming-cpu jobs to the CPU workflow. - Add pipelining-hip / streaming-hip jobs to the HIP workflow, exercising both CPU and GPU AGO targets. - Pipelining jobs run GraphPipeline.* with a 90-minute timeout. - Streaming jobs run GraphStreaming.* with a 15-minute timeout.
…alidate The previous change inferred output metadata for every kernel without a validate callback, including built-in kernels that rely on upstream nodes or default meta resolution. This corrupted virtual image metadata and broke vxVerifyGraph for any graph with virtual images/arrays/pyramids (e.g., Graph.GraphFactory, Graph.VirtualImage, Graph.VirtualArray, vision-arithmetic, vision-pyramid, and several GDF tests). Restrict the inference to user kernels only, which is the intended use case for the OpenVX pipelining extension's source/sink kernels.
agoApplyQueuedRefsToBindings and the all-ready checks in the manual and auto executor loops were accessing ready_refs without holding the queue mutex, causing data races and (on some runs) the fatal glibc mutex-owner assertion during GraphPipeline.UniformImage on the CI runner. Lock q->mtx around all ready_refs/consumed_refs accesses. Also remove a leftover empty debug block. Verified: GraphPipeline.* + GraphStreaming.* combined 133/133 pass locally; legacy graph repro and And_alt.gdf still pass.
- Lock q->mtx in the QUEUE_AUTO poll path of agoExecuteGraphPipelined, which raced with the background executor and could corrupt the std::deque / adjacent mutex state seen as the pthread_mutex_owner assertion. - Initialize the context event system eagerly in AgoContext construction and leave it disabled by default; vxEnableEvents toggles it on. This removes the racy lazy creation path and avoids emitting/logging events unless the application explicitly enables them. Verified: full GraphPipeline.* + GraphStreaming.* combined = 133/133 pass; legacy graph repro and And_alt.gdf still pass.
There was a problem hiding this comment.
Pull request overview
Adds Khronos OpenVX 1.3.2 vx_khr_pipelining support (graph parameter queues, streaming, and event queue APIs) to the AMD OpenVX engine, including a build-time toggle and CTS CI coverage to close the conformance gap.
Changes:
- Introduces
OPENVX_USE_PIPELININGCMake option and wires new pipelining/streaming/event APIs into the core engine. - Adds new internal pipelining engine (
ago_pipelining.cpp) and public API layer (vx_pipelining_api.cpp) implementing queueing, streaming threads, and event delivery. - Expands CI conformance workflows to run CTS
GraphPipeline.*andGraphStreaming.*suites on CPU and HIP.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| amd_openvx/openvx/CMakeLists.txt | Adds OPENVX_USE_PIPELINING option and builds new pipelining sources. |
| amd_openvx/openvx/api/vx_pipelining_api.cpp | New public extension API implementation (+ stubs when disabled). |
| amd_openvx/openvx/api/vx_api.cpp | Adds/updates context/graph/kernel/node/reference attributes needed for pipelining + relaxes vxAddUserKernel validate requirement. |
| amd_openvx/openvx/ago/ago_util.cpp | Adds new pipelining/event system state to core structs and cleans up in destructors. |
| amd_openvx/openvx/ago/ago_platform.h | Includes pipelining header and adds STL synchronization primitives. |
| amd_openvx/openvx/ago/ago_pipelining.cpp | New internal pipelining execution helpers, queue handling, and event emission. |
| amd_openvx/openvx/ago/ago_internal.h | Adds pipelining/event system structs and new fields on graph/context/kernel/node. |
| amd_openvx/openvx/ago/ago_interface.cpp | Wires event notifications and pipelining execution into verify/execute/process/wait paths. |
| .github/workflows/conformance.yml | Adds CTS configuration vars and new CPU pipelining/streaming jobs. |
| .github/workflows/conformance-hip.yml | Adds CTS configuration vars and new HIP pipelining/streaming jobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dule race Changes: - Guard VX_CONTEXT_EVENT_TIMEOUT/VX_KERNEL_PIPEUP_* when pipelining disabled. - Restore default labels in vxQueryNode/vxSetNodeAttribute switches. - Add AgoGraphParameterQueue constructor and initialize AgoData::children cleanup. - Use atomic graph thread counters; fix semaphore lost-wakeup in WaitForSingleObject. - Remove spurious semaphore releases in agoWaitGraph that caused phantom executions. - Implement VX_REFERENCE_ENQUEUE_COUNT and use graph timeout in dequeue. - Add timeout support to vxWaitEvent via evsys->timeout_ms. - Clean up event registrations in AgoGraph/AgoNode/AgoData destructors. - Replace QUEUE_AUTO polling with condition-variable wakeups. - Remove debug prints and bogus -DOPENVX_USE_STREAMING=ON CI flags. Verification: - GraphPipeline.* + GraphStreaming.* = 133/133 pass (no ASan leaks). - ManualSchedule/8 stress run = 20/20 pass. - Legacy Graph.VirtualImage repro and And_alt.gdf still pass.
simonCatBot
left a comment
There was a problem hiding this comment.
@Copilot — thanks for the detailed review. The latest push addresses all of the items below. PTAL.
-
VX_CONTEXT_EVENT_TIMEOUT guard (
vx_api.cpp) —vxSetContextAttributenow returnsVX_ERROR_NOT_SUPPORTEDwhenagoGetContextEventSystem()returnsnullptr(i.e. pipelining compiled out). -
vxQueryNode
default:label (vx_api.cpp) — restored the explicitdefault:label in thevxQueryNodeswitch. -
AgoGraphParameterQueue uninitialized fields (
ago_internal.h) — added a constructor that initializesindex,max_depth, andenabled. -
vxWaitEvent timeout (
vx_pipelining_api.cpp) — now honorsevsys->timeout_ms: waits forever whenVX_TIMEOUT_WAIT_FOREVER, otherwise useswait_for(..., timeout_ms)and returnsVX_FAILUREon timeout. -
vxGraphParameterDequeueDoneRef timeout (
vx_pipelining_api.cpp) — now usespipe->timeout_mswith a timed wait ondone_cv. -
Bogus
-DOPENVX_USE_STREAMING=ONin CI (.github/workflows/conformance.ymland.github/workflows/conformance-hip.yml) — removed the unused flag. -
VX_KERNEL_PIPEUP_ query guard* (
vx_api.cpp) —vxQueryKernelonly reports pipeup depths whenOPENVX_USE_PIPELININGis enabled; otherwise returnsVX_ERROR_NOT_SUPPORTED. -
VX_KERNEL_PIPEUP_ set guard* (
vx_api.cpp) —vxSetKernelAttributeonly accepts pipeup depths whenOPENVX_USE_PIPELININGis enabled. -
VX_REFERENCE_ENQUEUE_COUNT (
vx_api.cpp+ago_pipelining.cpp) — implementedagoGetReferenceEnqueueCount()and wired it intovxQueryReference; returns the number of graph-parameter queues that currently hold the reference inready/consumed/done. -
Event-registration cleanup (
ago_pipelining.cpp,ago_util.cpp) —agoRemoveEventRegistrations()is now invoked fromAgoGraph::~AgoGraph,AgoNode::~AgoNode, andAgoData::~AgoDatato prevent stale registrations/use-after-free. -
QUEUE_AUTO polling (
ago_pipelining.cpp) — replaced the busysleep_forloop with acondition_variablewait onpipe->enqueue_cv, waking whenever refs are enqueued or the executor is stopped. -
Duplicate/early GPU node-completed events (
ago_interface.cpp) — reviewed the event emission points.VX_EVENT_NODE_COMPLETEDis emitted once per node execution in the CPU path and once after GPU synchronization in the GPU path, not twice.
Also fixed a pre-existing flaky race in the graph-scheduling thread that caused GraphPipeline.ManualSchedule/8 to hang in combined CTS runs:
- Made the graph thread counters atomic (
threadScheduleCount,threadExecuteCount, etc.). - Fixed
WaitForSingleObjectfor semaphores so it does not lose wake-ups when a release happens before the wait. - Removed the spurious
ReleaseSemaphorecalls inagoWaitGraphthat were generating phantom graph executions and desynchronizing the schedule/wait counts. - Made the graph-thread loop resilient to spurious semaphore returns.
Verification:
GraphPipeline.*:GraphStreaming.*combined: 133/133 pass (no AddressSanitizer leaks).GraphPipeline.ManualSchedule/8afterGraph.VirtualImage: 20/20 stress runs pass.- Legacy
Graph.VirtualImagerepro andAnd_alt.gdfstill pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
amd_openvx/openvx/api/vx_pipelining_api.cpp:230
- vxGraphParameterDequeueDoneRef() returns VX_FAILURE on timeout, but the extension spec (vx_khr_pipelining.h) requires returning VX_ERROR_TIMEOUT when VX_GRAPH_TIMEOUT is finite and the wait expires.
if (!q->done_cv.wait_for(lock, std::chrono::milliseconds(pipe->timeout_ms),
[q]() { return !q->done_refs.empty(); }))
return VX_FAILURE;
}
amd_openvx/openvx/api/vx_pipelining_api.cpp:313
- vxWaitEvent() returns VX_FAILURE on timeout, but the extension spec requires VX_ERROR_TIMEOUT when VX_CONTEXT_EVENT_TIMEOUT is finite and the wait expires.
} else {
if (!evsys->events_cv.wait_for(lock, std::chrono::milliseconds(evsys->timeout_ms),
[&evsys]() { return !evsys->events.empty(); }))
return VX_FAILURE;
}
amd_openvx/openvx/api/vx_pipelining_api.cpp:412
- vxWaitGraphEvent() is currently implemented as a thin wrapper over vxWaitEvent() on the context. That means it can return events from other graphs (no graph filtering) and it ignores the VX_GRAPH_EVENT_TIMEOUT attribute (spec requires using the graph-level timeout for this API).
VX_API_ENTRY vx_status VX_API_CALL vxWaitGraphEvent(vx_graph graph, vx_event_t *event, vx_bool do_not_block)
{
if (!agoIsValidGraph((AgoGraph *)graph) || !event)
return VX_ERROR_INVALID_REFERENCE;
AgoContext * context = ((AgoGraph *)graph)->ref.context;
return vxWaitEvent(context, event, do_not_block);
}
amd_openvx/openvx/api/vx_pipelining_api.cpp:192
- vxGraphParameterEnqueueReadyRef() pushes into q->ready_refs but never enforces q->max_depth (which is set via vxSetGraphScheduleConfig / first enqueue). This allows unbounded growth and breaks the queue-depth contract; once the queue is full the function should fail (VX_FAILURE) instead of silently accepting more refs.
std::lock_guard<std::mutex> lock(q->mtx);
q->ready_refs.push_back((AgoData *)refs[i]);
}
amd_openvx/openvx/api/vx_pipelining_api.cpp:395
- vxRegisterEvent() currently (1) allows registering events after vxVerifyGraph (spec requires registration before verification), (2) does not validate that the event type matches the reference type (graph vs node), and (3) appends duplicate registrations instead of updating the stored app_value when the same event is registered twice (spec says duplicates should update). This can lead to incorrect app_value resolution and unexpected duplicate registrations.
This issue also appears on line 406 of the same file.
VX_API_ENTRY vx_status VX_API_CALL vxRegisterEvent(vx_reference ref, enum vx_event_type_e type, vx_uint32 param, vx_uint32 app_value)
{
if (!ref || !agoIsValidReference((AgoReference *)ref))
return VX_ERROR_INVALID_REFERENCE;
AgoReference * r = (AgoReference *)ref;
AgoContextEventSystem * evsys = agoGetContextEventSystem(r->context);
if (!evsys)
return VX_FAILURE;
if (type != VX_EVENT_GRAPH_PARAMETER_CONSUMED &&
type != VX_EVENT_GRAPH_COMPLETED &&
type != VX_EVENT_NODE_COMPLETED &&
type != VX_EVENT_NODE_ERROR) {
return VX_ERROR_NOT_SUPPORTED;
}
AgoEventRegistration reg;
reg.ref = ref;
reg.event_type = type;
reg.app_value = app_value;
reg.graph_parameter_index = (type == VX_EVENT_GRAPH_PARAMETER_CONSUMED) ? param : 0;
std::lock_guard<std::mutex> lock(evsys->registrations_mtx);
evsys->registrations.push_back(reg);
return VX_SUCCESS;
}
amd_openvx/openvx/ago/ago_pipelining.cpp:448
- agoMoveConsumedRefsToDone() emits VX_EVENT_GRAPH_PARAMETER_CONSUMED whenever done_refs is non-empty. Since done_refs can remain non-empty across executions until the app dequeues, this will re-emit "consumed" events on subsequent executions even when nothing was consumed in that execution.
if (!q->done_refs.empty()) {
// Notify that a reference at this parameter was consumed during this execution.
agoNotifyGraphParameterConsumed(graph, q->index);
}
amd_openvx/openvx/api/vx_api.cpp:2871
- VX_KERNEL_PIPEUP_INPUT_DEPTH can be set/queried on the kernel, but the value is never propagated to nodes or used by the scheduler/executor. As-is, the attribute is effectively a no-op, which can break kernels that rely on the pipeup input-depth semantics described in vx_khr_pipelining.h (e.g., sink nodes retaining inputs during PIPEUP).
case VX_KERNEL_PIPEUP_INPUT_DEPTH:
if (size == sizeof(vx_uint32)) {
vx_uint32 v = *(const vx_uint32 *)ptr;
if (v < 1 || kernel->finalized) status = VX_ERROR_INVALID_PARAMETERS;
else { kernel->pipeup_input_depth = v; status = VX_SUCCESS; }
}
break;
Summary
Implements the Khronos OpenVX 1.3.2
vx_khr_pipeliningextension (graph pipelining, streaming, and event queue support) in AMD OpenVX.What changed
OPENVX_USE_PIPELININGCMake option (default ON) so the extension can be compiled out when not needed.amd_openvx/openvx/ago/ago_pipelining.cppvxRegisterEvent,vxSendUserEvent, graph/node events)amd_openvx/openvx/api/vx_pipelining_api.cppvxSetGraphScheduleConfig, enqueue/dequeue/check done refs, event APIs, streaming start/stopvxAddUserKernelto accept a NULL validate callback (required by CTS source/sink kernels) and added output-metadata inference so source/sink graphs verify.Conformance results
Run with the minimal OpenVX-CTS runner built against this
libopenvx.so:GraphPipeline.*GraphStreaming.*Build
-DOPENVX_USE_PIPELINING=ON— extension enabled.-DOPENVX_USE_PIPELINING=OFF— extension APIs returnVX_ERROR_NOT_SUPPORTEDand the legacyvxProcessGraphpath remains functional.Related