[Mesa-dev] [RFC] opencl: mega-cl

[Rob Clark]

On Sun, Feb 25, 2018 at 3:00 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Seems like a serious hack to me to work around broken linking...  IMO we
> should just fix the linking issue.  The symbols for the various GLSL
> types need to be linked with the proper binding and visibility -- I
> assume that the cause of your problem is that people are making
> assumptions about the equality of GLSL types based on their memory
> addresses *and* marking the symbols as hidden *and* passing pointers to
> GLSL types across shared objects?  That sounds like a recipe for
> disaster.

tbh, maybe hack, or I think more likely, maybe a good idea.. I'm not
terribly sold on the idea of dynamically loading pipe driver and
linking a lot of shared code into N different pipe_${driver}.so on
disk, since the # of drivers seems to be greater than # of state
trackers.. not to mention multiple copies of shared gallium code in
memory due to being statically linked into both state tracker and
driver.

That said, glsl_type's defn does seem to expect to only exist once in
a process, ie. == is ptr comparison and when you link nir/glsl_types
into both state tracker and driver, glsl_type ptrs are getting passed
across that boundary.  I'm not really sure that is worth fixing (ie.
why should it exist twice in a process in the first place?)

Maybe there are some linker tricks to solve this, idk.. that is a bit
outside my area of expertise.  To me, the mega-driver approach worked
well for gl, and many of the other state trackers are already using
libpipe_loader_static.. why not just fix the problems with that
approach and abandon libpipe_loader_dynamic?  This seems like it would
work out better for memory footprint since pages for drivers not used
simply wouldn't get paged in.  And you aren't ending up with two
copies of libgallium or libnir code in memory.

BR,
-R

> The current code is likely being correctly linked based on the
> assumption that the third condition from my question above doesn't hold
> (i.e. that the hidden symbols are really being kept local to the GLSL
> compiler), but your branch probably breaks this assumption?  It's a bug
> to pass around pointers to hidden symbols to other shared objects except
> where you know with confidence that the identity of the objects you're
> passing around (as in their memory address) is unimportant.
>
> Rob Clark <robdclark at gmail.com> writes:
>
>> This is a sort of workaround for an issue encountered with the ongoing
>> work for clover clc->spirv->nir work for supporting OpenCL with drivers
>> that support consuming nir instead of llvm-ir.  The problem is that
>> libnir (and therefore glsl_types) ends up statically linked into both
>> clover and the pipe driver it dynamically loads.  Which causes *much*
>> hillarity.
>>
>> An easy solution is to statically link the drivers into clover.  In fact
>> I'm not even really sure what other solution there is, but maybe there
>> is some linker magic I am not aware of.
>>
>> (Note, if mesa state tracker ever used libpipe_loader_dynamic, I think
>> it would hit the same problem.  If we still support that config, we
>> should drop it ASAP.)
>>
>> The down side is we end up statically linking in drivers and other
>> things that don't make sense (ie. only support TGSI or don't support
>> compute shaders, etc).  I'm thinking a sane option here is to have for
>> each driver, "driver_${name}" plus "driver_${name}_stub", the stub
>> containing an entry point that just returns an error.  (Maybe also do
>> similar for winsys and some other things?)  This way, a target like
>> opencl can pick either the real driver or the stub as needed.
>>
>> Btw, I'm kinda thinking we should drop libpipe_loader_dynamic
>> completely, only only use libpipe_loader_static, and use the stub
>> drivers to avoid pulling in stuff that doesn't make sense.  (I noticed,
>> for example, the omx target statically links a bunch of drivers that
>> don't support the video related gallium APIs.. this stub driver approach
>> would help there too.)
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  src/gallium/targets/opencl/meson.build | 18 +++++++++++++++---
>>  src/gallium/targets/opencl/target.c    |  2 ++
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>  create mode 100644 src/gallium/targets/opencl/target.c
>>
>> diff --git a/src/gallium/targets/opencl/meson.build b/src/gallium/targets/opencl/meson.build
>> index bebe0547d45..8d5fd81b55e 100644
>> --- a/src/gallium/targets/opencl/meson.build
>> +++ b/src/gallium/targets/opencl/meson.build
>> @@ -35,13 +35,19 @@ opencl_libname = with_opencl_icd ? 'MesaOpenCL' : 'OpenCL'
>>
>>  libopencl = shared_library(
>>    opencl_libname,
>> -  [],
>> +  ['target.c'],
>> +  include_directories : [
>> +    inc_common, inc_util, inc_gallium_winsys, inc_gallium_drivers,
>> +  ],
>>    link_args : [ld_args_gc_sections, opencl_link_args],
>>    link_depends : opencl_link_deps,
>>    link_whole : libclover,
>> -  link_with : [libpipe_loader_dynamic, libgallium, libmesa_util],
>> +  link_with : [
>> +    libpipe_loader_static, libgallium, libmesa_util,
>> +    libgalliumvl, libws_null, libwsw, libswdri, libswkmsdri,
>> +  ],
>>    dependencies : [
>> -    dep_thread, dep_clock, dep_dl, dep_unwind, dep_elf, dep_expat,
>> +    dep_libdrm, dep_thread, dep_clock, dep_dl, dep_unwind, dep_elf, dep_expat,
>>      cpp.find_library('clangCodeGen', dirs : llvm_libdir),
>>      cpp.find_library('clangFrontendTool', dirs : llvm_libdir),
>>      cpp.find_library('clangFrontend', dirs : llvm_libdir),
>> @@ -54,6 +60,12 @@ libopencl = shared_library(
>>      cpp.find_library('clangEdit', dirs : llvm_libdir),
>>      cpp.find_library('clangLex', dirs : llvm_libdir),
>>      cpp.find_library('clangBasic', dirs : llvm_libdir),
>> +    # TODO only a small subset of these makes sense, but libpipe_loader_static
>> +    # depends on all.. maybe there is a way to have stub versions for the
>> +    # drivers which don't support OpenCL?
>> +    driver_swrast, driver_r300, driver_r600, driver_radeonsi, driver_nouveau,
>> +    driver_pl111, driver_vc4, driver_vc5, driver_freedreno, driver_etnaviv,
>> +    driver_imx, driver_i915, driver_svga, driver_virgl, driver_swr,
>>    ],
>>    version : opencl_version,
>>    install : true,
>> diff --git a/src/gallium/targets/opencl/target.c b/src/gallium/targets/opencl/target.c
>> new file mode 100644
>> index 00000000000..308e23bb4a0
>> --- /dev/null
>> +++ b/src/gallium/targets/opencl/target.c
>> @@ -0,0 +1,2 @@
>> +#include "target-helpers/drm_helper.h"
>> +#include "target-helpers/sw_helper.h"
>> --
>> 2.14.3