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

Mon Apr 27 06:48:20 PDT 2015

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 at riseup.net> wrote:
> Marek Olšák <maraeo at gmail.com> writes:
>
>> On Tue, Apr 21, 2015 at 3:50 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Marek Olšák <maraeo at gmail.com> writes:
>>>
>>>> From: Marek Olšák <marek.olsak at 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