Date: Sat, 08 Apr 2017 21:37:57 +0200 From: Jan Beich <jbeich@FreeBSD.org> To: Matthew Rezny <rezny@freebsd.org> 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 Message-ID: <tw5y-pu5m-wny@FreeBSD.org> In-Reply-To: <4355650.qKugQOSgqh@workstation.reztek> (Matthew Rezny's message of "Sat, 08 Apr 2017 14:55:56 %2B0200") References: <201704071930.v37JUBGj017948@repo.freebsd.org> <24496938.jsC6qZ9reS@workstation.reztek> <fuhj-kilw-wny@FreeBSD.org> <4355650.qKugQOSgqh@workstation.reztek>
next in thread | previous in thread | raw e-mail | index | archive | help
Matthew Rezny <rezny@freebsd.org> writes:
> On Friday 07 April 2017 23:34:03 Jan Beich wrote:
>
>> Matthew Rezny <rezny@freebsd.org> writes:
>> > On Friday 07 April 2017 22:23:18 Jan Beich wrote:
>> >> Matthew Rezny <rezny@FreeBSD.org> 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 <dlfcn.h>
#include <stdio.h>
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?tw5y-pu5m-wny>