Date: Thu, 30 Mar 2017 11:39:11 -0400 From: Johannes M Dieterich <jmd@freebsd.org> To: Matthew Rezny <rezny@freebsd.org> Cc: Jan Beich <jbeich@freebsd.org>, svn-ports-head@freebsd.org, svn-ports-all@freebsd.org, ports-committers@freebsd.org, owner-ports-committers@freebsd.org, swills@freebsd.org, kwm@freebsd.org Subject: Re: svn commit: r437215 - in head/graphics: gbm libEGL libGL libglapi Message-ID: <425ed90f9f572884f734898c9997e499@freebsd.org> In-Reply-To: <19694803.doR80QHWTi@workstation.reztek> References: <201703291657.v2TGvrpM076369@repo.freebsd.org> <1824022.AbFPd1OJUh@workstation.reztek> <8tnn-5tcs-wny@FreeBSD.org> <19694803.doR80QHWTi@workstation.reztek>
next in thread | previous in thread | raw e-mail | index | archive | help
Dear Matthew,
CC: Jan, Koop, Steve
I will unify your two mails below.
On 2017-03-30 01:26, Matthew Rezny wrote:
> On Thursday 30 March 2017 05:36:03 Jan Beich wrote:
>> Matthew Rezny <rezny@freebsd.org> writes:
>> > On Wednesday 29 March 2017 19:51:55 Jan Beich wrote:
>> >> Matthew Rezny <rezny@FreeBSD.org> writes:
>> >> > - @${REINPLACE_CMD} -e 's|x86_64|amd64|' \
>> >> > + @${REINPLACE_CMD} -e 's|x86_64|amd64|' -e 's|\\S\*//|[:space:]*
> //|'
>> >> > \
>> >>
>> >> [:space:] is invalid character class thus treated as a list of
>> >> characters.
>> >> \S corresponds to [^[:space:]], while \s to [[:space:]].
>> >>
>> >> $ man pcrepattern | col -b | fgrep -m1 \\S
>> >>
>> >> \S any character that is not a white space character
>> >>
>> >> This may break build given -march, etc. are no longer stripped.
>> >
>> > I wish that information had been presented when I said "I guess it should
>> > have been [:space:] instead of [:graph:] in the replacement." after you
>> > stated [:graph:] was plain incorrect, although it is what had been
>> > previously suggested to me and did seem to be working.
>>
>> I didn't focus on pointing out every mistake with the existing hack
>> because it was soon going away. Now that devel/libclc depends on
>> llvm40
>> the original motivation to hold out on 17.* (bug 217016) before 2017Q2
>> has been weakened.
>>
> Pointing out something is wrong without giving the correct solution is
> not
> helpful. The upstream change in the 17.1-dev branch was not directly
> applicable to 13.0.x, so the the 'hack' would remain unless I was going
> to
> change to change the configure.ac and patch-configure myself, which is
> certainly
> more work that to edit the post-patch and it would have been the same
> changes
> in either place lacking clearer input.
>
> Nobody said anything to me about committing an update to libclc at this
> moment. I do not believe that has tested in combination with the rest
> of the
> graphics stack at the current versions in ports, the mix of LLVM
> versions
> almost certainly will be a problem, and it's a day before the quarterly
> branch. WTF? I've been holding off Mesa 17.x and LLVM4.0 for after that
> branch
> while attempting to get Xorg 1.19 ready in time for that. The latter
> won't
> happen because it took over a week to even start an exp-run on the
> bsd.xorg.mk
> changes. I just explained the reason for holding of an update of libclc
> yesterday in a PR that proposed a more recent snapshot for the
> transition to
> dependence on llvm40. I had not even gotten everything onto llvm39 yet;
> pocl
> 0.14 should be compatible past 3.8, but it is not yet released and I
> had
> difficulties with rc1 as they switched to cmake so the build system
> needs to be
> redone. While I'm sure Mesa 17.0.x will be ok with llvm40 after
> appropriate
> patching (I had to add several patches for clover in Mesa 13), I cannot
> say
> the same for all the OpenCL ports, i.e. beignet which was only recently
> made
> compatible past llvm37 with the 1.3.0 update that allowed it to use
> llvm39.
> So I expect r438268 to be reverted, or someone else to handle all the
> fallout
> this causes on the quarterly branch during Q2. I realize multiple
> people want
> to help, but we need some coordination or else we are just wasting each
> other's time. Sorry to be brunt, but that was an ill timed commit.
This patch was committed by me, after https://reviews.freebsd.org/D9394
got accepted by swills, reviewed by kwm (with x11 hat on) in the
beginning of February. No objections were raised since then. I did not
commit it earlier to wait for stable llvm40 to hit the tree. I again
checked with swills yesterday before committing.
Hence, I will back this out if either kwm or swills tell me to.
Steve/Koop: opinions? I will also personally say that I believe we
should only support one version of llvm (the latest stable) across Mesa
ports if possible, which is another reason why my patch to
graphics/libGL bumps to LLVM4.0. I do not see any added value despite
more maintenance work in supporting older versions of LLVM for Mesa. The
PR you referenced was also explicitly looking for support of a newer
LLVM, not an older one.
Concerning your changes to graphics/libGL: I appreciate you wanting to
help and improve things. The Mesa ports are in dire need of that. I
however would also appreciate if you would acknowledge the work other
people (Mark Johnston, Johannes Lundberg, Kip Macy, Koop Mast, Pete
Wright, and myself) have put into this over the last 6+ months. For some
selected time line:
* Johannes Lundberg enabled wayland and dri3 on the public
FreeBSDDesktop github in last autumn
* Kip and I changed to using libudev-devd there in December
* I updated to Mesa 17.0 rc in January there
* I bumped to llvm40 in the end of January there as that was needed to
support Polaris and Carrizo (better)
* before that part goes amiss: compilation with the same llvm as is
linked against is also needed for amdgpu, we had crashes otherwise
* throughout this time kwm (with x11 hat on) was involved in these
updates
* I uploaded the first version of the Mesa 17.0 patch to reviews on Feb
7 (which you were apparently aware of)
* I've since updated the patch multiple times for minor releases and
address comments by mat and kwm (with x11 hat on)
* again, no comments/objections were raised there by anybody else
The first Mesa 17 patch in PR217016 dates from March 6, a month after my
first Mesa 17 patch on reviews. Apparently you were aware of (at least)
the last two items on the list above. So yes, coordination is important,
which is also why we have an x11 hat (Koop). As you can see from the
above, Koop was involved in all of this from the onset.
> I had not noticed that your review was updated recently as it sat idle
> for
> quite a while after the update to 13.0.x landed in ports. I'm curious
> about
> the comment regarding a need for libudev. As far as I know, Mesa
> dropped the
> dependence on udev in v13 and relies upon libdrm for all the direct
> hardware
> interaction. Initially, I had some hope to use libudev-devd as a single
> translation layer, but after reading through the libdrm code it became
> obvious
> that would not be the case as the conditional udev code in libdrm is
> withing
> Linux-specific code paths so no help to us. Thus, I implemented support
> for our
> platform directly in libdrm in order to drop dependence on libdevq,
> which was
> needed for no other purpose. Could you explain why Mesa 17 would need
> libudev-
> devd for AMDGPU?
We used libudev for loader related functionality in Mesa. I am happy if
we do not need it anymore, at the time Kip and I just wanted to get rid
of libdevq -- a FreeBSD-ism. libdrm did not support the linuxkpi amdgpu
and Mesa would not load radeonsi properly.
Concerning the libdrm udev: as you may have seen from the FreeBSDDesktop
github, I tried to do that before Christmas w/o success.
>> > To be sure there is no misunderstanding now, would you consider this
>> > correct? @${REINPLACE_CMD} -e 's|x86_64|amd64|' -e
>> > 's|\\S\*//|[^[:space:]]* //|' \
>> Not quite, adding extra space after [] is unnecessary.
>>
>> -e 's|\\S\*|[^[:space:]]*|' \
>
> It is interesting how many variations appear to produce the same
> result. You
> mention the space is not needed, so 's|\\S\*//|[^[:space:]]*//|' but
> then the
> example has the trailing // removed. Any reason why?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?425ed90f9f572884f734898c9997e499>