Hi Rebecca, This version LGTM except some points need to be minor refined. Just see my comments below.
-----Original Message----- From: Beignet [mailto:beignet-boun...@lists.freedesktop.org] On Behalf Of Rebecca N. Palmer Sent: Wednesday, October 12, 2016 5:50 AM To: Weng, Chuanbo <chuanbo.w...@intel.com>; beignet@lists.freedesktop.org Subject: Re: [Beignet] [PATCH v3] Utests: Allow testing cl_intel_accelerator via ICD v3: Use extension check, not beignet check. Treat claiming to have the extension but not having the kernel as a failure. --- (v2 was the un-numbered 10/10/16 08:07 version...which I subsequently noticed was broken in that it assumed a non-NULL clGetExtensionFunctionAddressForPlatform result meant the extension was supported, which it doesn't in general, https://www.khronos.org/registry/cl/sdk/2.1/docs/man/xhtml/clGetExtensionFunctionAddressForPlatform.html ) --- a/utests/builtin_kernel_block_motion_estimate_intel.cpp +++ b/utests/builtin_kernel_block_motion_estimate_intel.cpp @@ -8,6 +8,19 @@ OCLRELEASEACCELERATORINTEL * oclReleaseA void builtin_kernel_block_motion_estimate_intel(void) { + std::string extStr; + size_t param_value_size; + OCL_CALL(clGetDeviceInfo, device, CL_DEVICE_EXTENSIONS, 0, 0, + ¶m_value_size); std::vector<char> param_value(param_value_size); + OCL_CALL(clGetDeviceInfo, device, CL_DEVICE_EXTENSIONS, param_value_size, + param_value.empty() ? NULL : ¶m_value.front(), + ¶m_value_size); if (!param_value.empty()) + extStr = std::string(¶m_value.front(), param_value_size-1); // + cl_intel_motion_estimation depends on cl_intel_accelerator, so we only + need to check one if (strstr(extStr.c_str(), "cl_intel_motion_estimation") == NULL) { + printf("No cl_intel_motion_estimation, Skip!"); + return; + } [****Chuanbo****] It would be better if you wrapper this part of code into cl_check_motion_estimation() and then move it to utest_helper.cpp. This will keep existing code organization style. There is a bug in Beignet: cl_intel_motion_estimation is supported by IVB only, but all devices show string cl_intel_motion_estimation in their CL_DEVICE_EXTENSIONS. I'll work out a patch to fix this problem. char* built_in_kernel_names; size_t built_in_kernels_size; cl_int err = CL_SUCCESS; @@ -21,7 +34,8 @@ void builtin_kernel_block_motion_estimat if (strstr(built_in_kernel_names, "block_motion_estimate_intel") == NULL) { free(built_in_kernel_names); - return; + printf("Can't find block_motion_estimate_intel built-in kernel"); [****Chuanbo****] Although I know there are somewhere else using printf instead of fprintf(stderr, ...), let's keep in mind that we should better use fprintf(stderr, ...) for output of error info. + OCL_ASSERT(0); } cl_program built_in_prog = clCreateProgramWithBuiltInKernels(ctx, 1, &device, built_in_kernel_names, &err); --- a/utests/CMakeLists.txt +++ b/utests/CMakeLists.txt @@ -287,7 +287,8 @@ set (utests_sources multi_queue_events.cpp compiler_mix.cpp compiler_math_3op.cpp - compiler_bsort.cpp) + compiler_bsort.cpp + builtin_kernel_block_motion_estimate_intel.cpp) if (LLVM_VERSION_NODOT VERSION_GREATER 34) SET(utests_sources @@ -328,7 +329,6 @@ else(GEN_PCI_ID) endif(GEN_PCI_ID) if (NOT_BUILD_STAND_ALONE_UTEST) - SET(utests_sources ${utests_sources} builtin_kernel_block_motion_estimate_intel.cpp) ADD_CUSTOM_TARGET(kernel_bin.bin DEPENDS ${kernel_bin}.bin) endif (NOT_BUILD_STAND_ALONE_UTEST) _______________________________________________ Beignet mailing list Beignet@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list Beignet@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/beignet