From d889c599b2a6975c2db28e1d4b6044f980df9a99 Mon Sep 17 00:00:00 2001 From: Filip Hazubski Date: Mon, 7 Sep 2020 19:44:29 +0200 Subject: [PATCH] Correct callback API functions Add implementation to clSetProgramReleaseCallback and clSetContextDestructorCallback functions. Resolves: NEO-4962, NEO-5051 Change-Id: Iad6fffc663665a3cf16b96aa90065140cf8c5477 Signed-off-by: Filip Hazubski --- opencl/source/api/api.cpp | 10 +++--- opencl/source/context/context.cpp | 13 ++++++++ opencl/source/context/context.h | 8 +++++ opencl/source/helpers/CMakeLists.txt | 1 + opencl/source/helpers/destructor_callback.h | 33 +++++++++++++++++++ opencl/source/mem_obj/mem_obj.cpp | 16 +++------ opencl/source/mem_obj/mem_obj.h | 17 ++-------- opencl/source/program/program.cpp | 14 ++++++++ opencl/source/program/program.h | 7 ++++ .../cl_set_context_destructor_callback.inl | 15 +++++---- .../api/cl_set_program_release_callback.inl | 15 +++++---- .../test/unit_test/context/context_tests.cpp | 31 +++++++++++++++++ .../test/unit_test/program/program_tests.cpp | 32 ++++++++++++++++++ 13 files changed, 171 insertions(+), 41 deletions(-) create mode 100644 opencl/source/helpers/destructor_callback.h diff --git a/opencl/source/api/api.cpp b/opencl/source/api/api.cpp index a804f06e48..72f64d38b5 100644 --- a/opencl/source/api/api.cpp +++ b/opencl/source/api/api.cpp @@ -5381,11 +5381,12 @@ cl_int CL_API_CALL clSetProgramReleaseCallback(cl_program program, cl_int retVal = CL_SUCCESS; API_ENTER(&retVal); - retVal = validateObjects(program, + Program *pProgram = nullptr; + retVal = validateObjects(WithCastToInternal(program, &pProgram), reinterpret_cast(pfnNotify)); if (retVal == CL_SUCCESS) { - retVal = CL_INVALID_OPERATION; + retVal = pProgram->setReleaseCallback(pfnNotify, userData); } return retVal; @@ -5597,11 +5598,12 @@ cl_int CL_API_CALL clSetContextDestructorCallback(cl_context context, cl_int retVal = CL_SUCCESS; API_ENTER(&retVal); - retVal = validateObjects(context, + Context *pContext = nullptr; + retVal = validateObjects(WithCastToInternal(context, &pContext), reinterpret_cast(pfnNotify)); if (retVal == CL_SUCCESS) { - retVal = CL_OUT_OF_HOST_MEMORY; + retVal = pContext->setDestructorCallback(pfnNotify, userData); } return retVal; diff --git a/opencl/source/context/context.cpp b/opencl/source/context/context.cpp index 2f68993c79..4ce20365b5 100644 --- a/opencl/source/context/context.cpp +++ b/opencl/source/context/context.cpp @@ -62,6 +62,10 @@ Context::~Context() { memoryManager->getDeferredDeleter()->removeClient(); } gtpinNotifyContextDestroy((cl_context)this); + for (auto callback : destructorCallbacks) { + callback->invoke(this); + delete callback; + } for (auto &device : devices) { device->decRefInternal(); } @@ -71,6 +75,15 @@ Context::~Context() { schedulerBuiltIn->pProgram = nullptr; } +cl_int Context::setDestructorCallback(void(CL_CALLBACK *funcNotify)(cl_context, void *), + void *userData) { + auto cb = new ContextDestructorCallback(funcNotify, userData); + + std::unique_lock theLock(mtx); + destructorCallbacks.push_front(cb); + return CL_SUCCESS; +} + DeviceQueue *Context::getDefaultDeviceQueue() { return defaultDeviceQueue; } diff --git a/opencl/source/context/context.h b/opencl/source/context/context.h index 9602f548cb..2e83c8a57d 100644 --- a/opencl/source/context/context.h +++ b/opencl/source/context/context.h @@ -14,6 +14,9 @@ #include "opencl/source/context/context_type.h" #include "opencl/source/context/driver_diagnostics.h" #include "opencl/source/helpers/base_object.h" +#include "opencl/source/helpers/destructor_callback.h" + +#include namespace NEO { @@ -63,6 +66,9 @@ class Context : public BaseObject<_cl_context> { ~Context() override; + cl_int setDestructorCallback(void(CL_CALLBACK *funcNotify)(cl_context, void *), + void *userData); + cl_int getInfo(cl_context_info paramName, size_t paramValueSize, void *paramValue, size_t *paramValueSizeRet); @@ -164,5 +170,7 @@ class Context : public BaseObject<_cl_context> { bool interopUserSync = false; cl_bool preferD3dSharedResources = 0u; ContextType contextType = ContextType::CONTEXT_TYPE_DEFAULT; + + std::list destructorCallbacks; }; } // namespace NEO diff --git a/opencl/source/helpers/CMakeLists.txt b/opencl/source/helpers/CMakeLists.txt index f0c8dd8980..5e4a92a3b2 100644 --- a/opencl/source/helpers/CMakeLists.txt +++ b/opencl/source/helpers/CMakeLists.txt @@ -14,6 +14,7 @@ set(RUNTIME_SRCS_HELPERS_BASE ${CMAKE_CURRENT_SOURCE_DIR}/cl_device_helpers.h ${CMAKE_CURRENT_SOURCE_DIR}/cl_helper.h ${CMAKE_CURRENT_SOURCE_DIR}/convert_color.h + ${CMAKE_CURRENT_SOURCE_DIR}/destructor_callback.h ${CMAKE_CURRENT_SOURCE_DIR}/dispatch_info.cpp ${CMAKE_CURRENT_SOURCE_DIR}/dispatch_info.h ${CMAKE_CURRENT_SOURCE_DIR}/dispatch_info_builder.h diff --git a/opencl/source/helpers/destructor_callback.h b/opencl/source/helpers/destructor_callback.h new file mode 100644 index 0000000000..35357ee301 --- /dev/null +++ b/opencl/source/helpers/destructor_callback.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2020 Intel Corporation + * + * SPDX-License-Identifier: MIT + * + */ + +#pragma once +#include "CL/cl.h" + +namespace NEO { + +template +class DestructorCallback { + public: + DestructorCallback(void(CL_CALLBACK *funcNotify)(T, void *), + void *userData) + : funcNotify(funcNotify), userData(userData){}; + + inline void invoke(T object) { + this->funcNotify(object, userData); + } + + private: + void(CL_CALLBACK *funcNotify)(T, void *); + void *userData; +}; + +using ContextDestructorCallback = DestructorCallback; +using MemObjDestructorCallback = DestructorCallback; +using ProgramReleaseCallback = DestructorCallback; + +} // namespace NEO diff --git a/opencl/source/mem_obj/mem_obj.cpp b/opencl/source/mem_obj/mem_obj.cpp index d639432455..f1814a2a7c 100644 --- a/opencl/source/mem_obj/mem_obj.cpp +++ b/opencl/source/mem_obj/mem_obj.cpp @@ -105,20 +105,14 @@ MemObj::~MemObj() { releaseAllocatedMapPtr(); } } - if (!destructorCallbacks.empty()) { - for (auto iter = destructorCallbacks.rbegin(); iter != destructorCallbacks.rend(); iter++) { - (*iter)->invoke(this); - delete *iter; - } + for (auto callback : destructorCallbacks) { + callback->invoke(this); + delete callback; } context->decRefInternal(); } -void MemObj::DestructorCallback::invoke(cl_mem memObj) { - this->funcNotify(memObj, userData); -} - cl_int MemObj::getMemObjectInfo(cl_mem_info paramName, size_t paramValueSize, void *paramValue, @@ -220,10 +214,10 @@ cl_int MemObj::getMemObjectInfo(cl_mem_info paramName, cl_int MemObj::setDestructorCallback(void(CL_CALLBACK *funcNotify)(cl_mem, void *), void *userData) { - auto cb = new DestructorCallback(funcNotify, userData); + auto cb = new MemObjDestructorCallback(funcNotify, userData); std::unique_lock theLock(mtx); - destructorCallbacks.push_back(cb); + destructorCallbacks.push_front(cb); return CL_SUCCESS; } diff --git a/opencl/source/mem_obj/mem_obj.h b/opencl/source/mem_obj/mem_obj.h index a63949c36f..491e2dcf4e 100644 --- a/opencl/source/mem_obj/mem_obj.h +++ b/opencl/source/mem_obj/mem_obj.h @@ -12,6 +12,7 @@ #include "opencl/extensions/public/cl_ext_private.h" #include "opencl/source/api/cl_types.h" #include "opencl/source/helpers/base_object.h" +#include "opencl/source/helpers/destructor_callback.h" #include "opencl/source/helpers/mipmap.h" #include "opencl/source/mem_obj/map_operations_handler.h" #include "opencl/source/sharings/sharing.h" @@ -20,6 +21,7 @@ #include #include +#include #include namespace NEO { @@ -160,19 +162,6 @@ class MemObj : public BaseObject<_cl_mem> { std::shared_ptr sharingHandler; std::vector propertiesVector; - class DestructorCallback { - public: - DestructorCallback(void(CL_CALLBACK *funcNotify)(cl_mem, void *), - void *userData) - : funcNotify(funcNotify), userData(userData){}; - - void invoke(cl_mem memObj); - - private: - void(CL_CALLBACK *funcNotify)(cl_mem, void *); - void *userData; - }; - - std::vector destructorCallbacks; + std::list destructorCallbacks; }; } // namespace NEO diff --git a/opencl/source/program/program.cpp b/opencl/source/program/program.cpp index 8a8e2fd8f4..fcebeca2ae 100644 --- a/opencl/source/program/program.cpp +++ b/opencl/source/program/program.cpp @@ -113,6 +113,11 @@ Program::Program(ExecutionEnvironment &executionEnvironment, Context *context, b } Program::~Program() { + for (auto callback : releaseCallbacks) { + callback->invoke(this); + delete callback; + } + cleanCurrentKernelInfo(); freeBlockResources(); @@ -267,6 +272,15 @@ cl_int Program::updateSpecializationConstant(cl_uint specId, size_t specSize, co return CL_INVALID_SPEC_ID; } +cl_int Program::setReleaseCallback(void(CL_CALLBACK *funcNotify)(cl_program, void *), + void *userData) { + auto cb = new ProgramReleaseCallback(funcNotify, userData); + + std::unique_lock theLock(mtx); + releaseCallbacks.push_front(cb); + return CL_SUCCESS; +} + void Program::setDevice(Device *device) { this->pDevice = device; } diff --git a/opencl/source/program/program.h b/opencl/source/program/program.h index 4d8a7ac551..24449c1fb9 100644 --- a/opencl/source/program/program.h +++ b/opencl/source/program/program.h @@ -14,10 +14,12 @@ #include "opencl/source/api/cl_types.h" #include "opencl/source/helpers/base_object.h" +#include "opencl/source/helpers/destructor_callback.h" #include "cif/builtins/memory/buffer/buffer.h" #include "patch_list.h" +#include #include #include @@ -148,6 +150,9 @@ class Program : public BaseObject<_cl_program> { cl_int setProgramSpecializationConstant(cl_uint specId, size_t specSize, const void *specValue); MOCKABLE_VIRTUAL cl_int updateSpecializationConstant(cl_uint specId, size_t specSize, const void *specValue); + cl_int setReleaseCallback(void(CL_CALLBACK *funcNotify)(cl_program, void *), + void *userData); + size_t getNumKernels() const; const KernelInfo *getKernelInfo(const char *kernelName) const; const KernelInfo *getKernelInfo(size_t ordinal) const; @@ -341,6 +346,8 @@ class Program : public BaseObject<_cl_program> { bool isBuiltIn = false; bool kernelDebugEnabled = false; + + std::list releaseCallbacks; }; } // namespace NEO diff --git a/opencl/test/unit_test/api/cl_set_context_destructor_callback.inl b/opencl/test/unit_test/api/cl_set_context_destructor_callback.inl index 96c10ffa06..096b2bece2 100644 --- a/opencl/test/unit_test/api/cl_set_context_destructor_callback.inl +++ b/opencl/test/unit_test/api/cl_set_context_destructor_callback.inl @@ -23,12 +23,15 @@ TEST_F(clSetContextDestructorCallbackTests, givenPfnNotifyNullptrWhenSettingCont EXPECT_EQ(CL_INVALID_VALUE, retVal); } -TEST_F(clSetContextDestructorCallbackTests, WhenSettingContextDestructorCallbackThenOutOfHostMemoryErrorIsReturned) { - using NotifyFunctionType = void(CL_CALLBACK *)(cl_context, void *); - NotifyFunctionType pfnNotify = reinterpret_cast(0x1234); - void *userData = reinterpret_cast(0x4321); - auto retVal = clSetContextDestructorCallback(pContext, pfnNotify, userData); - EXPECT_EQ(CL_OUT_OF_HOST_MEMORY, retVal); +void CL_CALLBACK callback(cl_context, void *){}; + +TEST_F(clSetContextDestructorCallbackTests, WhenSettingContextDestructorCallbackThenSucccessIsReturned) { + auto retVal = clSetContextDestructorCallback(pContext, callback, nullptr); + EXPECT_EQ(CL_SUCCESS, retVal); + + auto userData = reinterpret_cast(0x4321); + retVal = clSetContextDestructorCallback(pContext, callback, userData); + EXPECT_EQ(CL_SUCCESS, retVal); } } // namespace ULT diff --git a/opencl/test/unit_test/api/cl_set_program_release_callback.inl b/opencl/test/unit_test/api/cl_set_program_release_callback.inl index 407a77b917..10cea0bc6d 100644 --- a/opencl/test/unit_test/api/cl_set_program_release_callback.inl +++ b/opencl/test/unit_test/api/cl_set_program_release_callback.inl @@ -23,12 +23,15 @@ TEST_F(clSetProgramReleaseCallbackTests, givenPfnNotifyNullptrWhenSettingProgram EXPECT_EQ(CL_INVALID_VALUE, retVal); } -TEST_F(clSetProgramReleaseCallbackTests, WhenSettingProgramReleaseCallbackThenInvalidOperationErrorIsReturned) { - using NotifyFunctionType = void(CL_CALLBACK *)(cl_program, void *); - NotifyFunctionType pfnNotify = reinterpret_cast(0x1234); - void *userData = reinterpret_cast(0x4321); - auto retVal = clSetProgramReleaseCallback(pProgram, pfnNotify, userData); - EXPECT_EQ(CL_INVALID_OPERATION, retVal); +void CL_CALLBACK callback(cl_program, void *){}; + +TEST_F(clSetProgramReleaseCallbackTests, WhenSettingProgramReleaseCallbackThenSucccessIsReturned) { + auto retVal = clSetProgramReleaseCallback(pProgram, callback, nullptr); + EXPECT_EQ(CL_SUCCESS, retVal); + + auto userData = reinterpret_cast(0x4321); + retVal = clSetProgramReleaseCallback(pProgram, callback, userData); + EXPECT_EQ(CL_SUCCESS, retVal); } } // namespace ULT diff --git a/opencl/test/unit_test/context/context_tests.cpp b/opencl/test/unit_test/context/context_tests.cpp index 2228084102..18b08dccb3 100644 --- a/opencl/test/unit_test/context/context_tests.cpp +++ b/opencl/test/unit_test/context/context_tests.cpp @@ -444,3 +444,34 @@ TEST(Context, givenContextWithMultipleSubDevicesWhenGettingDeviceBitfieldForAllo EXPECT_EQ(expectedDeviceBitfield.to_ulong(), context->getDeviceBitfieldForAllocation().to_ulong()); context->release(); } + +TEST(Context, WhenSettingContextDestructorCallbackThenCallOrderIsPreserved) { + struct UserDataType { + cl_context expectedContext; + std::vector &vectorToModify; + size_t valueToAdd; + }; + auto callback = [](cl_context context, void *userData) -> void { + auto pUserData = reinterpret_cast(userData); + EXPECT_EQ(pUserData->expectedContext, context); + pUserData->vectorToModify.push_back(pUserData->valueToAdd); + }; + + auto pContext = new MockContext{}; + std::vector callbacksReturnValues; + UserDataType userDataArray[]{ + {pContext, callbacksReturnValues, 1}, + {pContext, callbacksReturnValues, 2}, + {pContext, callbacksReturnValues, 3}}; + + for (auto &userData : userDataArray) { + cl_int retVal = clSetContextDestructorCallback(pContext, callback, &userData); + ASSERT_EQ(CL_SUCCESS, retVal); + } + delete pContext; + + ASSERT_EQ(3u, callbacksReturnValues.size()); + EXPECT_EQ(3u, callbacksReturnValues[0]); + EXPECT_EQ(2u, callbacksReturnValues[1]); + EXPECT_EQ(1u, callbacksReturnValues[2]); +} diff --git a/opencl/test/unit_test/program/program_tests.cpp b/opencl/test/unit_test/program/program_tests.cpp index bf6049d678..d29e04dd54 100644 --- a/opencl/test/unit_test/program/program_tests.cpp +++ b/opencl/test/unit_test/program/program_tests.cpp @@ -3121,3 +3121,35 @@ TEST(ProgramReplaceDeviceBinary, GivenBinaryZebinThenUseAsBothPackedAndUnpackedB EXPECT_EQ(0, memcmp(program.packedDeviceBinary.get(), zebin.storage.data(), program.packedDeviceBinarySize)); EXPECT_EQ(0, memcmp(program.unpackedDeviceBinary.get(), zebin.storage.data(), program.unpackedDeviceBinarySize)); } + +TEST(Program, WhenSettingProgramReleaseCallbackThenCallOrderIsPreserved) { + struct UserDataType { + cl_program expectedProgram; + std::vector &vectorToModify; + size_t valueToAdd; + }; + auto callback = [](cl_program program, void *userData) -> void { + auto pUserData = reinterpret_cast(userData); + EXPECT_EQ(pUserData->expectedProgram, program); + pUserData->vectorToModify.push_back(pUserData->valueToAdd); + }; + + MockExecutionEnvironment executionEnvironment; + auto pProgram = new MockProgram{executionEnvironment}; + std::vector callbacksReturnValues; + UserDataType userDataArray[]{ + {pProgram, callbacksReturnValues, 1}, + {pProgram, callbacksReturnValues, 2}, + {pProgram, callbacksReturnValues, 3}}; + + for (auto &userData : userDataArray) { + cl_int retVal = clSetProgramReleaseCallback(pProgram, callback, &userData); + ASSERT_EQ(CL_SUCCESS, retVal); + } + delete pProgram; + + ASSERT_EQ(3u, callbacksReturnValues.size()); + EXPECT_EQ(3u, callbacksReturnValues[0]); + EXPECT_EQ(2u, callbacksReturnValues[1]); + EXPECT_EQ(1u, callbacksReturnValues[2]); +}