[Piglit] [PATCH 1/1] cl: fix clGetKernelWorkGroupInfo

[Jan Vesely]

On Wed, 2014-08-13 at 15:01 +0200, EdB wrote:
> On Tuesday 12 August 2014 17:25:37 Jan Vesely wrote:
> > On Sun, 2014-08-10 at 16:46 +0200, EdB wrote:
> > > From: EdB <edb at sigluy.net>
> > > 
> > > If there is only one device associed with the kernel and device arg is
> > > NULL
> > > you don't have to trigger CL_INVALID_DEVICE
> > > ---
> > > 
> > >  tests/cl/api/get-kernel-work-group-info.c | 31
> > >  +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12
> > >  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..bf0bb29 100644
> > > --- a/tests/cl/api/get-kernel-work-group-info.c
> > > +++ b/tests/cl/api/get-kernel-work-group-info.c
> > > @@ -61,6 +61,7 @@ piglit_cl_test(const int argc,
> > > 
> > >  	int i;
> > >  	cl_int errNo;
> > >  	cl_kernel kernel;
> > > 
> > > +	cl_uint* dev_count_ptr;
> > > 
> > >  	size_t param_value_size;
> > >  	void* param_value;
> > > 
> > > @@ -168,24 +169,30 @@ piglit_cl_test(const int argc,
> > > 
> > >  		        piglit_cl_get_error_name(errNo));
> > >  		
> > >  		piglit_merge_result(&result, PIGLIT_FAIL);
> > >  	
> > >  	}
> > > 
> > > -
> > > +
> > > 
> > >  	/*
> > >  	
> > >  	 * CL_INVALID_DEVICE if device is not in the list of devices 
> associated
> > >  	 with
> > >  	 * kernel or if device is NULL but there is more than one device
> > >  	 associated
> > >  	 * with kernel.
> > >  	 */
> > > 
> > > -	errNo = clGetKernelWorkGroupInfo(kernel,
> > > -	                                 NULL,
> > > -	                                 CL_KERNEL_WORK_GROUP_SIZE,
> > > -	                                 0,
> > > -	                                 NULL,
> > > -	                                 &param_value_size);
> > > -	if(!piglit_cl_check_error(errNo, CL_INVALID_DEVICE)) {
> > > -		fprintf(stderr,
> > > -		        "Failed (error code: %s): Trigger CL_INVALID_DEVICE if 
> device
> > > is NULL but there is more than one device associated with kernel.\n", -		
> > >        piglit_cl_get_error_name(errNo));
> > > -		piglit_merge_result(&result, PIGLIT_FAIL);
> > > +	dev_count_ptr = piglit_cl_get_program_info(env->program,
> > > CL_PROGRAM_NUM_DEVICES); +	if (*dev_count_ptr != 1) {
> > > +		errNo = clGetKernelWorkGroupInfo(kernel,
> > > +		                               NULL,
> > > +		                               CL_KERNEL_WORK_GROUP_SIZE,
> > > +		                               0,
> > > +		                               NULL,
> > > +		                               &param_value_size);
> > > +		if(!piglit_cl_check_error(errNo, CL_INVALID_DEVICE)) {
> > > +			if (*dev_count_ptr != 1) {
> > 
> > What is the purpose of testing (*dev_count_ptr != 1) again? It's already
> > in one big !=1 block.
> 
> It was the first attempt to fix the test, I forgot to remove it
> 
> > 
> > Moreover, can the number of devices ever be != 1 since the test is
> > marked run_per_device?
> > IMO this change effectively disables the subtest
> 
> Indeed. I was thinking that even if the test is per device, the program is 
> build only once (if not, why this test).
> 
> But you're right, piglit_cl_api_test_t rebuild the program per device
> and this should always return CL_SUCCES
> 
> Thank for the review

thanks for working on this, the test is definitely broken.
IMO it would be good enough to either
a) change the expected return value to CL_SUCCESS with an explanatory
comment
or
b) change it to something like ( *dev_count_ptr == 1 ? CL_SUCCESS :
CL_INVALID_DEVICE ), in case the way per-device tests are handled
changes in the future, although this might be a bit too cautious.


if you plan to send v2, you might want to cc Francisco or Tom, I can't
push patches.

gl,
jan

> 
> > 
> > jan
> > 
> > > +				fprintf(stderr,
> > > +				        "Failed (error code: %s): Trigger 
> CL_INVALID_DEVICE if device
> > > is NULL but there is more than one device associated with kernel.\n",
> > > +				        piglit_cl_get_error_name(errNo));
> > > +				piglit_merge_result(&result, PIGLIT_FAIL);
> > > +			}
> > > +		}
> > > 
> > >  	}
> > > 
> > > +	free(dev_count_ptr);
> > > 
> > >  	clReleaseKernel(kernel);


-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140822/3574951a/attachment.sig>