[Mesa-dev] [PATCH] clover: Add a stub implementation of clCreateImage()

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Tom Stellard <thomas.stellard at amd.com> writes:
>
>> Now that we are uisng the OpenCL 1.2 headers, applications expect all
>> the OpenCL 1.2 functions to be implemented.
>>
>> This fixes linking errors with the piglit CL tests.
>> ---
>>  src/gallium/state_trackers/clover/api/dispatch.cpp |  2 +-
>>  src/gallium/state_trackers/clover/api/dispatch.hpp |  8 +++++++-
>>  src/gallium/state_trackers/clover/api/memory.cpp   | 11 +++++++++++
>>  3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/api/dispatch.cpp b/src/gallium/state_trackers/clover/api/dispatch.cpp
>> index 2ee6208..488e654 100644
>> --- a/src/gallium/state_trackers/clover/api/dispatch.cpp
>> +++ b/src/gallium/state_trackers/clover/api/dispatch.cpp
>> @@ -120,7 +120,7 @@ namespace clover {
>>        clCreateSubDevices,
>>        clRetainDevice,
>>        clReleaseDevice,
>> -      NULL, // clCreateImage
>> +      clCreateImage,
>>        NULL, // clCreateProgramWithBuiltInKernels
>>        NULL, // clCompileProgram
>>        NULL, // clLinkProgram
>> diff --git a/src/gallium/state_trackers/clover/api/dispatch.hpp b/src/gallium/state_trackers/clover/api/dispatch.hpp
>> index 833fb0e..ffae1ae 100644
>> --- a/src/gallium/state_trackers/clover/api/dispatch.hpp
>> +++ b/src/gallium/state_trackers/clover/api/dispatch.hpp
>> @@ -653,7 +653,13 @@ struct _cl_icd_dispatch {
>>     CL_API_ENTRY cl_int (CL_API_CALL *clReleaseDevice)(
>>        cl_device_id device);
>>  
>> -   void *clCreateImage;
>> +   CL_API_ENTRY cl_mem (CL_API_CALL *clCreateImage)(
>> +      cl_context context,
>> +      cl_mem_flags flags,
>> +      const cl_image_format *image_format,
>> +      const cl_image_desc *image_desc,
>> +      void *host_ptr,
>> +      cl_int *errcode_ret);
>>  
>>     CL_API_ENTRY cl_program (CL_API_CALL *clCreateProgramWithBuiltInKernels)(
>>        cl_context context,
>> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
>> index 7ed2191..977e06c 100644
>> --- a/src/gallium/state_trackers/clover/api/memory.cpp
>> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
>> @@ -334,3 +334,14 @@ clSetMemObjectDestructorCallback(cl_mem d_mem,
>>  } catch (error &e) {
>>     return e.get();
>>  }
>> +
>> +/* This function was added in OpenCL 1.2 */
>
> Can you put this comment inside the function and use C++-style comments
> for consistency?
>
>> +CLOVER_API cl_mem
>> +clCreateImage(cl_context d_ctx, cl_mem_flags flags,
>> +              const cl_image_format *format,
>> +              const cl_image_desc *image_desc,
>> +              void *host_ptr, cl_int *r_errcode) {
>> +  fprintf(stderr, "clCreateImage() not implemented\n");
>
> How about using the C++ standard library?
>
> |   std::cerr << "clCreateImage() not implemented." << std::endl;
>

And how about rephrasing the error message to make it clear that the
user is at fault for calling a CL 1.2 function on a CL 1.1
implementation?  E.g. "CL user error: clCreateImage() not supported by
OpenCL 1.1."

> (you'll probably have to include <iostream> e.g. from api/util.hpp)
>
>> +  *r_errcode = CL_IMAGE_FORMAT_NOT_SUPPORTED;
>
> This will cause a segfault if r_errcode is NULL.  You could call
> 'ret_error(r_errcode, CL_IMAGE_FORMAT_NOT_SUPPORTED)', but how about
> returning CL_INVALID_OPERATION instead in all cases where the user calls
> an unsupported entry-point?  That would match what the spec says when
> e.g. an image operation is called and the implementation doesn't support
> images.
>
>> +  return NULL;
>
> I think the indentation of the whole block is inconsistent?
>
> Thanks.
>
>> +}
>> -- 
>> 1.8.1.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140501/d314c6a6/attachment.sig>