Marek Olšák <mar...@gmail.com> writes: > On Tue, Apr 21, 2015 at 3:50 PM, Francisco Jerez <curroje...@riseup.net> > wrote: >> Marek Olšák <mar...@gmail.com> writes: >> >>> From: Marek Olšák <marek.ol...@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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev