Skip to content

Conversation

@jkasprza
Copy link
Contributor

@jkasprza jkasprza commented May 29, 2025

Details:

@jkasprza jkasprza added the category: GPU OpenVINO GPU plugin label May 29, 2025
@jkasprza jkasprza requested a review from sshlyapn May 29, 2025 09:56
@github-actions github-actions bot added category: inference OpenVINO Runtime library - Inference category: build OpenVINO cmake script / infra category: CI OpenVINO public CI category: dependency_changes Pull requests that update a dependency file category: CPP API OpenVINO CPP API bindings github_actions Pull requests that update GitHub Actions code labels May 29, 2025
@p-durandin
Copy link
Contributor

build_jenkins

@jkasprza jkasprza marked this pull request as ready for review August 13, 2025 09:19
@jkasprza jkasprza requested review from a team as code owners August 13, 2025 09:19
@github-actions github-actions bot removed the category: Core OpenVINO Core (aka ngraph) label Nov 28, 2025
url = https://github.com/herumi/xbyak_riscv.git
[submodule "src/plugins/intel_gpu/thirdparty/l0_onednn_gpu"]
path = src/plugins/intel_gpu/thirdparty/l0_onednn_gpu
url = https://github.com/jkasprza/oneDNN.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the record: I guess we will have single copy of onednn_gpu. Please update that before it is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to wait for uxlfoundation/oneDNN#4499 to be merged first.

bool has_separate_cache; ///< Does the target hardware has separate cache for usm_device and usm_host

bool supports_cp_offload; ///< [L0] Does the command queue support copy offload
bool supports_cb_events; ///< [L0] Does the target runtime support counter based events
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cb_events sounds ambiguous because cb usually means callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in 8c9e855

_profiling_info.clear();
}
// Set event profiling data instead of retrieving it from event object
void set_profiling(uint64_t duration_nsec) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about set_profiling_duration instead? I think the name is difficult to understand.

Copy link
Contributor Author

@jkasprza jkasprza Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in d377c45

#include <utility>
#include <utility>
#include <functional>
#include <optional>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we need this header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in d377c45


QueueTypes stream::detect_queue_type(engine_types engine_type, void* queue_handle) {
switch (engine_type) {
case engine_types::sycl:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need create_surfaces_lock for sycl_stream as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here sycl_stream derives create_surfaces_lock from ocl_stream. It will create ocl::ocl_surfaces_lock for sycl_stream the same way It was implemented before changes.

}

void ze_event::wait_impl() {
OV_ZE_EXPECT(zeEventHostSynchronize(m_event, default_timeout));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about endless_wait instead of default_timeout to make it explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in 5ccd52c

// Ensure event handle is not null
if (ev == nullptr) {
OPENVINO_THROW("[GPU] Trying to create event with null handle");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what about OPENVINO_ASSERT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8621d68

void set_impl() override;
bool is_set_impl() override;
// TODO: Implement add_event_handler_impl
// bool add_event_handler_impl(event_handler, void*) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, but is it required?

ze_event_pool_flags_t flags = is_profiling_enabled() ? ZE_EVENT_POOL_FLAG_KERNEL_TIMESTAMP : 0;
flags |= ZE_EVENT_POOL_FLAG_HOST_VISIBLE;
m_current_pool = std::make_shared<ze_event_pool>(m_engine, m_capacity, flags);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assume single thread execution, right? isn't it necessary to have some synchronization here?

struct ze_event_factory : public ze_base_event_factory {
public:
ze_event_factory(const ze_engine &engine, bool enable_profiling, uint32_t capacity = 255);
event::ptr create_event(uint64_t queue_stamp) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does queue_stamp mean here? is it relevant to queue? does it mean unique id of event? What about event_stamp then?

Copy link
Contributor Author

@jkasprza jkasprza Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here queue_stamp is analogous to already existing queue_stamp from ocl_base_event. It is just a value provided by the stream when creating events and is used for ordering the events for example in ocl_stream::sync_events. It should be unique if the events were created by the same stream. I do not see variable named event_stamp anywhere in the code.


ze_event_handle_t m_last_event = nullptr;
std::vector<event::ptr> m_events;
const ze_engine &m_engine;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please use same convention for naming as ocl_event.


auto mem_properties = std::find_if(device_memory_properties.begin(), device_memory_properties.end(), [](const ze_device_memory_properties_t& p) {
auto name = std::string(p.name);
return name == "DDR" || name == "HBM";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • out of curiosity, what other name does it have other than DDR and HBM? e.g. does it have property like "SRAM"?
  • Am I correct that intel GPUs have only 1 type of memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I am not sure. So far, I only encountered single type that is either DDR or HBM

}
_mapped_ptr = nullptr;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These code is very similar to the one in ocl_memory.cpp. Could you refactor it to reduce duplication?

ev_ze,
ze_dep_events.size(),
ze_dep_events.data()));
// FIXME: when not blocking pattern goes out of scope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer to local variable pattern is passed to the zeCommandListAppendMemoryFill. To avoid use after free, memory fill must finish execution before this variable gets destroyed.

@isanghao
Copy link
Contributor

I finished 1st round of review. Could you check my comment? I see code duplication in many places. Could you review whether it can be reduced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: build OpenVINO cmake script / infra category: CI OpenVINO public CI category: CPP API OpenVINO CPP API bindings category: dependency_changes Pull requests that update a dependency file category: GPU OpenVINO GPU plugin category: inference OpenVINO Runtime library - Inference do_not_merge github_actions Pull requests that update GitHub Actions code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants