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 <[email protected]> > > > > > > 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, > > > - ¶m_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, > > > + ¶m_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 <[email protected]>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
