On Aug 20, 2016 5:00 AM, "Emil Velikov" <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote: > > On 20 August 2016 at 07:25, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > On Aug 19, 2016 18:05, "Eric Engestrom" <<a href="mailto:eric.engestrom@imgtec.com">eric.engestrom@imgtec.com</a>> wrote: > >> > >> On Fri, Aug 19, 2016 at 09:04:14AM -0700, Jason Ekstrand wrote: > >> > Not providing a path allows the ICD to work on multi-arch systems but > >> > breaks it if you install anywhere other than /usr/lib. Given that users > >> > may be installing locally in .local or similar, we probably do want to > >> > provide a filename. Distros can carry a revert of this commit if they > >> > want > >> > an intel_icd.json file without the path. > >> > > Not sure I understand what this patch fixes that the Vulkan spec does > not have a recommendation or two for. Can anyone elaborate step by > step what we're having here, please ? > > Furthermore this sounds like a developer only thing, so perhaps we > should restrict it there ? As-is we're effectively annoying some/most > distros. That a sticky question. I believe including the full path is the "correct" thing to do. That is what the ICD file is there for after all. Distros, however, may choose to do differently for a packaged system-wide install due to multi-arch issues. The problem with a full path is because of an oversight in the defining of the ICD file format, namely that they provided no way of specifying the architecture of the .so. This makes it very difficult to install both 32 and 64-bit versions side-by-side. The solution that ajax chose to use in his early fedora packages was to place libvulkan_Intel.so in the regular library install path and not specify any path beyond the filename. This way the loader will call dlopen("libvulkan_Intel.so") and regular search paths will be used. Since the operating system sets those paths up correctly, the correct version of libvulkan_Intel.so is loaded and off you go. Personally, I think the more correct way to handle this would be with an architecture field in the ICD json file. Proposing that and fixing up the loader to handle has been on my to-do list for some time but has been eclipsed by other things. > >> > Signed-off-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > >> > Cc: Mark Janes <<a href="mailto:mark.a.janes@intel.com">mark.a.janes@intel.com</a>> > >> > >> I have one question (below), but this patch is good regardless: > >> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com">eric.engestrom@imgtec.com</a>> > >> > >> > --- > >> > src/intel/vulkan/.gitignore | 1 + > >> > src/intel/vulkan/Makefile.am | 7 ++++++- > >> > src/intel/vulkan/{intel_icd.json => <a href="http://intel_icd.json.in">intel_icd.json.in</a>} | 2 +- > >> > 3 files changed, 8 insertions(+), 2 deletions(-) > >> > rename src/intel/vulkan/{intel_icd.json => <a href="http://intel_icd.json.in">intel_icd.json.in</a>} (59%) > >> > > >> > diff --git a/src/intel/vulkan/.gitignore b/src/intel/vulkan/.gitignore > >> > index bde5cd8..a099ff6 100644 > >> > --- a/src/intel/vulkan/.gitignore > >> > +++ b/src/intel/vulkan/.gitignore > >> > @@ -3,3 +3,4 @@ > >> > /anv_entrypoints.h > >> > /anv_timestamp.h > >> > /dev_icd.json > >> > +/intel_icd.json > >> > diff --git a/src/intel/vulkan/Makefile.am b/src/intel/vulkan/Makefile.am > >> > index ad0148d..9fef960 100644 > >> > --- a/src/intel/vulkan/Makefile.am > >> > +++ b/src/intel/vulkan/Makefile.am > >> > @@ -141,7 +141,7 @@ anv_timestamp.h: > >> > $(AM_V_GEN) echo "#define ANV_TIMESTAMP \"$(TIMESTAMP_CMD)\"" > $@ > >> > > >> > BUILT_SOURCES = $(VULKAN_GENERATED_FILES) > >> > -CLEANFILES = $(BUILT_SOURCES) dev_icd.json > >> > +CLEANFILES = $(BUILT_SOURCES) dev_icd.json intel_icd.json > >> > EXTRA_DIST = \ > >> > $(top_srcdir)/include/vulkan/vk_icd.h \ > >> > anv_entrypoints_gen.py \ > >> > @@ -170,6 +170,11 @@ dev_icd.json : <a href="http://dev_icd.json.in">dev_icd.json.in</a> > >> > -e "s#@build_libdir@#${abs_top_builddir}/${LIB_DIR}#" \ > >> > < $(srcdir)/<a href="http://dev_icd.json.in">dev_icd.json.in</a> > $@ > >> > > >> > +intel_icd.json : <a href="http://intel_icd.json.in">intel_icd.json.in</a> > >> > + $(AM_V_GEN) $(SED) \ > >> > + -e "s#@install_libdir@#${libdir}#" \ > >> > + < $(srcdir)/<a href="http://intel_icd.json.in">intel_icd.json.in</a> > $@ > >> > >> I think I may have already asked when dev_icd.json was added, but why > >> use a relative path for the dependency and a full path when reading it? > >> Why not use the full path for the dep and read `< $<` instead? > > > > No good reason that I know of other than copy+paste of the one above. We > > could probably make that change. Emil? > > > IIRC '< $<' was only applicable in some cases according to POSIX make. > One the relative/full path, I think that non-gnumake implementations > were having issues with the proposed solution. If anyone can confirm > that it works I'm ok with changing it. > > Thanks > Emil