[Beignet] [PATCH 1/2] runtime: Use cl_get_platform_default to replace global value.
Yang, Rong R
rong.r.yang at intel.com
Sun Jul 5 22:00:23 PDT 2015
One comment.
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> junyan.he at inbox.com
> Sent: Friday, July 3, 2015 18:25
> To: beignet at lists.freedesktop.org
> Cc: Junyan He
> Subject: [Beignet] [PATCH 1/2] runtime: Use cl_get_platform_default to
> replace global value.
>
> From: Junyan He <junyan.he at linux.intel.com>
>
> The init order of the intel_platform and the intel_extension is somehow not
> clear. When some API such as clGetDeviceIDs can pass NULL as
> cl_platform_id, we just use the global value intel_platform as the default but
> do not care about the init state of the extension.
> The init of the extension may be done when the cl device is created.
> This is OK if the paltform and the device have the same extensions.
> But now because of the fp16, they are not always the same.
> Use cl_get_platform_default to replace the global value to ensure that when
> default platform is available, the extension is also inited.
>
> Signed-off-by: Junyan He <junyan.he at linux.intel.com>
> ---
> src/cl_api.c | 6 +++---
> src/cl_context.c | 2 +-
> src/cl_device_id.c | 55 +++++++++++++++++++++------------------------------
> -
> src/cl_extensions.c | 38 +++++++++++++++++-------------------
> src/cl_platform_id.c | 28 +++++++++++++++++--------- src/cl_platform_id.h
> | 4 ++--
> 6 files changed, 65 insertions(+), 68 deletions(-)
>
> diff --git a/src/cl_api.c b/src/cl_api.c index 3e72deb..1ba775f 100644
> --- a/src/cl_api.c
> +++ b/src/cl_api.c
> @@ -195,7 +195,7 @@ clGetPlatformInfo(cl_platform_id platform,
> size_t * param_value_size_ret)
> {
> /* Only one platform. This is easy */
> - if (UNLIKELY(platform != NULL && platform != intel_platform))
> + if (UNLIKELY(platform != NULL && platform !=
> + cl_get_platform_default()))
> return CL_INVALID_PLATFORM;
>
> return cl_get_platform_info(platform, @@ -217,7 +217,7 @@
> clGetDeviceIDs(cl_platform_id platform,
> /* Check parameter consistency */
> if (UNLIKELY(devices == NULL && num_devices == NULL))
> return CL_INVALID_VALUE;
> - if (UNLIKELY(platform && platform != intel_platform))
> + if (UNLIKELY(platform && platform != cl_get_platform_default()))
> return CL_INVALID_PLATFORM;
> if (UNLIKELY(devices && num_entries == 0))
> return CL_INVALID_VALUE;
> @@ -3193,7 +3193,7 @@ void*
> clGetExtensionFunctionAddressForPlatform(cl_platform_id platform,
> const char *func_name) {
> - if (UNLIKELY(platform != NULL && platform != intel_platform))
> + if (UNLIKELY(platform != NULL && platform !=
> + cl_get_platform_default()))
> return NULL;
> return internal_clGetExtensionFunctionAddress(func_name);
> }
> diff --git a/src/cl_context.c b/src/cl_context.c index 773f545..45064ad 100644
> --- a/src/cl_context.c
> +++ b/src/cl_context.c
> @@ -68,7 +68,7 @@ cl_context_properties_process(const
> cl_context_properties *prop,
> case CL_CONTEXT_PLATFORM:
> CHECK (set_cl_context_platform);
> cl_props->platform_id = *(prop + 1);
> - if (UNLIKELY((cl_platform_id) cl_props->platform_id != intel_platform)) {
> + if (UNLIKELY((cl_platform_id) cl_props->platform_id !=
> + cl_get_platform_default())) {
> err = CL_INVALID_PLATFORM;
> goto error;
> }
> diff --git a/src/cl_device_id.c b/src/cl_device_id.c index 9f03027..7956646
> 100644
> --- a/src/cl_device_id.c
> +++ b/src/cl_device_id.c
> @@ -351,7 +351,7 @@ cl_get_gt_device(void)
> DECL_INFO_STRING(has_break, intel_hsw_gt3_device, name, "Intel(R)
> HD Graphics Haswell CRW GT3 reserved");
> has_break:
> device->vendor_id = device_id;
> - device->platform = intel_platform;
> + device->platform = cl_get_platform_default();
> ret = device;
> break;
>
> @@ -363,7 +363,7 @@ has_break:
> DECL_INFO_STRING(ivb_gt1_break, intel_ivb_gt1_device, name, "Intel(R)
> HD Graphics IvyBridge S GT1");
> ivb_gt1_break:
> intel_ivb_gt1_device.vendor_id = device_id;
> - intel_ivb_gt1_device.platform = intel_platform;
> + intel_ivb_gt1_device.platform = cl_get_platform_default();
> ret = &intel_ivb_gt1_device;
> break;
>
> @@ -375,7 +375,7 @@ ivb_gt1_break:
> DECL_INFO_STRING(ivb_gt2_break, intel_ivb_gt2_device, name, "Intel(R)
> HD Graphics IvyBridge S GT2");
> ivb_gt2_break:
> intel_ivb_gt2_device.vendor_id = device_id;
> - intel_ivb_gt2_device.platform = intel_platform;
> + intel_ivb_gt2_device.platform = cl_get_platform_default();
> ret = &intel_ivb_gt2_device;
> break;
>
> @@ -383,7 +383,7 @@ ivb_gt2_break:
> DECL_INFO_STRING(baytrail_t_device_break, intel_baytrail_t_device,
> name, "Intel(R) HD Graphics Bay Trail-T");
> baytrail_t_device_break:
> intel_baytrail_t_device.vendor_id = device_id;
> - intel_baytrail_t_device.platform = intel_platform;
> + intel_baytrail_t_device.platform = cl_get_platform_default();
> ret = &intel_baytrail_t_device;
> break;
>
> @@ -399,9 +399,9 @@ baytrail_t_device_break:
> DECL_INFO_STRING(brw_gt1_break, intel_brw_gt1_device, name,
> "Intel(R) HD Graphics BroadWell ULX GT1");
> brw_gt1_break:
> /* For Gen8 and later, half float is suppported and we will enable
> cl_khr_fp16. */
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_brw_gt1_device.vendor_id = device_id;
> - intel_brw_gt1_device.platform = intel_platform;
> + intel_brw_gt1_device.platform = cl_get_platform_default();
> ret = &intel_brw_gt1_device;
> break;
>
> @@ -416,9 +416,9 @@ brw_gt1_break:
> case PCI_CHIP_BROADWLL_U_GT2:
> DECL_INFO_STRING(brw_gt2_break, intel_brw_gt2_device, name,
> "Intel(R) HD Graphics BroadWell ULX GT2");
> brw_gt2_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_brw_gt2_device.vendor_id = device_id;
> - intel_brw_gt2_device.platform = intel_platform;
> + intel_brw_gt2_device.platform = cl_get_platform_default();
> ret = &intel_brw_gt2_device;
> break;
>
> @@ -433,9 +433,9 @@ brw_gt2_break:
> case PCI_CHIP_BROADWLL_U_GT3:
> DECL_INFO_STRING(brw_gt3_break, intel_brw_gt3_device, name,
> "Intel(R) HD Graphics BroadWell ULX GT2");
> brw_gt3_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_brw_gt3_device.vendor_id = device_id;
> - intel_brw_gt3_device.platform = intel_platform;
> + intel_brw_gt3_device.platform = cl_get_platform_default();
> ret = &intel_brw_gt3_device;
> break;
>
> @@ -445,9 +445,9 @@ brw_gt3_break:
> case PCI_CHIP_CHV_3:
> DECL_INFO_STRING(chv_break, intel_chv_device, name, "Intel(R) HD
> Graphics Cherryview");
> chv_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_chv_device.vendor_id = device_id;
> - intel_chv_device.platform = intel_platform;
> + intel_chv_device.platform = cl_get_platform_default();
> ret = &intel_chv_device;
> break;
>
> @@ -463,9 +463,9 @@ chv_break:
> case PCI_CHIP_SKYLAKE_SRV_GT1:
> DECL_INFO_STRING(skl_gt1_break, intel_skl_gt1_device, name, "Intel(R)
> HD Graphics Skylake Server GT1");
> skl_gt1_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_skl_gt1_device.vendor_id = device_id;
> - intel_skl_gt1_device.platform = intel_platform;
> + intel_skl_gt1_device.platform = cl_get_platform_default();
> ret = &intel_skl_gt1_device;
> break;
>
> @@ -482,9 +482,9 @@ skl_gt1_break:
> case PCI_CHIP_SKYLAKE_SRV_GT2:
> DECL_INFO_STRING(skl_gt2_break, intel_skl_gt2_device, name, "Intel(R)
> HD Graphics Skylake Server GT2");
> skl_gt2_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_skl_gt2_device.vendor_id = device_id;
> - intel_skl_gt2_device.platform = intel_platform;
> + intel_skl_gt2_device.platform = cl_get_platform_default();
> ret = &intel_skl_gt2_device;
> break;
>
> @@ -495,9 +495,9 @@ skl_gt2_break:
> case PCI_CHIP_SKYLAKE_SRV_GT3:
> DECL_INFO_STRING(skl_gt3_break, intel_skl_gt3_device, name, "Intel(R)
> HD Graphics Skylake Server GT3");
> skl_gt3_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_skl_gt3_device.vendor_id = device_id;
> - intel_skl_gt3_device.platform = intel_platform;
> + intel_skl_gt3_device.platform = cl_get_platform_default();
> ret = &intel_skl_gt3_device;
> break;
>
> @@ -506,9 +506,9 @@ skl_gt3_break:
> case PCI_CHIP_SKYLAKE_SRV_GT4:
> DECL_INFO_STRING(skl_gt4_break, intel_skl_gt4_device, name, "Intel(R)
> HD Graphics Skylake Server GT4");
> skl_gt4_break:
> - cl_intel_platform_enable_fp16_extension(intel_platform);
> +
> + cl_intel_platform_enable_fp16_extension(cl_get_platform_default());
> intel_skl_gt4_device.vendor_id = device_id;
> - intel_skl_gt4_device.platform = intel_platform;
> + intel_skl_gt4_device.platform = cl_get_platform_default();
> ret = &intel_skl_gt4_device;
> break;
>
> @@ -636,17 +636,6 @@ cl_get_device_ids(cl_platform_id platform,
> {
> cl_device_id device;
>
> - /* Spec allow platform to be NULL, and If platform
> - is NULL, the behavior is implementation-defined.
> - We can not init the device before platform init. */
> - if (!platform) {
> - if (num_devices)
> - *num_devices = 0;
> - if (devices)
> - *devices = 0;
> - return CL_DEVICE_NOT_FOUND;
> - }
> -
> /* Do we have a usable device? */
> device = cl_get_gt_device();
> if (device) {
> @@ -678,8 +667,8 @@ cl_get_device_ids(cl_platform_id platform,
> *num_devices = 1;
> if (devices) {
> *devices = device;
> - (*devices)->extensions = intel_platform->extensions;
> - (*devices)->extensions_sz = intel_platform->extensions_sz;
> + (*devices)->extensions = cl_get_platform_default()->extensions;
> + (*devices)->extensions_sz =
> + cl_get_platform_default()->extensions_sz;
> }
> return CL_SUCCESS;
> }
> diff --git a/src/cl_extensions.c b/src/cl_extensions.c index 0a29d2f..f1948b3
> 100644
> --- a/src/cl_extensions.c
> +++ b/src/cl_extensions.c
> @@ -13,7 +13,9 @@
> #include <string.h>
> #include <assert.h>
>
> -static struct cl_extensions intel_extensions =
> +/* This extension should be common for all the intel GPU platform.
> + Every device may have its own additional externsions. */ static
> +struct cl_extensions intel_platform_extensions =
> {
> {
> #define DECL_EXT(name) \
> @@ -91,14 +93,12 @@ process_extension_str(cl_extensions_t *extensions)
> }
> }
>
> -static int ext_initialized = 0;
>
> LOCAL void
> cl_intel_platform_enable_fp16_extension(cl_platform_id intel_platform) {
> - cl_extensions_t *extensions = &intel_extensions;
> + cl_extensions_t *extensions = &intel_platform_extensions;
> int id;
> - assert(ext_initialized);
>
> for(id = OPT1_EXT_START_ID; id <= OPT1_EXT_END_ID; id++)
> {
> @@ -107,29 +107,27 @@
> cl_intel_platform_enable_fp16_extension(cl_platform_id intel_platform)
> }
>
> process_extension_str(extensions);
> - intel_platform->internal_extensions = &intel_extensions;
> - intel_platform->extensions = intel_extensions.ext_str;
> + intel_platform->internal_extensions = &intel_platform_extensions;
> + intel_platform->extensions = intel_platform_extensions.ext_str;
> intel_platform->extensions_sz = strlen(intel_platform->extensions) + 1; }
>
> LOCAL void
> cl_intel_platform_extension_init(cl_platform_id intel_platform) {
> - if (ext_initialized) {
> - intel_platform->internal_extensions = &intel_extensions;
> - intel_platform->extensions = intel_extensions.ext_str;
> - return;
> + static int ext_initialized = 0;
> +
> + if (!ext_initialized) {
> + check_basic_extension(&intel_platform_extensions);
> + check_opt1_extension(&intel_platform_extensions);
> + check_gl_extension(&intel_platform_extensions);
> + check_intel_extension(&intel_platform_extensions);
> + process_extension_str(&intel_platform_extensions);
> + ext_initialized = 1;
> }
Is ext_initialized check still needed?
More information about the Beignet
mailing list