From owner-svn-ports-all@freebsd.org Sat Apr 8 19:36:18 2017 Return-Path: Delivered-To: svn-ports-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B50B2D3541D; Sat, 8 Apr 2017 19:36:18 +0000 (UTC) (envelope-from jbeich@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2610:1c1:1:6074::16:84]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8A1AD2D9; Sat, 8 Apr 2017 19:36:18 +0000 (UTC) (envelope-from jbeich@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 1354) id BAB9E57E0; Sat, 8 Apr 2017 19:36:17 +0000 (UTC) From: Jan Beich To: Matthew Rezny Cc: svn-ports-head@freebsd.org, svn-ports-all@freebsd.org, ports-committers@freebsd.org Subject: Re: svn commit: r437953 - in head/lang/beignet: . files References: <201704071930.v37JUBGj017948@repo.freebsd.org> <24496938.jsC6qZ9reS@workstation.reztek> <4355650.qKugQOSgqh@workstation.reztek> Date: Sat, 08 Apr 2017 21:37:57 +0200 In-Reply-To: <4355650.qKugQOSgqh@workstation.reztek> (Matthew Rezny's message of "Sat, 08 Apr 2017 14:55:56 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: svn-ports-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the ports tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Apr 2017 19:36:18 -0000 Matthew Rezny writes: > On Friday 07 April 2017 23:34:03 Jan Beich wrote: > >> Matthew Rezny writes: >> > On Friday 07 April 2017 22:23:18 Jan Beich wrote: >> >> Matthew Rezny writes: >> >> > - Enable OpenCL 2.0 on AMD64 >> >> >> >> Isn't this premature before 1.3.2 or bug 217635 fix? >> > >> > There was no clear answer given in that PR as to whether or not version >> > 1.3.1 addressed the issue, but as I understood it the issue in that PR >> > was caused by a lack of OpenCL 2.0 so I do not see how enabling it would >> > be premature. >> DRM_IOCTL_I915_GEM_USERPTR is only available on drm-next but even there is >> half broken. Beignet *requires* it for OpenCL 2.0 but, looking more, the >> default is still 1.2. The patch just limits the ioctl to when an application >> explicitly requests 2.0. >> >> If you want a clear answer look no further than comment 0. OpenCL 2.0 >> support upstream *is* the source of the regression. The patch just have a >> different effect on 1.3.0 and 1.3.1 but still required for both on FreeBSD. >> > Given that comments 0 and 1 in 217635 are the reporting of the original issue, > I assume you are referring to comment 0 of 217771, which is not the correct > place to give an answer about 217635. However, that was just a summary of your > testing without any statement of impact on 217635. Clear as mud. I was referring to bug 217635. The update in bug 217771 was mostly irrelevant except for the option that allows *disabling* OpenCL 2.0. At the time bug 217635 comment 4 was written your version of the patch wasn't available in bug 217771 yet. My QA required testing OpenCL 2.0 disabled in order to make sure i386 still works but doing so in a jail on amd64 host is unreliable due to incomplete compat32 shims in DRM. If there was a clear relation I'd have adjusted bugzilla fields. > Not long after creating 217771, you posted comment 4 to 217635 that reads > "With bug 217771 in scope comment 0 is limited to OPENCL20=off while comment 1 > no longer happens." To me, that says both issues in 217635 are fixed if Beignet > is updated to 1.3.1 and OpenCL 2.0 support is enabled. After discussing with upstream developers that joined in the bug it became clear that my testing was flawed (making comment 4 invalid) - both cases crash without root. i386 also cannot use OpenCL 2.0 thus 1.3.1 couldn't completely fix the crash even with misunderstanding. > That only needed a simple yes or no answer, Whatever lands upstream can be used as the answer but it's not "simple": - OpenCL 1.2 calling code related to OpenCL 2.0 support (see the patch) - OpenCL 2.0 using ioctl N/A on FreeBSD and incomplete support on drm-next - QA required many permutations, so "yes or no" would just dumb it down - bug 217635 trail isn't long for maintainer to fully grasp > but more than 3 weeks have passed without any answer to it in either PR. Bug 217771 comment 11 had issues like: - expected re-doing QA but ability to disable OpenCL 2.0 in order to mimic i386 configuration is not exposed - turned disagreement about OPENCL option into an "experienced" strawman Prior to that I was unconvinced your version did anything more than: - change style (required explicitly asking why) - introduce bugs (fixed) - force OpenCL 2.0 build on amd64 > Fortunately, someone else has followed up with a more useful response; > "Blindly following the PHB (or portlint) is still merely cargo-culting. > USE_LDCONFIG is bogus for *.so's that are not being normally linked by > consumers, but dlopen(3)ed during runtime instead." > Sentence 1 I'm ignoring as the product of this thread. > Sentence 2 is what would have helped to see in the PHB or earlier in this > thread. danfe@ was incorrect as well. dlopen(3) is implemented in rtld(1), so it does use ldconfig hints but they're often unnecessary. When to add USE_LDCONFIG != yes requires a "double-check". If intel-beignet.icd didn't use absolute paths the following would happen: $ cat a.c #include #include int main() { void * p = dlopen("libcl.so", RTLD_LAZY); printf("%p\n", p); return 0; } $ cc a.c $ ./a.out 0x0 # ldconfig -m /usr/local/lib/beignet $ ./a.out 0x800625800 Nothing to output ! > Given that your interaction thus far has consisted primarily of useless > criticism, where half might be constructive if some reasoning was provided but > half is just personal opinion/style, it feels like your aim is to cause grief > rather than improve or educate, so I may just begin ignoring you in favor of > those who can provide useful feedback. Apoloiges, I usually don't explain homework questions (e.g. how regexps work in sed(1) commands). And mentor approval is not a substitute for review that may happen pre- or post- commit. Style (and consistency with other ports) are important. If you pick an unusual just be prepared to elaborate, others may want to adopt it. However, the review in question doesn't focus on style too much.