For upstream changes see URL bugzilla field, for downstream changes see commit message (in the patch). Build testing: 10.3 i386 - http://sprunge.us/MSbN 10.3 amd64 - http://sprunge.us/FBaG 11.0 i386 - http://sprunge.us/VLTY 11.0 amd64 - http://sprunge.us/BMZJ /head@r314638 amd64 - http://sprunge.us/ADLP 10.3 i386 + TEST=on - http://sprunge.us/XdKA 10.3 amd64 + FP64=on + OPENCL20=off - http://sprunge.us/UhYb 11.0 amd64 + TEST=on - http://sprunge.us/Seji Runtime testing (drm-next): $ clinfo | fgrep -i version Platform Version OpenCL 2.0 beignet 1.3 Device Version OpenCL 2.0 beignet 1.3 Driver Version 1.3 Device OpenCL C Version OpenCL C 2.0 beignet 1.3 SPIR versions 1.2 ICD loader Version 2.2.11 $ clpeak Platform: Intel Gen OCL Driver Device: Intel(R) HD Graphics Skylake Desktop GT2 Driver version : 1.3 (FreeBSD) Compute units : 24 Clock frequency : 1000 MHz Global memory bandwidth (GBPS) float : 29.06 float2 : 28.91 float4 : 31.40 float8 : 32.07 float16 : 30.10 Single-precision compute (GFLOPS) float : 422.04 float2 : 438.56 float4 : 438.05 float8 : 436.74 float16 : 434.20 No double precision support! Skipped Transfer bandwidth (GBPS) enqueueWriteBuffer : 8.68 enqueueReadBuffer : 11.32 enqueueMapBuffer(for read) : 40215.05 memcpy from mapped ptr : 11.48 enqueueUnmap(after write) : 43296.04 memcpy to mapped ptr : 10.58 Kernel launch latency : 29.47 us Nothing to output !
Created attachment 180792 [details] update, v1
*** Bug 213551 has been marked as a duplicate of this bug. ***
Is there any reason that OpenCL 2.0 should not always be enabled? It should not be a problem to do so for 1.3.1. Enable OpenCL 2.0 only where supported "This allows a single beignet binary to both offer 2.0 where available, and still work on older hardware."
OpenCL 2.0 is not supported on i386. Do you want me to hide it via OPTIONS_SLAVE?
Created attachment 180799 [details] update lang/beignet to 1.3.1
(In reply to Jan Beich (mail not working) from comment #4) I mean to not use an option and just have PLIST_SUB depend on ARCH as in the patch I just attached. I've verified the build in Poudriere 10/11 amd64/i386. Runtime testing would be appreciated.
Comment on attachment 180799 [details] update lang/beignet to 1.3.1 OpenCL 2.0 requires LLVM >= 3.9 and libdrm >= 2.4.67 (i.e. ports r431463 and maybe[1] drm-next). I forgot to adjust at least BUILD_DEPENDS in my version. [1] Whether drm_intel_bo_set_softpin_offset() works on FreeBSD 10.3 or 11.0 or causes a crash similar to bug 217635 is unknown. https://cgit.freedesktop.org/mesa/drm/commit/?id=8b4d57e7 > +OPTIONS_DEFINE= FP64 TEST > +FP64_DESC= Double precision (experimental) I don't like this style of grouping as it doesn't scale. If you have many options having to jump between descriptions and definitions when working on a port with many can get tiring, see multimedia/ffmpeg. > +.if ${ARCH} == amd64 > +PLIST_SUB+= OCL20="" > +.else # ${ARCH} == i386 > +PLIST_SUB+= OCL20="@comment " > +.endif Conditionals break (mostly) declarative style of makefiles. Try the following instead PLIST_SUB+= OCL20="${ARCH:Namd64:C/.+/@comment /}" > + -@(cd ${TEST_WRKSRC}.utests; . ./setenv.sh; \ Why do you need to introduce typos? $ make test WITH=TEST ===> Testing for beignet-1.3.1 cd: /usr/ports/lang/beignet/work/Beignet-1.3.1-Source.utests: No such file or directory .: cannot open ./setenv.sh: No such file or directory *** Error code 2 (ignored) $ make test WITH=TEST ===> Testing for beignet-1.3.1 /bin/sh: ./flataddress_space: not found > +- src_data = (uint32_t*)memalign(base_address_alignment, buffer_sz); > +- if(!src_data) { > ++ if(posix_memalign((void**)&src_data, base_address_alignment, buffer_sz)) { posix_memalign has greater minimal alignment than memalign or aligned_alloc in jemalloc and glibc. And neither memalign nor posix_memalign are available on Windows. Or do you have something against using C11/C++17 features without passing corresponding -std= ? https://github.com/jemalloc/jemalloc/blob/4.5.0/src/jemalloc.c#L2038 https://github.com/jemalloc/jemalloc/blob/4.5.0/src/jemalloc.c#L1787 https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/malloc.c;hb=glibc-2.25#l3071 https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/malloc.c;hb=glibc-2.25#l3136 > @@ -17,7 +17,7 @@ > > # XLib > Find_Package(X11) > -@@ -219,7 +218,7 @@ IF(OCLIcd_FOUND) > +@@ -217,7 +216,7 @@ IF(OCLIcd_FOUND) > "intel-beignet.icd.in" > "${ICD_FILE_NAME}" > ) Try to avoid excessive noise (e.g. line offsets, timestamps) in patches. > @@ -29,6 +29,8 @@ > lib/beignet/include/ocl_vload_20.h > lib/beignet/include/ocl_work_group.h > lib/beignet/include/ocl_workitem.h > +%%OCL20%%lib/beignet/beignet_20.bc > +%%OCL20%%lib/beignet/beignet_20.pch > lib/beignet/libcl.so > lib/beignet/libgbe.so > lib/beignet/libgbeinterp.so Keep sorting consistent with a version where %% macros are expanded.
(In reply to Jan Beich (mail not working) from comment #7) >OpenCL 2.0 requires LLVM >= 3.9 and libdrm >= 2.4.67 (i.e. ports r431463 and pkg info libdrm >maybe[1] drm-next). I forgot to adjust at least BUILD_DEPENDS in my version. The LLVM requirement was bumped to 3.9 during the prior update to 1.3.0 and we have had an up to date libdrm long enough that should be no concern either. >[1] Whether drm_intel_bo_set_softpin_offset() works on FreeBSD 10.3 or 11.0 or causes a crash similar to bug 217635 is unknown. > https://cgit.freedesktop.org/mesa/drm/commit/?id=8b4d57e7 That is something that will need to be tested. Given that the only Intel box I have has a chipset with "Gen3" GPU, I cannot test any OpenCL on Intel. >> +OPTIONS_DEFINE= FP64 TEST >> +FP64_DESC= Double precision (experimental) >I don't like this style of grouping as it doesn't scale. If you have many >options having to jump between descriptions and definitions when working on a >port with many can get tiring, see multimedia/ffmpeg. And I don't like excess whitespace to scroll through, so it's merely a difference of opinion. This port is not ffmpeg and it does have an unwieldy number of options, nor is it expected to. Should there be an explosion of options, whitespace can be added as needed. >> +.if ${ARCH} == amd64 >> +PLIST_SUB+= OCL20="" >> +.else # ${ARCH} == i386 >> +PLIST_SUB+= OCL20="@comment " >> +.endif >Conditionals break (mostly) declarative style of makefiles. Try the following instead > PLIST_SUB+= OCL20="${ARCH:Namd64:C/.+/@comment /}" The conditional used it immediately understandable at a glance, the other way is not. Clarity has value greater than conciseness. >> + -@(cd ${TEST_WRKSRC}.utests; . ./setenv.sh; \ >Why do you need to introduce typos? Because I'm not a robot, but an imperfect human? If you mean to ask why did I retype it, well, I'd already bumped the port version and made some changes (i.e. adding the OCL20 stuff), so the patch wasn't going to apply without abandoning the progress made, copy and paste from the browser doesn't preserve the tabs, and that particular section had excess whitespace that would need to be removed, so retyping was the fastest solution, until I have to explain how a typo could possibly come to be... I did not try to run the tests as I do not expect any to succeed without the requisite hardware. >> +- src_data = (uint32_t*)memalign(base_address_alignment, buffer_sz); >> +- if(!src_data) { >> ++ if(posix_memalign((void**)&src_data, base_address_alignment, buffer_sz)) { >posix_memalign has greater minimal alignment than memalign or aligned_alloc in >jemalloc and glibc. And neither memalign nor posix_memalign are available on >Windows. Or do you have something against using C11/C++17 features without >passing corresponding -std= ? The intent was the make a patch that could be upstreamed more easily. posix_memalign is part of platforms (that doesn't include Windows). If aligned_alloc is available on all the platforms on which posix_memalign exists, then that is news to me. I am aware it is part of C11, but the support for C++11 (and therefore I assume C11 as well) in GCC (still the favorite on Linux) has been lacking every time I have tried to use it, and the last time I had to use a Microsoft compiler it did not support C99 and left a portion of C++0x stranded in the stdext namespace instead of std, so those are out the question. If the C11 support in GCC is strong enough, then aligned_alloc would be fine, though as you mention it would need the correct -std passed since GCC defaults to gnu++98. I do not see any more strict alignment requirements for posix_memalign; they both require the alignment be a power of 2. "As an example of the "supported by the implementation" requirement, POSIX function posix_memalign accepts any alignment that is a power of two and a multiple of sizeof(void *), and POSIX-based implementations of aligned_alloc inherit this requirements." >Try to avoid excessive noise (e.g. line offsets, timestamps) in patches. I had previously been advised to use makepatch when adding and changing patches. Is there a more strict set of criteria for when it is appropriate than I am aware of? > @@ -29,6 +29,8 @@ > lib/beignet/include/ocl_vload_20.h > lib/beignet/include/ocl_work_group.h > lib/beignet/include/ocl_workitem.h > +%%OCL20%%lib/beignet/beignet_20.bc > +%%OCL20%%lib/beignet/beignet_20.pch > lib/beignet/libcl.so > lib/beignet/libgbe.so > lib/beignet/libgbeinterp.so >Keep sorting consistent with a version where %% macros are expanded. What is inconsistent about the sorting? lib/beignet/b* < lib/beignet/l*
(In reply to Matthew Rezny from comment #8) > The LLVM requirement was bumped to 3.9 during the prior update to > 1.3.0 and we have had an up to date libdrm long enough that should > be no concern either. libdrm was updated just ~2 months ago. Once 2017Q2 is branched users may find OpenCL 2.0 is auto-disabled during build. Partial upgrades were never supported, so this can be ignored. > That is something that will need to be tested. Given that the only > Intel box I have has a chipset with "Gen3" GPU, I cannot test any > OpenCL on Intel. Removing OPENCL20 option makes 1.2 vs. 2.0 troubleshooting harder. Neither beignet port exposes debugging support (vendor optimization gets in the way) nor ports framework provides packages with debugging symbols to help distinguish crash fingerprints from already reported. > need the correct -std passed since GCC defaults to gnu++98. Not necessarily. GCC switched from C89 to C11 since 5.0 and from C++98 to C++14 since 6.0. Clang switched from C99 to C11 in 3.6 but as of 4.0 is still stuck with C++98. -std=c++0x (and -std=c++11) is already used in CMakeLists.txt. Downstream that provides beignet package also has new enough GCC version, newer than lang/gcc on FreeBSD. https://repology.org/metapackage/beignet/versions https://repology.org/metapackage/gcc/versions > multiple of sizeof(void *) memalign and aligned_alloc don't have such a restriction. Upstream already uses posix_memalign in other places, so I'm giving up. When pushing files/ upstream some __FreeBSD__ checks would need to be dropped to avoid churn for other BSDs. > copy and paste from the browser doesn't preserve the tabs, and that > particular section had excess whitespace that would need to be > removed, so retyping was the fastest solution Firefox and Chromium on X11 platforms preserve tabs just fine both via clipboard or primary selection. > I had previously been advised to use makepatch when adding and changing > patches. Is there a more strict set of criteria for when it is appropriate than > I am aware of? "make makepatch" (like "make makeplist") output isn't supposed to be used as is. It can accidentally merge separate patch files, incorporate sed(1) usage, etc. Porter's Handbook describes noise mainly in relation to "non-functional whitespace changes", so I maybe wrong. https://www.freebsd.org/doc/en/books/porters-handbook/slow-patch.html > What is inconsistent about the sorting? > lib/beignet/b* < lib/beignet/l* See where lib/beignet/beignet.pch is already placed, move *_20.* files there as well. lib/beignet/b* < lib/beignet/i* < lib/beignet/l*
(In reply to Jan Beich (mail not working) from comment #9) >libdrm was updated just ~2 months ago. Once 2017Q2 is branched users may find OpenCL 2.0 is auto-disabled during build. Partial upgrades were never supported, so this can be ignored. Right, people on a quarterly branch with old libdrm won't have the new beignet with OCL2, people on head will get both new libdrm and new beignet, and anyone who cares about something like OpenCL is not on quarterly. >Removing OPENCL20 option makes 1.2 vs. 2.0 troubleshooting harder. Neither beignet port exposes debugging support (vendor optimization gets in the way) nor ports framework provides packages with debugging symbols to help distinguish crash fingerprints from already reported. Wouldn't two variants, one with 2.0 support and one without, make that exact issue worse than having a single build would? I must be missing something, it was my impression that the single build was desirable. >Not necessarily. GCC switched from C89 to C11 since 5.0 and from C++98 to C++14 since 6.0. Clang switched from C99 to C11 in 3.6 but as of 4.0 is still stuck with C++98. Ah, good to know, I had not been keeping up on the changes in GCC, and still bumping into the old default since the default ports gcc is still 4.9 >memalign and aligned_alloc don't have such a restriction. Upstream already uses posix_memalign in other places, so I'm giving up. When pushing files/ upstream some __FreeBSD__ checks would need to be dropped to avoid churn for other BSDs. I did not mean to cause contention. Using posix_memalign looked to be the more portable solution is all, and if that is not the case I do welcome to be corrected as that entails an expansion of knowledge. It sounds like you had a reason I didn't fully grasp. >Firefox and Chromium on X11 platforms preserve tabs just fine both via clipboard or primary selection. This falls into the category of things that didn't work reliably and thus are no longer tried. Perhaps it depends on the site from which it's copied, some sites might be converting tabs to spaces before it gets to the browser. >> I had previously been advised to use makepatch when adding and changing >> patches. Is there a more strict set of criteria for when it is appropriate than >> I am aware of? >"make makepatch" (like "make makeplist") output isn't supposed to be used as is. It can accidentally merge separate patch files, incorporate sed(1) usage, etc. Porter's Handbook describes noise mainly in relation to "non-functional whitespace changes", so I maybe wrong. I am aware the output needs to be checked and quite used to doing the dance with makepatch wherein I copy back all those that were touched in post-patch. Of course I'm not perfect, I have accidentally committed sed cruft after regenerating a set of patches so many times I blinded myself to that part. I have briefly mulled over the idea of another target for this purpose, e.g. make cleanpatch. I assume the reason to regularly regenerate patches is to ease fixing hunks that don't automatically apply after a port update. If the line numbers have been drifting for a long time and the context is insufficient, it can be difficult to be sure where to apply that hunk. >> What is inconsistent about the sorting? >> lib/beignet/b* < lib/beignet/l* >See where lib/beignet/beignet.pch is already placed, move *_20.* files there as well. >lib/beignet/b* < lib/beignet/i* < lib/beignet/l* Doh! I should have noticed that from your diff. I must have been thinking of directory first sorting or mentally rewriting lib/beignet/include/ to include/beignet/ (as it probably should be) when I inserted those.
Created attachment 180820 [details] update lang/beignet to 1.3.1 The patch has been revised to correct the issues noted thus far. It is my hope we can get this tested and committed without strife. I sense that my nature of wanting to know the why of everything that is done is wearing on you. The update to 1.3.0 was a blind update motivated by a desire to drop dependencies on llvm37 since there was an observed conflict when llvm37 is installed alongside llvm39. I normally prefer to test first, but could not do so myself and did not know of anyone using this port. When a PR was opened against the port, I thought "great, we have a user aka tester" but I didn't expect an update PR to soon follow after a period in which it appeared nobody cared for this port. Perhaps instead of trying to meld our efforts, I should have just taken notice of your apparent interest and said "looks good enough, go for it." I've spent a lot of time getting to know the graphics stack, but you are the more experienced committer and my explorations have not included the Intel specific parts such as this port.
A commit references this bug: Author: rezny Date: Fri Apr 7 19:30:12 UTC 2017 New revision: 437953 URL: https://svnweb.freebsd.org/changeset/ports/437953 Log: Update to 1.3.1 - Enable OpenCL 2.0 on AMD64 - Add options FP64 (experimental) and TEST [1] - Follow MESA_LLVM_VER if set, currently only 39 works for this port PR: 217771 [1] Submitted by: jbeich [1] Approved by: swills (mentor) Differential Revision: https://reviews.freebsd.org/D10251 Changes: head/lang/beignet/Makefile head/lang/beignet/distinfo head/lang/beignet/files/patch-utests_image__from__buffer.cpp head/lang/beignet/pkg-plist
Do you plan to MFH this? If so send request to ports-secteam@. This has very low risk since it affects only one port.
(In reply to Jan Beich from comment #13) MFH is for security updates and bug fixes. The former does not apply. Does the latter? The email you sent just minutes prior suggested that the issue from PR 217635 is still present and that enabling OpenCL 2.0 was premature. That email and this last question seem to be in direct opposition. So which is it, did the 1.3.1 update resolve bugs and thus should be MFHed because it has low-risk, or does the 1.3.1 update still contain bugs and present a risk with a possibly premature feature?
(In reply to Jan Beich from comment #0) > MFH is for security updates and bug fixes. The former does not apply. Does the > latter? IIRC, the users on quarterly branches want less moving parts that result in accidental bustage in cosumer ports, due to old configs, etc. Regular updates are fine as long as maintainer or committer understands the risks. It's a tradeoff of maintaining an old version + new version or just a new version which sometimes drives even major backports like with lang/gcc or devel/boost-*. ports-secteam only cares about security fixes while the rest lies on the interested parties. Many bug fixes are also covered under blanket unless tied to the update. > The email you sent just minutes prior suggested that the issue from PR 217635 > is still present and that enabling OpenCL 2.0 was premature. That email and > this last question seem to be in direct opposition. Bug 217635 affects 2017Q2 even without the update here. The regression was introduced in 1.3.0 but even landing upstream didn't help to propagate the fix to the port. > So which is it, did the 1.3.1 update resolve bugs and thus should be MFHed > because it has low-risk, or does the 1.3.1 update still contain bugs and > present a risk with a possibly premature feature? 1.3.1 has other bug fixes, including for OpenCL 2.0, but ultimately still broken on FreeBSD. It is low-risk compared to 1.3.0 but not 1.2.0
A commit references this bug: Author: jbeich Date: Fri Oct 6 22:04:58 UTC 2017 New revision: 451412 URL: https://svnweb.freebsd.org/changeset/ports/451412 Log: lang/beignet: make OpenCL 2.0 optional on amd64 This is mainly to aid debugging OpenCL 2.0 issues and help testing OpenCL 1.2 without relying on kernel translating 32bit DRM ioctls. PR: 217771 Approved by: maintainer timeout (2 months) MFH: 2017Q2 (requires r437953), 2017Q3 Differential Revision: https://reviews.freebsd.org/D11377 Changes: head/lang/beignet/Makefile