https://bugzilla.redhat.com/show_bug.cgi?id=2036135
Bug ID: 2036135 Summary: Review Request: intel-compute-runtime - Compute API support for Intel(R) graphics Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: fzatlouk@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
František Zatloukal fzatlouk@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |2036099
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2036099 [Bug 2036099] Review Request: intel-igc - Intel(R) Graphics Compiler for OpenCL(TM)
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
František Zatloukal fzatlouk@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1942132
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1942132 [Bug 1942132] Review Request: intel-media-driver-free - The Intel Media Driver for VAAPI
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
Dominik 'Rathann' Mierzejewski dominik@greysector.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? Assignee|nobody@fedoraproject.org |dominik@greysector.net Status|NEW |ASSIGNED
--- Comment #1 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- I'm interested in OpenCL support for newer Intel GPUs and I was going to package this, but it's great you beat me to it. I'll help with reviews.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #2 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- Initial review:
BuildRequires: g++
Shouldn't this be: gcc-g++ ?
BuildRequires: intel-gmmlib BuildRequires: intel-gmmlib-devel BuildRequires: libva-devel BuildRequires: intel-igc BuildRequires: intel-igc-devel
I'd expect the -devel packages to have Requires: on their non-devel parts, so are the non-devel ones (intel-gmmlib and intel-igc) strictly necessary?
Version: 21.50.21939
...
-DNEO_OCL_VERSION_MAJOR=21 \ -DNEO_OCL_VERSION_MINOR=50 \ -DNEO_VERSION_BUILD=21939 \
This begs for automation, otherwise it's prone to missed updates in both places. Either construct Version: from these three components or extract the right component from Version when passing to cmake.
-DCMAKE_BUILD_TYPE=Release \ -DBUILD_SHARED_LIBS:BOOL=OFF \ -DCMAKE_BUILD_TYPE=Release \
You're passing CMAKE_BUOLD_TYPE twice.
%if 0%{?__isa_bits} == 64 -DCMAKE_INSTALL_LIBDIR=lib64 \ %else -DCMAKE_INSTALL_LIBDIR=lib \ %endif
%cmake macro contains this: -DLIB_INSTALL_DIR:PATH=/usr/lib64 \ ... %if "lib64" == "lib64" -DLIB_SUFFIX=64 \ %endif so the above should not be necessary and if it is, it's a bug in CMakeLists.
-DSKIP_UNIT_TESTS=1 \
How hard would it be to build and run the tests?
%{_libdir}/libocloc.so
Unversioned .so in %{_libdir} is questionable[1]. Please explain why it's OK.
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
%global optflags %{optflags} -Wno-error=odr
...
-Wno-dev \
Why are you passing one flag in optflags and another in %cmake? This seems inconsistent.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #3 from František Zatloukal fzatlouk@redhat.com --- SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #4 from František Zatloukal fzatlouk@redhat.com --- (In reply to Dominik 'Rathann' Mierzejewski from comment #2)
Initial review:
BuildRequires: g++
Shouldn't this be: gcc-g++ ?
Fixed
BuildRequires: intel-gmmlib BuildRequires: intel-gmmlib-devel BuildRequires: libva-devel BuildRequires: intel-igc BuildRequires: intel-igc-devel
I'd expect the -devel packages to have Requires: on their non-devel parts, so are the non-devel ones (intel-gmmlib and intel-igc) strictly necessary?
Fixed
Version: 21.50.21939
...
-DNEO_OCL_VERSION_MAJOR=21 \ -DNEO_OCL_VERSION_MINOR=50 \ -DNEO_VERSION_BUILD=21939 \
This begs for automation, otherwise it's prone to missed updates in both places. Either construct Version: from these three components or extract the right component from Version when passing to cmake.
Fixed
-DCMAKE_BUILD_TYPE=Release \ -DBUILD_SHARED_LIBS:BOOL=OFF \ -DCMAKE_BUILD_TYPE=Release \
You're passing CMAKE_BUOLD_TYPE twice.
Fixed
%if 0%{?__isa_bits} == 64 -DCMAKE_INSTALL_LIBDIR=lib64 \ %else -DCMAKE_INSTALL_LIBDIR=lib \ %endif
%cmake macro contains this: -DLIB_INSTALL_DIR:PATH=/usr/lib64 \ ... %if "lib64" == "lib64" -DLIB_SUFFIX=64 \ %endif so the above should not be necessary and if it is, it's a bug in CMakeLists.
Fixed
-DSKIP_UNIT_TESTS=1 \
How hard would it be to build and run the tests?
Tests are currently failing on multiple places on copy commands, which makes it seem like they're not maintained like they should be (or it may be caused by DNEO_DISABLE_BUILTINS_COMPILATION), I'll report upstream.
%{_libdir}/libocloc.so
Unversioned .so in %{_libdir} is questionable[1]. Please explain why it's OK.
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
Upstream doesn't version .so files by default. I'll ask about it, shall I go on with plain mv & ln ?
%global optflags %{optflags} -Wno-error=odr
...
-Wno-dev \
Why are you passing one flag in optflags and another in %cmake? This seems inconsistent.
Fixed.
Also, bumped the package to the latest tag, fixed gcc12 build.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #5 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- It looks like it needs a runtime dependency on intel-igc. Without intel-igc installed, it doesn't do anything under clpeak: $ strace -f clpeak ... [pid 17433] openat(AT_FDCWD, "/lib64/libigdfcl.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory) [pid 17433] openat(AT_FDCWD, "/usr/lib64/libigdfcl.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory) [pid 17433] munmap(0x7fabf564d000, 53975) = 0 [pid 17433] openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3 [pid 17433] newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=53975, ...}, AT_EMPTY_PATH) = 0 [pid 17433] mmap(NULL, 53975, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7fabf564d000 [pid 17433] close(3) = 0 [pid 17433] openat(AT_FDCWD, "/lib64/libigc.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory) [pid 17433] openat(AT_FDCWD, "/usr/lib64/libigc.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
However, once added, it segfaults. clinfo segfaults as well.
Now, I'm doing this on F35 with your RPMs from COPR installed, so it's possible there's some library mismatch.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #6 from František Zatloukal fzatlouk@redhat.com --- (In reply to Dominik 'Rathann' Mierzejewski from comment #5)
It looks like it needs a runtime dependency on intel-igc.
I'll add that.
However, once added, it segfaults. clinfo segfaults as well.
Yeah, this is kinda expected right now. It's due to https://github.com/intel/intel-graphics-compiler/issues/204 (or, by me working around that by -DNEO_DISABLE_BUILTINS_COMPILATION=ON, at least if I understood it right). Sorry, I somehow must've assumed you've seen this ticket. According to https://github.com/intel/intel-graphics-compiler/issues/225 , it might not take too long before newer llvm get properly supported, but before that, the compute-runtime package will be useful only for vaapi driver compilation.
The other possibility to get this working is to get one commit reverted from llvm, or try https://github.com/intel/intel-graphics-compiler/issues/204#issuecomment-100... , but I'd rather try waiting, hopefully it'll get resolved sooner than later upstream.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #7 from Neal Gompa ngompa13@gmail.com --- (In reply to František Zatloukal from comment #4)
(In reply to Dominik 'Rathann' Mierzejewski from comment #2)
Initial review:
BuildRequires: g++
Shouldn't this be: gcc-g++ ?
Fixed
This should be gcc-c++.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #8 from František Zatloukal fzatlouk@redhat.com --- SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #9 from František Zatloukal fzatlouk@redhat.com --- (In reply to Neal Gompa from comment #7)
This should be gcc-c++.
Addressed, intel-igc dependency added.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #10 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- You forgot to add the changelog. :)
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #11 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- There's lots of bundled stuff in third_party/. In particular:
third_party/opencl_headers third_party/opengl_headers can be removed and system ones used instead. Add Requires: libglvnd-devel Requires: ocl-icd-devel and patch appropriately.
third_party/gtest It'd be nice to use system gtest once the tests are fixed to work, but not critical.
# https://github.com/DLTcollab/sse2neon is bundled in third_party/sse2neon Provides: bundled(sse2neon)
third_party/uapi Seems to bundle three different versions of kernel drm headers. Not sure if Fedora kernel-headers can be used instead. Please check.
DirectX stuff can be ignored, I guess it's not used for build.
I'm not sure what aub_stream headers are or where they come from.
third_party/source_level_debugger contains one header under BSD license. Not sure where that came from, either.
Included spec file (scripts/packaging/opencl/rhel_8.4/SPECS/opencl.spec) puts the built files into two subpackages: intel-ocloc and intel-opencl. Perhaps it makes sense to do the same in Fedora.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #12 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- The uapi drm headers seem to be MIT licensed as well, but it looks like a variant that's not detected by licensecheck. Licensing seems to be fine, then.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #13 from František Zatloukal fzatlouk@redhat.com --- (In reply to Dominik 'Rathann' Mierzejewski from comment #11)
There's lots of bundled stuff in third_party/. In particular:
third_party/opencl_headers third_party/opengl_headers can be removed and system ones used instead. Add
Unfortunately, the bundled versions are prehistoric, I've tried, ended up with reporting it upstream: https://github.com/intel/compute-runtime/issues/496
I guess the way forward here is to mark those two as bundlet too for now?
third_party/uapi
Will take a look!
Perhaps it makes sense to do the same in Fedora.
Mhm, seems like a good idea. I think we can keep intel-compute-runtime as a metapackage which would then pull in both intel-ocloc and intel-opencl. This also makes sense in case we decide sometime in the future that we want to have the OpenCL driver in default package set for some Fedora images.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #14 from František Zatloukal fzatlouk@redhat.com --- SPEC: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
SRPM: https://download.copr.fedorainfracloud.org/results/frantisekz/intel-media-dr...
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #15 from František Zatloukal fzatlouk@redhat.com --- Hopefully, I didn't forget anything, it's currently not possible to use the upstream drm.h, I've added commented out code to change that if/when that becomes a case in the future (by copying all the headers into builddir, it seemed easier than patching up all the files, there're lots of them).
Package split into intel-ocloc and intel-opencl.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
Dominik 'Rathann' Mierzejewski dominik@greysector.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #16 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- It'd be nice to have more useful Summary: and description of the subpackages.
ocloc has this in help:
ocloc is a tool for managing Intel Compute GPU device binary format. It can be used for generation (as part of 'compile' command) as well as manipulation (decoding/modifying - as part of 'disasm'/'asm' commands) of such binary files. Intel Compute GPU device binary is a format used by Intel Compute GPU runtime (aka NEO). Intel Compute GPU runtime will return this binary format when queried using clGetProgramInfo(..., CL_PROGRAM_BINARIES, ...). It will also honor this format as input to clCreateProgramWithBinary function call. ocloc does not require Intel GPU device to be present in the system nor does it depend on Intel Compute GPU runtime driver to be installed. It does however rely on the same set of compilers (IGC, common_clang) as the runtime driver.
For the -opencl subpackage, maybe take a look at the unmaintained beignet package description, e.g. https://src.fedoraproject.org/rpms/beignet/blob/f29/f/beignet.spec .
I think this package is good to go and you can improve the above when importing.
Please open a bug to track the crashes. We do want the OpenCL driver functional as soon as possible.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #17 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- And once the crashes are fixed, please backport the package to F35.
https://bugzilla.redhat.com/show_bug.cgi?id=2036135 Bug 2036135 depends on bug 2036099, which changed state.
Bug 2036099 Summary: Review Request: intel-igc - Intel(R) Graphics Compiler for OpenCL(TM) https://bugzilla.redhat.com/show_bug.cgi?id=2036099
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #18 from František Zatloukal fzatlouk@redhat.com --- Thanks for the review!
I've updated descriptions, should be better, probably not perfect :)
Please open a bug to track the crashes. We do want the OpenCL driver functional as soon as possible.
Yeah, those would be https://github.com/intel/intel-graphics-compiler/issues/204 and https://github.com/intel/intel-graphics-compiler/issues/224 , or did you mean buzgilla tracker?
Also, I've added rm -v for sse2neon (and removed bundled provide for it) and ExclusiveArch: x86_64 i686 as intel-gmmlib is built only for those and intel-igc now too. The support for other arches seems non-existent/incomplete throughout the entire stack.
I've requested repo for the package: https://pagure.io/releng/fedora-scm-requests/issue/41679
And once the crashes are fixed, please backport the package to F35.
I wasn't planning to do that originally, but I'll take a look/do my best. I'll have to find/use some older combination of intel-igc and this package as intel-gmmlib in f35 is too old and new release breaks api/abi.
Also, I'd be glad if you'd want to co-maintain this (and/or any other part of the stack)?
Thanks again!
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #19 from Dominik 'Rathann' Mierzejewski dominik@greysector.net --- (In reply to František Zatloukal from comment #18)
Thanks for the review!
I've updated descriptions, should be better, probably not perfect :)
Please open a bug to track the crashes. We do want the OpenCL driver functional as soon as possible.
Yeah, those would be https://github.com/intel/intel-graphics-compiler/issues/204 and https://github.com/intel/intel-graphics-compiler/issues/224 , or did you mean buzgilla tracker?
I'd open a bugzilla tracker once the repo is created. When the package is out, people will find it, sooner or later, and will report crashes in bugzilla or at least look there first. Though hopefully the issue may get fixed before F36 is released.
Also, I've added rm -v for sse2neon (and removed bundled provide for it) and ExclusiveArch: x86_64 i686 as intel-gmmlib is built only for those and intel-igc now too. The support for other arches seems non-existent/incomplete throughout the entire stack.
Good point, thanks.
I've requested repo for the package: https://pagure.io/releng/fedora-scm-requests/issue/41679
And once the crashes are fixed, please backport the package to F35.
I wasn't planning to do that originally, but I'll take a look/do my best. I'll have to find/use some older combination of intel-igc and this package as intel-gmmlib in f35 is too old and new release breaks api/abi.
Alright. Let's keep this F36+ only, then.
Also, I'd be glad if you'd want to co-maintain this (and/or any other part of the stack)?
Sure. Please add me as co-maintainer to this and to intel-media-driver-free. I'll be an active user for the latter and in position to test the former.
Thanks again!
You're welcome. Thanks for your contribution!
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
--- Comment #20 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/intel-compute-runtime
https://bugzilla.redhat.com/show_bug.cgi?id=2036135
František Zatloukal fzatlouk@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version| |intel-compute-runtime-22.04 | |.22286-1.fc36 Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2022-02-02 23:31:25
--- Comment #21 from František Zatloukal fzatlouk@redhat.com --- Built in rawhide: intel-compute-runtime-22.04.22286-1.fc36
Sure. Please add me as co-maintainer to this and to intel-media-driver-free.
Added
I'd open a bugzilla tracker once the repo is created.
Good point, will do in the morning :)
package-review@lists.fedoraproject.org