[Beignet] [Piglit] [PATCH V2] fix CL_KERNEL_GLOBAL_WORK_SIZE bug.

[Tom Stellard]

On Fri, Sep 26, 2014 at 07:11:03AM +0800, xionghu.luo at intel.com wrote:
> From: Luo <xionghu.luo at intel.com>
> 
> the option  CL_KERNEL_GLOBAL_WORK_SIZE for clGetKernelWorkGroupInfo
> should call built in kernel or custom device according to the spec,
> this patch calls the built in kernel to query the GLOBAL_WORK_SIZE.
> 
> v2: use built in kernel to qury the GLOBAL_WORK_SIZE if exist, dummy
> kernel for other options, handle the case when no built in kernel is
> provided.
> 
> Signed-off-by: Luo <xionghu.luo at intel.com>
> ---
>  tests/cl/api/get-kernel-work-group-info.c |   66 +++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/cl/api/get-kernel-work-group-info.c b/tests/cl/api/get-kernel-work-group-info.c
> index 47d09da..11d29d2 100644
> --- a/tests/cl/api/get-kernel-work-group-info.c
> +++ b/tests/cl/api/get-kernel-work-group-info.c
> @@ -61,6 +61,11 @@ piglit_cl_test(const int argc,
>  	int i;
>  	cl_int errNo;
>  	cl_kernel kernel;
> +	cl_program built_in_prog = NULL;
> +	cl_kernel built_in_kernel = NULL;
> +	cl_kernel temp_kernel;
> +	size_t built_in_kernels_size;
> +
>  
>  	size_t param_value_size;
>  	void* param_value;
> @@ -71,19 +76,65 @@ piglit_cl_test(const int argc,
>  		PIGLIT_CL_ENUM_ARRAY(cl_kernel_work_group_info);
>  
>  	kernel = clCreateKernel(env->program,
> -	                        "dummy_kernel",
> -	                        &errNo);
> +		"dummy_kernel",
> +		&errNo);

This change is unnecessary.

> +
> +	errNo = clGetDeviceInfo(env->device_id, CL_DEVICE_BUILT_IN_KERNELS, 0, 0, &built_in_kernels_size);
>  	if(!piglit_cl_check_error(errNo, CL_SUCCESS)) {
>  		fprintf(stderr,
> -		        "Failed (error code: %s): Create kernel.\n",
> +		        "Failed (error code: %s): Get Device Info.\n",
>  		        piglit_cl_get_error_name(errNo));
>  		return PIGLIT_FAIL;
>  	}
>  
> +	if(built_in_kernels_size != 0)
> +	{
> +		char* built_in_kernel_names;
> +		char* kernel_name;
> +		size_t ret_sz;
> +		built_in_kernel_names = (char* )malloc(built_in_kernels_size * sizeof(char) );
> +
> +		errNo = clGetDeviceInfo(env->device_id, CL_DEVICE_BUILT_IN_KERNELS, built_in_kernels_size, (void*)built_in_kernel_names, &ret_sz);
> +		if(!piglit_cl_check_error(errNo, CL_SUCCESS)) {
> +			fprintf(stderr,
> +						"Failed (error code: %s): Get Device Info.\n",
> +						piglit_cl_get_error_name(errNo));
> +			return PIGLIT_FAIL;
> +		}
> +
> +		built_in_prog = clCreateProgramWithBuiltInKernels(env->context->cl_ctx, 1, &env->device_id, built_in_kernel_names, &errNo);
> +		if(!piglit_cl_check_error(errNo, CL_SUCCESS)) {
> +			fprintf(stderr,
> +						"Failed (error code: %s): Create BuiltIn Program.\n",
> +						piglit_cl_get_error_name(errNo));
> +			return PIGLIT_FAIL;
> +		}
> +
> +		kernel_name = strtok(built_in_kernel_names, ";");
> +
> +		built_in_kernel = clCreateKernel(built_in_prog, kernel_name,  &errNo);
> +		if(!piglit_cl_check_error(errNo, CL_SUCCESS)) {
> +			fprintf(stderr,
> +						"Failed (error code: %s): Create kernel.\n",
> +						piglit_cl_get_error_name(errNo));
> +			return PIGLIT_FAIL;
> +			}
> +			free(built_in_kernel_names);

The indentation here is wrong.

> +	}
> +
>  	/*** Normal usage ***/
>  	for(i = 0; i < num_kernel_work_group_infos; i++) {
>  		printf("%s ", piglit_cl_get_enum_name(kernel_work_group_infos[i]));
>  
> +		//use builtin kernel to test CL_KERNEL_GLOBAL_WORK_SIZE.	swap the dummy kernel and builtin_kernel.
> +		if(kernel_work_group_infos[i] == CL_KERNEL_GLOBAL_WORK_SIZE){
> +			if(built_in_kernel != NULL)	{
> +					temp_kernel = kernel;
> +					kernel = built_in_kernel;
> +					built_in_kernel = temp_kernel;
> +			}
> +		}
> +

Rather than swapping the kernel, I think it would be better to
factor the body of this loop out to its own function and do something like:

|
|	for (i = 0; i < num_kernel_work_group_infos; i++) {
|		cl_int expected_error = CL_SUCCESS;		
|		if (kernel_work_group_infos[i] == CL_KERNEL_GLOBAL_WORK_SIZE) {
|			if (cl_version < 1.2) {
|				continue;
|			}
|			expected_error = CL_INVALID_VALUE
|		}
|		test_cl_get_kernel_work_group_info(kernel_work_group_infos[i], expected_error, ...);
|	}
|
|	if (cl_version == 1.2) {
|		test_cl_get_kernel_work_group_info, CL_KERNEL_GLOBAL_WORK_SIZE, CL_SUCESS, ...);
|	}

Since this is a 1.2 feature, you will need to make sure that the platform
supports 1.2.

>  		errNo = clGetKernelWorkGroupInfo(kernel,
>  		                                 env->device_id,
>  		                                 kernel_work_group_infos[i],
> @@ -114,6 +165,13 @@ piglit_cl_test(const int argc,
>  			piglit_merge_result(&result, PIGLIT_FAIL);
>  		}
>  
> +		if(kernel_work_group_infos[i] == CL_KERNEL_GLOBAL_WORK_SIZE){
> +			if(built_in_kernel != NULL)	{
> +					temp_kernel = kernel;
> +					kernel = built_in_kernel;
> +					built_in_kernel = temp_kernel;
> +			}
> +		}
>  		//TODO: output returned values
>  		printf("\n");
>  		free(param_value);
> @@ -188,6 +246,8 @@ piglit_cl_test(const int argc,
>  	}
>  
>  	clReleaseKernel(kernel);
> +	clReleaseKernel(built_in_kernel);
> +	clReleaseProgram(built_in_prog);
>  
>  	return result;
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit