[Mesa-dev,09/10] gallium, clover: add OpenCL interoperability support for CL events

Submitted by Marek Olšák on April 14, 2015, 10:19 p.m.

Details

Message ID 1429049995-21768-10-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák April 14, 2015, 10:19 p.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
 src/gallium/state_trackers/clover/Makefile.sources |  1 +
 src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
 src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
 src/gallium/targets/opencl/opencl.sym              |  1 +
 5 files changed, 114 insertions(+)
 create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
 create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp

Patch hide | download patch | download mbox

diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
new file mode 100644
index 0000000..31a3bd3
--- /dev/null
+++ b/src/gallium/include/state_tracker/opencl_interop.h
@@ -0,0 +1,42 @@ 
+/**************************************************************************
+ *
+ * Copyright 2015 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
+ * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#ifndef OPENCL_INTEROP_H
+#define OPENCL_INTEROP_H
+
+/* dlsym these without the "_t" suffix. You should get the correct symbols
+ * if the OpenCL driver is loaded.
+ *
+ * All functions return non-zero on success.
+ */
+
+typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
+typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
+typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
+typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
+
+#endif /* OPENCL_INTEROP_H */
diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
index 5b3344c..4c2d0f3 100644
--- a/src/gallium/state_trackers/clover/Makefile.sources
+++ b/src/gallium/state_trackers/clover/Makefile.sources
@@ -22,6 +22,7 @@  CPP_SOURCES := \
 	core/event.hpp \
 	core/format.cpp \
 	core/format.hpp \
+	core/interop.cpp \
 	core/kernel.cpp \
 	core/kernel.hpp \
 	core/memory.cpp \
diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
index 0e1359a..28f510f 100644
--- a/src/gallium/state_trackers/clover/core/event.hpp
+++ b/src/gallium/state_trackers/clover/core/event.hpp
@@ -116,6 +116,10 @@  namespace clover {
 
       friend class command_queue;
 
+      struct pipe_fence_handle *get_fence() const {
+         return _fence;
+      }
+
    private:
       virtual void fence(pipe_fence_handle *fence);
       action profile(command_queue &q, const action &action) const;
diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
new file mode 100644
index 0000000..bb80cf5
--- /dev/null
+++ b/src/gallium/state_trackers/clover/core/interop.cpp
@@ -0,0 +1,66 @@ 
+//
+// Copyright 2015 Advanced Micro Devices, Inc.
+// All Rights Reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the "Software"),
+// to deal in the Software without restriction, including without limitation
+// the rights to use, copy, modify, merge, publish, distribute, sublicense,
+// and/or sell copies of the Software, and to permit persons to whom the
+// Software is furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+// OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+// ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+// OTHER DEALINGS IN THE SOFTWARE.
+//
+
+#include "core/event.hpp"
+
+using namespace clover;
+
+extern "C" {
+
+PUBLIC int
+opencl_dri_event_add_ref(intptr_t event)
+{
+   return clRetainEvent((cl_event)event) == CL_SUCCESS;
+}
+
+PUBLIC int
+opencl_dri_event_release(intptr_t event)
+{
+   return clReleaseEvent((cl_event)event) == CL_SUCCESS;
+}
+
+PUBLIC int
+opencl_dri_event_wait(intptr_t cl_event, uint64_t timeout)
+{
+   event *e = (event*)cl_event;
+
+   if (!timeout) {
+      return e->status() == CL_COMPLETE;
+   }
+
+   e->wait();
+   return true;
+}
+
+PUBLIC struct pipe_fence_handle *
+opencl_dri_event_get_fence(intptr_t cl_event)
+{
+   hard_event *e = dynamic_cast<hard_event*>((event*)cl_event);
+
+   if (e)
+      return e->get_fence();
+
+   return NULL;
+}
+
+}
diff --git a/src/gallium/targets/opencl/opencl.sym b/src/gallium/targets/opencl/opencl.sym
index ee8aacf..9fcc576 100644
--- a/src/gallium/targets/opencl/opencl.sym
+++ b/src/gallium/targets/opencl/opencl.sym
@@ -1,6 +1,7 @@ 
 {
 	global:
 		cl*;
+		opencl_dri_*;
 	local:
 		*;
 };

Comments

Only C warns about that. I didn't see any warning with C++.

Marek

On Tue, Apr 21, 2015 at 3:44 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 14/04/15 22:19, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>>  src/gallium/targets/opencl/opencl.sym              |  1 +
>>  5 files changed, 114 insertions(+)
>>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>>
>> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
>> new file mode 100644
>> index 0000000..31a3bd3
>> --- /dev/null
>> +++ b/src/gallium/include/state_tracker/opencl_interop.h
>> @@ -0,0 +1,42 @@
>> +/**************************************************************************
>> + *
>> + * Copyright 2015 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction, including
>> + * without limitation the rights to use, copy, modify, merge, publish,
>> + * distribute, sub license, and/or sell copies of the Software, and to
>> + * permit persons to whom the Software is furnished to do so, subject to
>> + * the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + **************************************************************************/
>> +
>> +#ifndef OPENCL_INTEROP_H
>> +#define OPENCL_INTEROP_H
>> +
>> +/* dlsym these without the "_t" suffix. You should get the correct symbols
>> + * if the OpenCL driver is loaded.
>> + *
>> + * All functions return non-zero on success.
>> + */
>> +
>> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
>> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
>> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
>> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
>> +
>> +#endif /* OPENCL_INTEROP_H */
>> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
>> index 5b3344c..4c2d0f3 100644
>> --- a/src/gallium/state_trackers/clover/Makefile.sources
>> +++ b/src/gallium/state_trackers/clover/Makefile.sources
>> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>>       core/event.hpp \
>>       core/format.cpp \
>>       core/format.hpp \
>> +     core/interop.cpp \
>>       core/kernel.cpp \
>>       core/kernel.hpp \
>>       core/memory.cpp \
>> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
>> index 0e1359a..28f510f 100644
>> --- a/src/gallium/state_trackers/clover/core/event.hpp
>> +++ b/src/gallium/state_trackers/clover/core/event.hpp
>> @@ -116,6 +116,10 @@ namespace clover {
>>
>>        friend class command_queue;
>>
>> +      struct pipe_fence_handle *get_fence() const {
>> +         return _fence;
>> +      }
>> +
>>     private:
>>        virtual void fence(pipe_fence_handle *fence);
>>        action profile(command_queue &q, const action &action) const;
>> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
>> new file mode 100644
>> index 0000000..bb80cf5
>> --- /dev/null
>> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
>> @@ -0,0 +1,66 @@
>> +//
>> +// Copyright 2015 Advanced Micro Devices, Inc.
>> +// All Rights Reserved.
>> +//
>> +// Permission is hereby granted, free of charge, to any person obtaining a
>> +// copy of this software and associated documentation files (the "Software"),
>> +// to deal in the Software without restriction, including without limitation
>> +// the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> +// and/or sell copies of the Software, and to permit persons to whom the
>> +// Software is furnished to do so, subject to the following conditions:
>> +//
>> +// The above copyright notice and this permission notice shall be included in
>> +// all copies or substantial portions of the Software.
>> +//
>> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> +// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> +// OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> +// ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> +// OTHER DEALINGS IN THE SOFTWARE.
>> +//
>> +
>> +#include "core/event.hpp"
>> +
>> +using namespace clover;
>> +
>> +extern "C" {
>> +
>> +PUBLIC int
>> +opencl_dri_event_add_ref(intptr_t event)
>> +{
>> +   return clRetainEvent((cl_event)event) == CL_SUCCESS;
>> +}
>> +
>> +PUBLIC int
>> +opencl_dri_event_release(intptr_t event)
>> +{
>> +   return clReleaseEvent((cl_event)event) == CL_SUCCESS;
>> +}
>> +
>> +PUBLIC int+
>> +opencl_dri_event_wait(intptr_t cl_event, uint64_t timeout)
>> +{
>> +   event *e = (event*)cl_event;
>> +
>> +   if (!timeout) {
>> +      return e->status() == CL_COMPLETE;
>> +   }
>> +
>> +   e->wait();
>> +   return true;
>> +}
>> +
>> +PUBLIC struct pipe_fence_handle *
>> +opencl_dri_event_get_fence(intptr_t cl_event)
>> +{
>> +   hard_event *e = dynamic_cast<hard_event*>((event*)cl_event);
>> +
>> +   if (e)
>> +      return e->get_fence();
>> +
>> +   return NULL;
>> +}
>> +
> It seems that the above public functions are lacking the respective
> declarations. Iirc C++ explicitly warns about that, although even if
> disabled (by us or the user) it might be worth adding the couple of lines ?
>
> -Emil
On 14/04/15 22:19, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> ---
>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>  src/gallium/targets/opencl/opencl.sym              |  1 +
>  5 files changed, 114 insertions(+)
>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
> 
> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
> new file mode 100644
> index 0000000..31a3bd3
> --- /dev/null
> +++ b/src/gallium/include/state_tracker/opencl_interop.h
> @@ -0,0 +1,42 @@
> +/**************************************************************************
> + *
> + * Copyright 2015 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#ifndef OPENCL_INTEROP_H
> +#define OPENCL_INTEROP_H
> +
> +/* dlsym these without the "_t" suffix. You should get the correct symbols
> + * if the OpenCL driver is loaded.
> + *
> + * All functions return non-zero on success.
> + */
> +
> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
> +
> +#endif /* OPENCL_INTEROP_H */
> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
> index 5b3344c..4c2d0f3 100644
> --- a/src/gallium/state_trackers/clover/Makefile.sources
> +++ b/src/gallium/state_trackers/clover/Makefile.sources
> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>  	core/event.hpp \
>  	core/format.cpp \
>  	core/format.hpp \
> +	core/interop.cpp \
>  	core/kernel.cpp \
>  	core/kernel.hpp \
>  	core/memory.cpp \
> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
> index 0e1359a..28f510f 100644
> --- a/src/gallium/state_trackers/clover/core/event.hpp
> +++ b/src/gallium/state_trackers/clover/core/event.hpp
> @@ -116,6 +116,10 @@ namespace clover {
>  
>        friend class command_queue;
>  
> +      struct pipe_fence_handle *get_fence() const {
> +         return _fence;
> +      }
> +
>     private:
>        virtual void fence(pipe_fence_handle *fence);
>        action profile(command_queue &q, const action &action) const;
> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
> new file mode 100644
> index 0000000..bb80cf5
> --- /dev/null
> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
> @@ -0,0 +1,66 @@
> +//
> +// Copyright 2015 Advanced Micro Devices, Inc.
> +// All Rights Reserved.
> +//
> +// Permission is hereby granted, free of charge, to any person obtaining a
> +// copy of this software and associated documentation files (the "Software"),
> +// to deal in the Software without restriction, including without limitation
> +// the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +// and/or sell copies of the Software, and to permit persons to whom the
> +// Software is furnished to do so, subject to the following conditions:
> +//
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +//
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> +// OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +// ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +// OTHER DEALINGS IN THE SOFTWARE.
> +//
> +
> +#include "core/event.hpp"
> +
> +using namespace clover;
> +
> +extern "C" {
> +
> +PUBLIC int
> +opencl_dri_event_add_ref(intptr_t event)
> +{
> +   return clRetainEvent((cl_event)event) == CL_SUCCESS;
> +}
> +
> +PUBLIC int
> +opencl_dri_event_release(intptr_t event)
> +{
> +   return clReleaseEvent((cl_event)event) == CL_SUCCESS;
> +}
> +
> +PUBLIC int+
> +opencl_dri_event_wait(intptr_t cl_event, uint64_t timeout)
> +{
> +   event *e = (event*)cl_event;
> +
> +   if (!timeout) {
> +      return e->status() == CL_COMPLETE;
> +   }
> +
> +   e->wait();
> +   return true;
> +}
> +
> +PUBLIC struct pipe_fence_handle *
> +opencl_dri_event_get_fence(intptr_t cl_event)
> +{
> +   hard_event *e = dynamic_cast<hard_event*>((event*)cl_event);
> +
> +   if (e)
> +      return e->get_fence();
> +
> +   return NULL;
> +}
> +
It seems that the above public functions are lacking the respective
declarations. Iirc C++ explicitly warns about that, although even if
disabled (by us or the user) it might be worth adding the couple of lines ?

-Emil
Marek Olšák <maraeo@gmail.com> writes:

> From: Marek Olšák <marek.olsak@amd.com>
>
Hi Marek, looks mostly OK to me, a few nits inline,

> ---
>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>  src/gallium/targets/opencl/opencl.sym              |  1 +
>  5 files changed, 114 insertions(+)
>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>
> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
> new file mode 100644
> index 0000000..31a3bd3
> --- /dev/null
> +++ b/src/gallium/include/state_tracker/opencl_interop.h
> @@ -0,0 +1,42 @@
> +/**************************************************************************
> + *
> + * Copyright 2015 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#ifndef OPENCL_INTEROP_H
> +#define OPENCL_INTEROP_H
> +
> +/* dlsym these without the "_t" suffix. You should get the correct symbols
> + * if the OpenCL driver is loaded.
> + *
> + * All functions return non-zero on success.
> + */
> +
> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
> +

Just curious, is there any reason you use intptr_t here and elsewhere
rather than cl_event or void *?  The former is an typedef of an opaque
pointer anyway.  Using CL types would likely avoid some "ugly" casts
later on.  And maybe bool as return type?

> +#endif /* OPENCL_INTEROP_H */
> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
> index 5b3344c..4c2d0f3 100644
> --- a/src/gallium/state_trackers/clover/Makefile.sources
> +++ b/src/gallium/state_trackers/clover/Makefile.sources
> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>  	core/event.hpp \
>  	core/format.cpp \
>  	core/format.hpp \
> +	core/interop.cpp \
>  	core/kernel.cpp \
>  	core/kernel.hpp \
>  	core/memory.cpp \
> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
> index 0e1359a..28f510f 100644
> --- a/src/gallium/state_trackers/clover/core/event.hpp
> +++ b/src/gallium/state_trackers/clover/core/event.hpp
> @@ -116,6 +116,10 @@ namespace clover {
>  
>        friend class command_queue;
>  
> +      struct pipe_fence_handle *get_fence() const {

The convention in the surrounding code is to name such accessors as the
object they access, how about "pipe_fence_handle *fence() const"?

> +         return _fence;
> +      }
> +
>     private:
>        virtual void fence(pipe_fence_handle *fence);
>        action profile(command_queue &q, const action &action) const;
> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
> new file mode 100644
> index 0000000..bb80cf5
> --- /dev/null
> +++ b/src/gallium/state_trackers/clover/core/interop.cpp

This probably belongs in clover/api/, as it's technically implementing
an API, clover/core/ is all about core data structures and such.

> @@ -0,0 +1,66 @@
> +//
> +// Copyright 2015 Advanced Micro Devices, Inc.
> +// All Rights Reserved.
> +//
> +// Permission is hereby granted, free of charge, to any person obtaining a
> +// copy of this software and associated documentation files (the "Software"),
> +// to deal in the Software without restriction, including without limitation
> +// the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +// and/or sell copies of the Software, and to permit persons to whom the
> +// Software is furnished to do so, subject to the following conditions:
> +//
> +// The above copyright notice and this permission notice shall be included in
> +// all copies or substantial portions of the Software.
> +//
> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> +// OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +// ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +// OTHER DEALINGS IN THE SOFTWARE.
> +//
> +
> +#include "core/event.hpp"
> +
> +using namespace clover;
> +
> +extern "C" {
> +
> +PUBLIC int
> +opencl_dri_event_add_ref(intptr_t event)
> +{
> +   return clRetainEvent((cl_event)event) == CL_SUCCESS;
> +}
> +
> +PUBLIC int
> +opencl_dri_event_release(intptr_t event)
> +{
> +   return clReleaseEvent((cl_event)event) == CL_SUCCESS;
> +}
> +
> +PUBLIC int
> +opencl_dri_event_wait(intptr_t cl_event, uint64_t timeout)
> +{
> +   event *e = (event*)cl_event;
> +
> +   if (!timeout) {
> +      return e->status() == CL_COMPLETE;
> +   }
> +
> +   e->wait();
> +   return true;
> +}
> +
> +PUBLIC struct pipe_fence_handle *
> +opencl_dri_event_get_fence(intptr_t cl_event)
> +{
> +   hard_event *e = dynamic_cast<hard_event*>((event*)cl_event);
> +
> +   if (e)
> +      return e->get_fence();
> +
> +   return NULL;
> +}
> +
> +}
> diff --git a/src/gallium/targets/opencl/opencl.sym b/src/gallium/targets/opencl/opencl.sym
> index ee8aacf..9fcc576 100644
> --- a/src/gallium/targets/opencl/opencl.sym
> +++ b/src/gallium/targets/opencl/opencl.sym
> @@ -1,6 +1,7 @@
>  {
>  	global:
>  		cl*;
> +		opencl_dri_*;
>  	local:
>  		*;
>  };
> -- 
> 2.1.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Hmm pretty sure I've read somewhere (old version of GCC's manual?) that
such warnings are enabled by default and cannot be controlled by the
compiler (for C++ of course). I'm not an C++ expect so it could be that
the standard does not require declarations/prototypes for non static
functions ?

Seems like we're building clover without even a single -W flag - perhaps
we could add some (not with this patch of course) ?

-Emil

On 21/04/15 13:13, Marek Olšák wrote:
> Only C warns about that. I didn't see any warning with C++.
> 
> Marek
> 
> On Tue, Apr 21, 2015 at 3:44 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 14/04/15 22:19, Marek Olšák wrote:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> ---
>>>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>>>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>>>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>>>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>>>  src/gallium/targets/opencl/opencl.sym              |  1 +
>>>  5 files changed, 114 insertions(+)
>>>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>>>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>>>
>>> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
>>> new file mode 100644
>>> index 0000000..31a3bd3
>>> --- /dev/null
>>> +++ b/src/gallium/include/state_tracker/opencl_interop.h
>>> @@ -0,0 +1,42 @@
>>> +/**************************************************************************
>>> + *
>>> + * Copyright 2015 Advanced Micro Devices, Inc.
>>> + * All Rights Reserved.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial portions
>>> + * of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + **************************************************************************/
>>> +
>>> +#ifndef OPENCL_INTEROP_H
>>> +#define OPENCL_INTEROP_H
>>> +
>>> +/* dlsym these without the "_t" suffix. You should get the correct symbols
>>> + * if the OpenCL driver is loaded.
>>> + *
>>> + * All functions return non-zero on success.
>>> + */
>>> +
>>> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
>>> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
>>> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
>>> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
>>> +
>>> +#endif /* OPENCL_INTEROP_H */
>>> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
>>> index 5b3344c..4c2d0f3 100644
>>> --- a/src/gallium/state_trackers/clover/Makefile.sources
>>> +++ b/src/gallium/state_trackers/clover/Makefile.sources
>>> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>>>       core/event.hpp \
>>>       core/format.cpp \
>>>       core/format.hpp \
>>> +     core/interop.cpp \
>>>       core/kernel.cpp \
>>>       core/kernel.hpp \
>>>       core/memory.cpp \
>>> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
>>> index 0e1359a..28f510f 100644
>>> --- a/src/gallium/state_trackers/clover/core/event.hpp
>>> +++ b/src/gallium/state_trackers/clover/core/event.hpp
>>> @@ -116,6 +116,10 @@ namespace clover {
>>>
>>>        friend class command_queue;
>>>
>>> +      struct pipe_fence_handle *get_fence() const {
>>> +         return _fence;
>>> +      }
>>> +
>>>     private:
>>>        virtual void fence(pipe_fence_handle *fence);
>>>        action profile(command_queue &q, const action &action) const;
>>> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
>>> new file mode 100644
>>> index 0000000..bb80cf5
>>> --- /dev/null
>>> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
>>> @@ -0,0 +1,66 @@
>>> +//
>>> +// Copyright 2015 Advanced Micro Devices, Inc.
>>> +// All Rights Reserved.
>>> +//
>>> +// Permission is hereby granted, free of charge, to any person obtaining a
>>> +// copy of this software and associated documentation files (the "Software"),
>>> +// to deal in the Software without restriction, including without limitation
>>> +// the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> +// and/or sell copies of the Software, and to permit persons to whom the
>>> +// Software is furnished to do so, subject to the following conditions:
>>> +//
>>> +// The above copyright notice and this permission notice shall be included in
>>> +// all copies or substantial portions of the Software.
>>> +//
>>> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> +// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>> +// OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> +// ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> +// OTHER DEALINGS IN THE SOFTWARE.
>>> +//
>>> +
>>> +#include "core/event.hpp"
>>> +
>>> +using namespace clover;
>>> +
>>> +extern "C" {
>>> +
>>> +PUBLIC int
>>> +opencl_dri_event_add_ref(intptr_t event)
>>> +{
>>> +   return clRetainEvent((cl_event)event) == CL_SUCCESS;
>>> +}
>>> +
>>> +PUBLIC int
>>> +opencl_dri_event_release(intptr_t event)
>>> +{
>>> +   return clReleaseEvent((cl_event)event) == CL_SUCCESS;
>>> +}
>>> +
>>> +PUBLIC int+
>>> +opencl_dri_event_wait(intptr_t cl_event, uint64_t timeout)
>>> +{
>>> +   event *e = (event*)cl_event;
>>> +
>>> +   if (!timeout) {
>>> +      return e->status() == CL_COMPLETE;
>>> +   }
>>> +
>>> +   e->wait();
>>> +   return true;
>>> +}
>>> +
>>> +PUBLIC struct pipe_fence_handle *
>>> +opencl_dri_event_get_fence(intptr_t cl_event)
>>> +{
>>> +   hard_event *e = dynamic_cast<hard_event*>((event*)cl_event);
>>> +
>>> +   if (e)
>>> +      return e->get_fence();
>>> +
>>> +   return NULL;
>>> +}
>>> +
>> It seems that the above public functions are lacking the respective
>> declarations. Iirc C++ explicitly warns about that, although even if
>> disabled (by us or the user) it might be worth adding the couple of lines ?
>>
>> -Emil
On Tue, Apr 21, 2015 at 5:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hmm pretty sure I've read somewhere (old version of GCC's manual?) that
> such warnings are enabled by default and cannot be controlled by the
> compiler (for C++ of course). I'm not an C++ expect so it could be that
> the standard does not require declarations/prototypes for non static
> functions ?

A prototype is only required by C++ when a function is called.
Functions don't need prototypes for themselves. Only their call sites
need them.

>
> Seems like we're building clover without even a single -W flag - perhaps
> we could add some (not with this patch of course) ?

Yes, I agree.

Marek
On Tue, Apr 21, 2015 at 3:50 PM, Francisco Jerez <currojerez@riseup.net> wrote:
> Marek Olšák <maraeo@gmail.com> writes:
>
>> From: Marek Olšák <marek.olsak@amd.com>
>>
> Hi Marek, looks mostly OK to me, a few nits inline,
>
>> ---
>>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>>  src/gallium/targets/opencl/opencl.sym              |  1 +
>>  5 files changed, 114 insertions(+)
>>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>>
>> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
>> new file mode 100644
>> index 0000000..31a3bd3
>> --- /dev/null
>> +++ b/src/gallium/include/state_tracker/opencl_interop.h
>> @@ -0,0 +1,42 @@
>> +/**************************************************************************
>> + *
>> + * Copyright 2015 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction, including
>> + * without limitation the rights to use, copy, modify, merge, publish,
>> + * distribute, sub license, and/or sell copies of the Software, and to
>> + * permit persons to whom the Software is furnished to do so, subject to
>> + * the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + **************************************************************************/
>> +
>> +#ifndef OPENCL_INTEROP_H
>> +#define OPENCL_INTEROP_H
>> +
>> +/* dlsym these without the "_t" suffix. You should get the correct symbols
>> + * if the OpenCL driver is loaded.
>> + *
>> + * All functions return non-zero on success.
>> + */
>> +
>> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
>> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
>> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
>> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
>> +
>
> Just curious, is there any reason you use intptr_t here and elsewhere
> rather than cl_event or void *?  The former is an typedef of an opaque
> pointer anyway.  Using CL types would likely avoid some "ugly" casts
> later on.  And maybe bool as return type?

I'll change intptr_t to void*. I don't trust that bool is the same in
both C and C++ and "int" seems to be a safe choice for an ABI.

>
>> +#endif /* OPENCL_INTEROP_H */
>> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
>> index 5b3344c..4c2d0f3 100644
>> --- a/src/gallium/state_trackers/clover/Makefile.sources
>> +++ b/src/gallium/state_trackers/clover/Makefile.sources
>> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>>       core/event.hpp \
>>       core/format.cpp \
>>       core/format.hpp \
>> +     core/interop.cpp \
>>       core/kernel.cpp \
>>       core/kernel.hpp \
>>       core/memory.cpp \
>> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
>> index 0e1359a..28f510f 100644
>> --- a/src/gallium/state_trackers/clover/core/event.hpp
>> +++ b/src/gallium/state_trackers/clover/core/event.hpp
>> @@ -116,6 +116,10 @@ namespace clover {
>>
>>        friend class command_queue;
>>
>> +      struct pipe_fence_handle *get_fence() const {
>
> The convention in the surrounding code is to name such accessors as the
> object they access, how about "pipe_fence_handle *fence() const"?

OK.

>
>> +         return _fence;
>> +      }
>> +
>>     private:
>>        virtual void fence(pipe_fence_handle *fence);
>>        action profile(command_queue &q, const action &action) const;
>> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
>> new file mode 100644
>> index 0000000..bb80cf5
>> --- /dev/null
>> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
>
> This probably belongs in clover/api/, as it's technically implementing
> an API, clover/core/ is all about core data structures and such.

OK.

Marek
Marek Olšák <maraeo@gmail.com> writes:

> On Tue, Apr 21, 2015 at 3:50 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>> Marek Olšák <maraeo@gmail.com> writes:
>>
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>> Hi Marek, looks mostly OK to me, a few nits inline,
>>
>>> ---
>>>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>>>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>>>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>>>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>>>  src/gallium/targets/opencl/opencl.sym              |  1 +
>>>  5 files changed, 114 insertions(+)
>>>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>>>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>>>
>>> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
>>> new file mode 100644
>>> index 0000000..31a3bd3
>>> --- /dev/null
>>> +++ b/src/gallium/include/state_tracker/opencl_interop.h
>>> @@ -0,0 +1,42 @@
>>> +/**************************************************************************
>>> + *
>>> + * Copyright 2015 Advanced Micro Devices, Inc.
>>> + * All Rights Reserved.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial portions
>>> + * of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + *
>>> + **************************************************************************/
>>> +
>>> +#ifndef OPENCL_INTEROP_H
>>> +#define OPENCL_INTEROP_H
>>> +
>>> +/* dlsym these without the "_t" suffix. You should get the correct symbols
>>> + * if the OpenCL driver is loaded.
>>> + *
>>> + * All functions return non-zero on success.
>>> + */
>>> +
>>> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
>>> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
>>> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
>>> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
>>> +
>>
>> Just curious, is there any reason you use intptr_t here and elsewhere
>> rather than cl_event or void *?  The former is an typedef of an opaque
>> pointer anyway.  Using CL types would likely avoid some "ugly" casts
>> later on.  And maybe bool as return type?
>
> I'll change intptr_t to void*. I don't trust that bool is the same in
> both C and C++ and "int" seems to be a safe choice for an ABI.
>
Any sensible ABI [the same thing that guarantees that C's and C++'s
"int" are the same type ;)] that as far as I'm aware clover has ever
been compiled for should give identical representations to C's _Bool and
C++'s bool.  Not sure what your concern is?

>>
>>> +#endif /* OPENCL_INTEROP_H */
>>> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
>>> index 5b3344c..4c2d0f3 100644
>>> --- a/src/gallium/state_trackers/clover/Makefile.sources
>>> +++ b/src/gallium/state_trackers/clover/Makefile.sources
>>> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>>>       core/event.hpp \
>>>       core/format.cpp \
>>>       core/format.hpp \
>>> +     core/interop.cpp \
>>>       core/kernel.cpp \
>>>       core/kernel.hpp \
>>>       core/memory.cpp \
>>> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
>>> index 0e1359a..28f510f 100644
>>> --- a/src/gallium/state_trackers/clover/core/event.hpp
>>> +++ b/src/gallium/state_trackers/clover/core/event.hpp
>>> @@ -116,6 +116,10 @@ namespace clover {
>>>
>>>        friend class command_queue;
>>>
>>> +      struct pipe_fence_handle *get_fence() const {
>>
>> The convention in the surrounding code is to name such accessors as the
>> object they access, how about "pipe_fence_handle *fence() const"?
>
> OK.
>
>>
>>> +         return _fence;
>>> +      }
>>> +
>>>     private:
>>>        virtual void fence(pipe_fence_handle *fence);
>>>        action profile(command_queue &q, const action &action) const;
>>> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
>>> new file mode 100644
>>> index 0000000..bb80cf5
>>> --- /dev/null
>>> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
>>
>> This probably belongs in clover/api/, as it's technically implementing
>> an API, clover/core/ is all about core data structures and such.
>
> OK.
>
> Marek
Alright. I suppose it's okay to use bool, but hypothetically a gallium
driver could have an OpenCL stack that isn't clover and the interop
interface should work with it too.

Marek

On Mon, Apr 27, 2015 at 2:46 PM, Francisco Jerez <currojerez@riseup.net> wrote:
> Marek Olšák <maraeo@gmail.com> writes:
>
>> On Tue, Apr 21, 2015 at 3:50 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>> Marek Olšák <maraeo@gmail.com> writes:
>>>
>>>> From: Marek Olšák <marek.olsak@amd.com>
>>>>
>>> Hi Marek, looks mostly OK to me, a few nits inline,
>>>
>>>> ---
>>>>  src/gallium/include/state_tracker/opencl_interop.h | 42 ++++++++++++++
>>>>  src/gallium/state_trackers/clover/Makefile.sources |  1 +
>>>>  src/gallium/state_trackers/clover/core/event.hpp   |  4 ++
>>>>  src/gallium/state_trackers/clover/core/interop.cpp | 66 ++++++++++++++++++++++
>>>>  src/gallium/targets/opencl/opencl.sym              |  1 +
>>>>  5 files changed, 114 insertions(+)
>>>>  create mode 100644 src/gallium/include/state_tracker/opencl_interop.h
>>>>  create mode 100644 src/gallium/state_trackers/clover/core/interop.cpp
>>>>
>>>> diff --git a/src/gallium/include/state_tracker/opencl_interop.h b/src/gallium/include/state_tracker/opencl_interop.h
>>>> new file mode 100644
>>>> index 0000000..31a3bd3
>>>> --- /dev/null
>>>> +++ b/src/gallium/include/state_tracker/opencl_interop.h
>>>> @@ -0,0 +1,42 @@
>>>> +/**************************************************************************
>>>> + *
>>>> + * Copyright 2015 Advanced Micro Devices, Inc.
>>>> + * All Rights Reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> + * "Software"), to deal in the Software without restriction, including
>>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>>> + * permit persons to whom the Software is furnished to do so, subject to
>>>> + * the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> + * next paragraph) shall be included in all copies or substantial portions
>>>> + * of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>>> + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR
>>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>>> + *
>>>> + **************************************************************************/
>>>> +
>>>> +#ifndef OPENCL_INTEROP_H
>>>> +#define OPENCL_INTEROP_H
>>>> +
>>>> +/* dlsym these without the "_t" suffix. You should get the correct symbols
>>>> + * if the OpenCL driver is loaded.
>>>> + *
>>>> + * All functions return non-zero on success.
>>>> + */
>>>> +
>>>> +typedef int (*opencl_dri_event_add_ref_t)(intptr_t cl_event);
>>>> +typedef int (*opencl_dri_event_release_t)(intptr_t cl_event);
>>>> +typedef int (*opencl_dri_event_wait_t)(intptr_t cl_event, uint64_t timeout);
>>>> +typedef struct pipe_fence_handle *(*opencl_dri_event_get_fence_t)(intptr_t cl_event);
>>>> +
>>>
>>> Just curious, is there any reason you use intptr_t here and elsewhere
>>> rather than cl_event or void *?  The former is an typedef of an opaque
>>> pointer anyway.  Using CL types would likely avoid some "ugly" casts
>>> later on.  And maybe bool as return type?
>>
>> I'll change intptr_t to void*. I don't trust that bool is the same in
>> both C and C++ and "int" seems to be a safe choice for an ABI.
>>
> Any sensible ABI [the same thing that guarantees that C's and C++'s
> "int" are the same type ;)] that as far as I'm aware clover has ever
> been compiled for should give identical representations to C's _Bool and
> C++'s bool.  Not sure what your concern is?
>
>>>
>>>> +#endif /* OPENCL_INTEROP_H */
>>>> diff --git a/src/gallium/state_trackers/clover/Makefile.sources b/src/gallium/state_trackers/clover/Makefile.sources
>>>> index 5b3344c..4c2d0f3 100644
>>>> --- a/src/gallium/state_trackers/clover/Makefile.sources
>>>> +++ b/src/gallium/state_trackers/clover/Makefile.sources
>>>> @@ -22,6 +22,7 @@ CPP_SOURCES := \
>>>>       core/event.hpp \
>>>>       core/format.cpp \
>>>>       core/format.hpp \
>>>> +     core/interop.cpp \
>>>>       core/kernel.cpp \
>>>>       core/kernel.hpp \
>>>>       core/memory.cpp \
>>>> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
>>>> index 0e1359a..28f510f 100644
>>>> --- a/src/gallium/state_trackers/clover/core/event.hpp
>>>> +++ b/src/gallium/state_trackers/clover/core/event.hpp
>>>> @@ -116,6 +116,10 @@ namespace clover {
>>>>
>>>>        friend class command_queue;
>>>>
>>>> +      struct pipe_fence_handle *get_fence() const {
>>>
>>> The convention in the surrounding code is to name such accessors as the
>>> object they access, how about "pipe_fence_handle *fence() const"?
>>
>> OK.
>>
>>>
>>>> +         return _fence;
>>>> +      }
>>>> +
>>>>     private:
>>>>        virtual void fence(pipe_fence_handle *fence);
>>>>        action profile(command_queue &q, const action &action) const;
>>>> diff --git a/src/gallium/state_trackers/clover/core/interop.cpp b/src/gallium/state_trackers/clover/core/interop.cpp
>>>> new file mode 100644
>>>> index 0000000..bb80cf5
>>>> --- /dev/null
>>>> +++ b/src/gallium/state_trackers/clover/core/interop.cpp
>>>
>>> This probably belongs in clover/api/, as it's technically implementing
>>> an API, clover/core/ is all about core data structures and such.
>>
>> OK.
>>
>> Marek