[DO NOT MERGE] [NPU] Using global command queue #28745

pereanub · 2025-01-30T10:01:26Z

Details:

Another option for PR#28661
Using a global command queue per core and not creating different queues for each compiled model. The plugin will create a different queue only if the properties for using them are different from a compiled model to another or if the properties(workload type) are changing at runtime

Tickets:

E#154336

Signed-off-by: Bogdan Pereanu <[email protected]>

…nd free methods Signed-off-by: Bogdan Pereanu <[email protected]>

Signed-off-by: Bogdan Pereanu <[email protected]>

razvanapetroaie

Haven't found any major issue regarding the design nor the code, but please don't rely on my review alone - I don't have much experience with the driver API.

razvanapetroaie · 2025-01-31T15:47:26Z

src/plugins/intel_npu/src/utils/src/zero/zero_wrappers.cpp

+
+CommandQueuePool::CommandQueuePool() : _log("CommandQueue", Logger::global().level()) {}
+int CommandQueuePool::computeHash(CommandQueueDesc desc) {
+    return (static_cast<size_t>(desc.priority) & 0xFF) | (static_cast<size_t>(desc.workload) & 0xFF) << 8 |


Several things to note here:

Both priority and workload type can take the value of "0x7fffffff" (ZE_WORKLOAD_TYPE_FORCE_UINT32) which exceeds the 1 byte range. So we might have an issue if another value > 255 is defined in any of those enums. Perhaps not a problem since this scenario is unlikely, but worth taking into consideration.

int doesn't have a stable byte size and it seems you're using only 3 bytes so maybe we should use uint32_t.

C++ has a dedicated hash functionality, you may consider overriding its operator() instead. Example here, OV also has several in its implementation.

razvanapetroaie · 2025-01-31T15:51:21Z

src/plugins/intel_npu/src/utils/src/zero/zero_wrappers.cpp

+}
+std::shared_ptr<CommandQueue> CommandQueuePool::getCommandQueue(
+    const std::shared_ptr<ZeroInitStructsHolder>& init_structs,
+    const ze_command_queue_priority_t& priority,


Suggested change

const ze_command_queue_priority_t& priority,

const ze_command_queue_priority_t priority,

Primitive type, cheaper to pass by value.

razvanapetroaie · 2025-01-31T16:17:08Z

src/plugins/intel_npu/src/utils/src/zero/zero_wrappers.cpp

+    const ze_command_queue_workload_type_t& workload_type,
+    const uint32_t& group_ordinal,
+    bool turbo) {
+    CommandQueueDesc desc = {priority, workload_type, turbo};


Worth asking: are we certain these three are all attributes that should be used for determining command queue sharing? Just making sure, I don't know how to answer that. I see there's also some group_ordinal (whatever that is) and multiple fields inside init_structs.

razvanapetroaie · 2025-01-31T16:23:29Z

src/plugins/intel_npu/src/utils/include/intel_npu/utils/zero/zero_utils.hpp

@@ -50,6 +52,34 @@ namespace zeroUtils {
                       ze_result_to_description(result)); \
    }

