Date: Sun, 09 Apr 2017 01:11:38 +0200 From: Matthew Rezny <rezny@freebsd.org> To: Jan Beich <jbeich@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: <2717791.767zmPuqfH@workstation.reztek> In-Reply-To: <tw5y-pu5m-wny@FreeBSD.org> References: <201704071930.v37JUBGj017948@repo.freebsd.org> <4355650.qKugQOSgqh@workstation.reztek> <tw5y-pu5m-wny@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 08 April 2017 21:37:57 Jan Beich wrote:
> 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 you were referring to 217635 when you said that comment 0 answered my
question, then I guess we're basically done, because that response is outright
nonsense given the question was asked in comment 5 about something stated in
comment 4. You're clearly not one to be reasoned with.
Of course my WIP was yet not available in 217771 considering that I did not
know that PR existed or that anyone had any interest in working on beignet
until you mention that PR in comment 4 of 217635. Once I was aware, I added
the FP64 and TEST options stuff to what I had, only to get asked accusingly why
I had added a typo as if it was some intentional sabotage, and find myself
defending use of posix_memalign in a patch made sometime early that day, as
part of a general "why didn't you just do what I did?" line of comments. In
other words, totally unhelpful and nonconstructive. If you want to help, you
point out the typo exists move on to suggesting improvements with reasons for
why that way is better/correct/whatever.
> If there was a clear relation I'd have adjusted bugzilla fields.
>
Comment 4 stated a connection that was never confirmed or denied. Activity or
lack thereof in bugzilla fields is not a substitute for an answer.
> > 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.
>
At the point that you knew that, you should have replied to comment 5 saying
"No, the update in 217771 does not fix these problems." and then there would be
no confusion.
> > 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
>
The question only asked for a yes or no answer because that's all that was
needed. If you did not yet know, you say so, and when you figure out what you
stated above, you answer so. Leaving the question hanging helps nobody. Now I
finally know that 217635 should stay open at least until 1.3.2 lands.
> > 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
You did not state you needed the option for your testing. I was trying to ship
it as upstream intended, but could have easily added options for testing. The
statement earlier in this message about testing i386 with incomplete compat32
shims is the first hint at why the option would be useful for testing. I asked
you to runtime test what would be committed because you had shown interest and
had the hardware to do so, which I've already stated I do not.
> - turned disagreement about OPENCL option into an "experienced" strawman
The last of the OPENCL20 option discussion was in comment 10. What was said
about experience in comment 11 is separate from that option entirely. What I
said there is the same I'm saying here, which is that as the senior committer
you have more experience with the ports framework so I could surely learn some
things about how to best use it if you care to provide constructive criticism,
but the criticism thus far has mostly been counterproductive.
>
> Prior to that I was unconvinced your version did anything more than:
> - change style (required explicitly asking why)
Any change of style while while integrating with my WIP is incidental.
> - introduce bugs (fixed)
I assume by bugs you mean a couple typos.
> - force OpenCL 2.0 build on amd64
Actually, it's does not have a force disable; enabled is the default.
>
> > 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 !
>
Finally, a useful nugget. That check could be integrated into our port tools.
> > 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.
>
The sed questions were not from this thread but another, which I had not drawn
into this one as that is another quagmire. There, someone's request for a
simple, one-line change spawned a huge diff with unexplained and unexpected
changes. The sed changes looked suspicious until explained, but as you saw, an
incomplete explanation can be worse than none at all (using a wrong
replacement for a time instead of just leaving what was working but sub-
optimal to you). That was the cream; the rest of what is going on in that PR
with the insistence on trying to support obsolete and unsupported LLVM
versions is already on /ignore.
> 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.
The whitespace between options is not established style that I'm aware of,
merely something you personally did not like as you stated yourself. The use
of a conditional or a regex is implementation style and you labeled it so.
When you are bringing those style quibbles, then even a somewhat technical
issue such as how to replace memalign becomes part of an overall style debate
that essentially amounts to "I'd have done this differently."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2717791.767zmPuqfH>