[Mesa-dev] [PATCH] clover: Implement clSetCommandQueueProperty() from OpenCL 1.0

[Francisco Jerez]

Edward O'Callaghan <funfunctor at folklore1984.net> writes:

> Signed-off-by: Edward O'Callaghan <funfunctor at folklore1984.net>
> ---
>  include/CL/cl.h                                    |  6 ++++
>  src/gallium/state_trackers/clover/api/dispatch.cpp |  2 +-
>  src/gallium/state_trackers/clover/api/queue.cpp    | 34 ++++++++++++++++++++++
>  src/gallium/state_trackers/clover/core/queue.cpp   |  3 ++
>  src/gallium/state_trackers/clover/core/queue.hpp   |  1 +
>  5 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/include/CL/cl.h b/include/CL/cl.h
> index 316565d..51adf65 100644
> --- a/include/CL/cl.h
> +++ b/include/CL/cl.h
> @@ -656,6 +656,12 @@ clGetCommandQueueInfo(cl_command_queue      /* command_queue */,
>                        void *                /* param_value */,
>                        size_t *              /* param_value_size_ret */) CL_API_SUFFIX__VERSION_1_0;
>  
> +extern CL_API_ENTRY cl_int CL_API_CALL
> +clSetCommandQueueProperty(cl_command_queue            /* command_queue */,
> +                          cl_command_queue_properties /* properties */,
> +                          cl_bool                     /* param_value */,
> +                          cl_command_queue_properties /* properties */) CL_API_SUFFIX__VERSION_1_0;
> +

I guess this change is likely to get lost when we import future API
headers from Khronos, because the function has been removed from the API
headers by them.  If we do include a declaration of this entry point in
the headers it should probably be marked with both:

  CL_EXT_PREFIX__VERSION_1_0_DEPRECATED 
  CL_EXT_SUFFIX__VERSION_1_0_DEPRECATED

so users get a deprecation warning if they use it by mistake.

Also, the declaration doesn't seem to match the CL 1.0 spec, the fourth
argument is supposed to be a pointer type as in your definition below.

>  /* Memory Object APIs */
>  extern CL_API_ENTRY cl_mem CL_API_CALL
>  clCreateBuffer(cl_context   /* context */,
> diff --git a/src/gallium/state_trackers/clover/api/dispatch.cpp b/src/gallium/state_trackers/clover/api/dispatch.cpp
> index 8f4cfdc..0f64914 100644
> --- a/src/gallium/state_trackers/clover/api/dispatch.cpp
> +++ b/src/gallium/state_trackers/clover/api/dispatch.cpp
> @@ -37,7 +37,7 @@ namespace clover {
>        clRetainCommandQueue,
>        clReleaseCommandQueue,
>        clGetCommandQueueInfo,
> -      NULL, // clSetCommandQueueProperty
> +      clSetCommandQueueProperty,
>        clCreateBuffer,
>        clCreateImage2D,
>        clCreateImage3D,
> diff --git a/src/gallium/state_trackers/clover/api/queue.cpp b/src/gallium/state_trackers/clover/api/queue.cpp
> index 06a2863..7734e19 100644
> --- a/src/gallium/state_trackers/clover/api/queue.cpp
> +++ b/src/gallium/state_trackers/clover/api/queue.cpp
> @@ -105,6 +105,40 @@ clGetCommandQueueInfo(cl_command_queue d_q, cl_command_queue_info param,
>  }
>  
>  CLOVER_API cl_int
> +clSetCommandQueueProperty(cl_command_queue d_q,
> +                     cl_command_queue_properties props, cl_bool enabled,

Align this and the next line to the first function argument.  The CL
spec refers to the third argument of this entry point as 'enable', which
makes more sense to me than 'enabled'.

> +                     cl_command_queue_properties *old_props) try {
> +   //throws CL_INVALID_COMMAND_QUEUE if command_queue is not a valid.
> +   auto &q = obj(d_q);
> +
> +   if (props & ~(CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE |
> +                 CL_QUEUE_PROFILING_ENABLE))
> +      throw error(CL_INVALID_VALUE);
> +
> +   if (!old_props) // If old_properties is NULL, it is ignored.

Inverted condition as you pointed out already.

> +	   *old_props = q.propertices();
> +

Typo in the previous line and below, did you compile-test this at all? ;)

> +   if (enabled == CL_TRUE) {
> +      if (q.propertices() & props)
> +         return CL_SUCCESS;
> +   } else { // CL_FALSE
> +      if (q.propertices() & ~props)
> +         return CL_SUCCESS;
> +   }
> +

I don't think this does what 'enable' is supposed to do.  IIUC it
controls whether the specified properties are added or subtracted from
the current set of enabled properties, e.g.:

| q.properties(enable ? q.properties() | props :
|                       q.properties() & ~props);

> +   // TODO how to determine if device supports props???

All devices should be able to support both properties.  I'd drop this
comment and the conditional below.

> +   if (1)
> +      q.propertices(props);
> +   else
> +      throw error(CL_INVALID_QUEUE_PROPERTIES);
> +
> +   return CL_SUCCESS;
> +
> +} catch (error &e) {
> +   return e.get();
> +}
> +
> +CLOVER_API cl_int
>  clFlush(cl_command_queue d_q) try {
>     obj(d_q).flush();
>     return CL_SUCCESS;
> diff --git a/src/gallium/state_trackers/clover/core/queue.cpp b/src/gallium/state_trackers/clover/core/queue.cpp
> index c91b97a..24b15f3 100644
> --- a/src/gallium/state_trackers/clover/core/queue.cpp
> +++ b/src/gallium/state_trackers/clover/core/queue.cpp
> @@ -87,6 +87,9 @@ command_queue::properties() const {
>     return props;
>  }
>  
> +void
> +command_queue::properties(cl_command_queue_properties props) : props(props);
> +

This needs a function body and you cannot use initializer syntax because
the method you're defining is not a constructor.

>  bool
>  command_queue::profiling_enabled() const {
>     return props & CL_QUEUE_PROFILING_ENABLE;
> diff --git a/src/gallium/state_trackers/clover/core/queue.hpp b/src/gallium/state_trackers/clover/core/queue.hpp
> index bddb86c..8c0906d 100644
> --- a/src/gallium/state_trackers/clover/core/queue.hpp
> +++ b/src/gallium/state_trackers/clover/core/queue.hpp
> @@ -49,6 +49,7 @@ namespace clover {
>        void flush();
>  
>        cl_command_queue_properties properties() const;
> +      void command_queue::properties(cl_command_queue_properties props);

I don't think this is valid syntax either?  Just declare it like 'void
properties(cl_command_queue_properties props)'.

>        bool profiling_enabled() const;
>  
>        const intrusive_ref<clover::context> context;
> -- 
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161119/f6c0d187/attachment-0001.sig>