+static inline size_t toPriorityVal(const ze_command_queue_priority_t& val) {


Not used? Same about toWorkloadVal.

razvanapetroaie · 2025-01-31T16:34:52Z

src/plugins/intel_npu/src/compiler_adapter/src/plugin_graph.cpp

+    }
+}
+
+void PluginGraph::create_new_command_queue() {


Name a bit misleading since we're not always creating a new one depending on whether or not we find one using the same stats in the pool.

razvanapetroaie · 2025-01-31T16:45:48Z

src/plugins/intel_npu/src/compiler_adapter/src/driver_graph.cpp

    if (config.has<TURBO>()) {
-        turbo = config.get<TURBO>();
+        _turbo = config.get<TURBO>();


Nit: Will Config::get() return the default value if no value was explicitly set? If so, you could allow the default value to do its job by not setting _turbo to false (in the header file) and not checking config.has<TURBO>().

razvanapetroaie · 2025-01-31T17:08:53Z

src/plugins/intel_npu/src/backend/src/zero_pipeline.cpp

@@ -147,9 +180,30 @@ Pipeline::Pipeline(const Config& config,
    _logger.debug("Pipeline - initialize completed");
 }

+void Pipeline::getCommandQueue() {


Nit: a bit weird to call a function a "getter" without returning anything.

MirceaDan99 · 2025-02-03T09:16:56Z

src/plugins/intel_npu/tests/functional/internal/overload/compile_and_infer.hpp

@@ -206,6 +208,60 @@ TEST_P(OVCompileAndInferRequest, CompiledModelWorkloadTypeUpdateAfterCompilation
    }
 }

+TEST_P(OVCompileAndInferRequest, CompiledModelWorkloadTypeUpdateAfterCompilationWithMultipleInfers) {
+    if (isCommandQueueExtSupported()) {
+        OV_ASSERT_NO_THROW(execNet = core->compile_model(function, target_device, configuration));


We should also change execNet old API naming with the new compiledModel.

pereanub added do not merge do_not_review labels Jan 30, 2025

github-actions bot added the category: NPU OpenVINO NPU plugin label Jan 30, 2025

pereanub added 14 commits January 30, 2025 16:47

Use global command queues

dbdd8f7

Signed-off-by: Bogdan Pereanu <[email protected]>

Adding test case

f92aeb2

Signed-off-by: Bogdan Pereanu <[email protected]>

Destroy pipeline if it was created but workload type is not supported

d7803f3

Signed-off-by: Bogdan Pereanu <[email protected]>

Update tests, command queue is created and set at the first infer

3d1a682

Signed-off-by: Bogdan Pereanu <[email protected]>

Update the names of the variables, methods, classes

ccc983c

Signed-off-by: Bogdan Pereanu <[email protected]>

Create a static instance for CommandQueueManager class and lock get a…

0c5e380

…nd free methods Signed-off-by: Bogdan Pereanu <[email protected]>

Add new func test

080eae6

Signed-off-by: Bogdan Pereanu <[email protected]>

Print correct error message

230a418

Signed-off-by: Bogdan Pereanu <[email protected]>

Run test only on newer drivers

92892ba

Signed-off-by: Bogdan Pereanu <[email protected]>

Create event pool and events only if they are used

6b8027f

Signed-off-by: Bogdan Pereanu <[email protected]>

Add new test case for changing priority, turbo and workload type

4fd51e2

Signed-off-by: Bogdan Pereanu <[email protected]>

Destroy pipeline even when use count is 0

b306c55

Signed-off-by: Bogdan Pereanu <[email protected]>

Make sure that the pipeline is still alive when fences are destroyed

937f700

Signed-off-by: Bogdan Pereanu <[email protected]>

Change logic to use dynamic unordered map

6d4ab81

Signed-off-by: Bogdan Pereanu <[email protected]>

pereanub removed the do_not_review label Jan 30, 2025

pereanub force-pushed the global_command_queue_graph branch from e0df99b to 6d4ab81 Compare January 30, 2025 14:50

pereanub marked this pull request as ready for review January 30, 2025 14:53

pereanub requested review from a team as code owners January 30, 2025 14:53

Set worklokad type

acb0b4f

Signed-off-by: Bogdan Pereanu <[email protected]>

pereanub force-pushed the global_command_queue_graph branch from 81f15a6 to acb0b4f Compare January 30, 2025 17:02

Dont' need to create new deleter, add extra test case

f71a8be

Signed-off-by: Bogdan Pereanu <[email protected]>

razvanapetroaie approved these changes Jan 31, 2025

View reviewed changes

MirceaDan99 reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] [NPU] Using global command queue #28745

[DO NOT MERGE] [NPU] Using global command queue #28745

pereanub commented Jan 30, 2025 •

edited

Loading

razvanapetroaie left a comment

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

razvanapetroaie Jan 31, 2025

MirceaDan99 Feb 3, 2025

	const ze_command_queue_priority_t& priority,
	const ze_command_queue_priority_t priority,

[DO NOT MERGE] [NPU] Using global command queue #28745

Are you sure you want to change the base?

[DO NOT MERGE] [NPU] Using global command queue #28745

Conversation

pereanub commented Jan 30, 2025 • edited Loading

Details:

Tickets:

razvanapetroaie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereanub commented Jan 30, 2025 •

edited

Loading