Page MenuHomeFreeBSD

x11/nvidia-driver: Update to 367.35
ClosedPublic

Authored by cem on Aug 18 2016, 6:42 PM.

Details

Summary
  • Add needed x11 and xext dependencies
  • Remove ancient legacy driver support (driver version <200)

The ancient version of Nvidia driver in ports is holding back more recent Xorg
work, which in turn is holding back drm-next graphics support.

A patch to update the port has been submitted and tested. It just needs to be
committed.

Maintainer has not responded for 13 months as of yesterday, so I consider this
a "maintainer time out" situation.

Test Plan

The related PR is https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=201340 . See also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209951 .

poudriere bulk -tC:
                      304        340      367.27
12amd64:  success   success   success
12i386:      success   success   success
101amd64:success   success   success
101i386:    success   success   success
93amd64:  spdylay requires OpenSSL 1.0.1+
93i386:      spdylay requires OpenSSL 1.0.1+

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cem edited the test plan for this revision. (Show Details)
cem added reviewers: mat, bapt, bdrewery, koobs.

This works for me on an GTX 560 Ti as well as before. I played an OpenGL-using game for about an hour to test.

koobs requested changes to this revision.EditedAug 19 2016, 1:58 PM
koobs edited edge metadata.

@cem While I understand this change has been tested, I note no test details in the TEST PLAN section detailing what/how was tested, and in order to actually provide substantive (not necessarily long) review rather than a cursory 'yeh it looks ok to me' Can you please

  • Add a reference to the Bugzilla issue mentioned
  • Update the review description to include the itemized changes with rationale. A full background is not strictly necessary, but treat it like you would a good commit log message.

This will help provide the context for the change as a whole and allow reviewers to review intent against execution as well

Edit: This is just the standard I set for myself when providing changes for other peoples ports, and what makes it simple/quick for me to review and put my (Reviewed/Approved by:) name to.

This revision now requires changes to proceed.Aug 19 2016, 1:58 PM
mat edited edge metadata.

Looks good to me, if it builds and runs, go ahead.

I also note the maintainer is active as per rP417910 and rP417812 and should be on the review.

As a side note, maintainer timeout is 14 days. When you reach 3 months, it's absent without leave, and the port's maintainership can be resetted.

In D7569#157332, @mat wrote:

As a side note, maintainer timeout is 14 days. When you reach 3 months, it's absent without leave, and the port's maintainership can be resetted.

There is a material difference between doesn't respond on an bugzilla issue for 3 months and doesn't respond at all for 3 months. The documentation is ambiguous on this point [1]. If the former also applies, then a large proportion of our ports would have no maintainers based on that fact. Go and look in Bugzilla for ports issues that are assigned to *@freebsd accounts with last mod times greater than 3 months.

You yourself would not have maintainership of nagios-plugins and tmux based on your argument and:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=197193
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204429

Would you consider it rude if someone reset maintainership on those ports, or even suggested it was OK to do so, particularly if they knew you were active?

Additionally, the fact that there is evidence that the maintainer is active, nullifies the option to use 'in absentia > 3 months' argument

Further, independent to whether or not you dispute the points above, the maintainer should be on the review.

Further, technicalities aside, and what is appreciably the most important thing, is the example your suggestion is setting for others, as a portmgr member. Please think about the impacts of what you post before you do so.

[1] https://www.freebsd.org/doc/en/books/porters-handbook/makefile-maintainer.html

cem edited the test plan for this revision. (Show Details)

@cem QA looks great, thanks for the context updates

danfe requested changes to this revision.Aug 19 2016, 3:36 PM
danfe edited edge metadata.

Yes it should've been updated months ago. There were several things that I was (and still is) not happy about (the way it deals with libglx.so and libGL.so et al., and that it did not package under tinderbox) that held me back, sorry.

One thing I'm unhappy about this patch that it's pretty huge. Do you mind if I first deorbit no longer needed legacy driver support and clean up Makefile and patches?

In D7569#157344, @danfe wrote:

Yes it should've been updated months ago.

Agreed.

One thing I'm unhappy about this patch that it's pretty huge.

I don't think so.

Do you mind if I first deorbit no longer needed legacy driver support and clean up Makefile and patches?

If you can do it *today*, ok. But I'm not happy to wait for any vague promise of cleanups, like has happened repeatedly in the bug.

Yes, today. Part one (rP420482) had just landed; part two will follow shortly.

In D7569#157495, @danfe wrote:

Yes, today. Part one (rP420482) had just landed; part two will follow shortly.

Great! Thank you for the prompt update.

@danfe regarding the changing how the libglx / libGL etc are handled I'm all ears for any suggestions. Those changes should be a separate PR/review issue.

@kwm of course; I have a half-baked patch ready that I wanted to eventually pass through you before updating nVidia driver, but never pushed myself far and hard enough to finish it. Since now updating the driver is the top priority, I'll hold it off and get back to you once we're done here.

I believe I'm done for the night. Would you please rebase your patchset against the current tree? I realize it might require some work, but resulting patch should be significantly smaller, less cluttered, and thus easier to review. I like it in principal, but would probably add some inline comments. Thanks!

cem edited edge metadata.

Rebase the patch over r420482, r420490, r420496, r420499.

I haven't had a chance to build or test it, but I've got to run for now.

cem edited edge metadata.

Incorporate Tomoaki AOKI's buildfixes.

x11/nvidia-driver/Makefile
60 ↗(On Diff #19511)

This line should be deleted. Non-extra equivalent is already created, so extra-patch-x11-driver-Makefile is now duplicate of patch-x11_driver_Makefile.
The patch itself should also be removed.

cem marked an inline comment as done.

Remove extra-patch x11/driver/Makefile.

x11/nvidia-driver/Makefile
60 ↗(On Diff #19511)

Thanks! It is removed.

Now it builds fine and running for stable/11 (r304528) and head (r304535), both amd64.
Thanks!

Hey @danfe , did you get a chance to take a look at the updated patch? Thanks.

We're only 3 days away from another maintainer timeout…

@cem yes, I'm currently reviewing it. I want to make sure those newly added NVVERSION checks are accurate and thus have to download quite a few driver versions which takes a bit of time on my slow Internet connection. Meanwhile, have a look at in-place comments, thanks.

x11/nvidia-driver/Makefile
68 ↗(On Diff #19514)

Perhaps setting NVSRC=. in the first .if clause above makes sense to avoid double slashes in patch names (though harmless) and silent use of undefined variable (e.g. to prevent accidental breakage in case user somehow manages to have this variable defined elsewhere)?

129 ↗(On Diff #19514)

Oops, sorry, missed this one during the cleanups stage. Removed in rP420797.

x11/nvidia-driver/files/extra-patch-src_nvidia-modeset_nvidia-modeset-freebsd.c
8 ↗(On Diff #19514)

Can we add a comment about why this change (and similar one below) is required? Is it related to the reported panic due to "acquiring duplicate lock" reported here?

x11/nvidia-driver/pkg-plist
13 ↗(On Diff #19514)

These two lines (lib/libnvidia-glcore.so*) look superfluous now after rP420496 I think.

In D7569#158590, @danfe wrote:

@cem yes, I'm currently reviewing it. I want to make sure those newly added NVVERSION checks are accurate and thus have to download quite a few driver versions which takes a bit of time on my slow Internet connection. Meanwhile, have a look at in-place comments, thanks.

Thanks!

x11/nvidia-driver/Makefile
68 ↗(On Diff #19514)

Sure, that wouldn't hurt.

x11/nvidia-driver/files/extra-patch-src_nvidia-modeset_nvidia-modeset-freebsd.c
8 ↗(On Diff #19514)

Yes, the patch comes from comment #50-51 in the bug. I am guessing that the true/false parameter to nvkms_alloc() specifies whether to M_ZERO or not.

Are you looking for a comment in the patch itself, or in the Makefile where the patch is applied? Thanks.

Maybe it would also be fine to mtx_init() with MTX_NEW instead, but I don't think there is a good reason to make that change at this time.

x11/nvidia-driver/pkg-plist
13 ↗(On Diff #19514)

Good catch, thanks!

cem marked 3 inline comments as done.

Fix issues noted by danfe@.

  • Explicitly initialize NVSRC for all versions
  • Comment on why modeset patch is required
  • Remove duplicate plist lines (mismerge)

Two more inline comments, please address. I need to sleep now, and would rather review NVVERSION checks on the morrow. Should not take too long, the patch looks pretty good, but I want it to be nearly perfect. :-)

Your comment on the modeset patch is just fine. It's good to keep all comments in one place (that is, port's Makefile).

x11/nvidia-driver/Makefile
181 ↗(On Diff #19643)

Should more accurately read: "New cap_rights_t structure was introduced in FreeBSD src SVN r255219". This is also more consistent with other similar hacks. Knowing exact revision that required a change is important due to complexity of the port.

182 ↗(On Diff #19643)

First clause (left of &&) should be OSVERSION < 1000053 (see rS255305; apparently I was aware that this change would affect nVidia driver).

In D7569#158688, @danfe wrote:

Two more inline comments, please address. I need to sleep now, and would rather review NVVERSION checks on the morrow. Should not take too long, the patch looks pretty good, but I want it to be nearly perfect. :-)

Thanks, I appreciate the review :-).

Your comment on the modeset patch is just fine. It's good to keep all comments in one place (that is, port's Makefile).

Ok, great.

x11/nvidia-driver/Makefile
181 ↗(On Diff #19643)

Good point, thanks.

182 ↗(On Diff #19643)

Good catch, thanks.

cem marked 2 inline comments as done.

Fix cap_rights_t OSVERSION check and make comment more specific and accurate.
Thanks danfe@.

x11/nvidia-driver/Makefile
127 ↗(On Diff #19650)

Original code should be updated to r420797.

Lines 127 to 129 of original code no longer exists after r420797 and prevents the patch applied cleanly.

Modifying patch (delete 3 lines and fix corresponding @@ line), applies, builds and runs OK for me.

Confirmed. Thanks!
Tested on stable/11 at r304717, amd64, ports tree is at r420859.

Build, install and run: 367.35
Build only: 364.19, 361.45.11, 361.42, 361.28, 358.16, 355.11, 352.79,
            352.55, 352.41, 352.30, 352.21, 349.16, 346.72, 346.59
bengta_sics.se added inline comments.
x11/nvidia-driver/files/extra-patch-src_nvidia-modeset_nvidia-modeset-freebsd.c
8 ↗(On Diff #19514)

I provided this patch. Yes, it is in comment #49. The "acquiring duplicate lock" message however turned out to be benign.

The allocated memory has to be initialised to zero, otherwise lock_initialized() can blow up when it looks into the allocated memory to check if the lock already is initialized. lock_initialized() is called from mtx_init() via lock_init(). These functions are in sys/kern/kern_mutex.c and subr_lock.c.

(I don't find MTX_NEW in 10.3 - is this new in 11?)

x11/nvidia-driver/files/extra-patch-src_nvidia-modeset_nvidia-modeset-freebsd.c
8 ↗(On Diff #19514)

Yeah, I guess MTX_NEW is new in 11 (although it's now 20 months old!): https://svnweb.freebsd.org/base?view=revision&revision=275751

It looks like it wasn't MFCed: http://fxr.watson.org/fxr/trackident?i=MTX_NEW

Works for me with a variant of 11-STABLE when I use nvidia-modeset. If I just try to use kldload nvidia by itself, however, I get strange artifacts and no visible X.

I'm happy with this. Any other comments, danfe@?

I have couple of more inline comments, take a look. NVVERSION checks look accurate, I didn't check every single version in-between, but didn't encounter mismatches so far and I don't want to delay the progress further.

Now, about the patches, I'd appreciate if you do the following prior to final commit (relative to files/):

  • extra-patch-mk-nvidia.lib.mk should be renamed (moved) to patch-mk_nvidia.lib.mk:

svn mv extra-patch-mk-nvidia.lib.mk patch-mk_nvidia.lib.mk

  • Three extra new patch files should be copied from their existing counter-parts (assuming clean checkout):

svn cp extra-patch-src-Makefile extra-patch-src_nvidia_Makefile
svn cp extra-patch-src-nv-freebsd.h extra-patch-src_nvidia_nv-freebsd.h
svn cp extra-patch-src-nv-misc.h extra-patch-src_nvidia_nv-misc.h

Then apply changes to files paths (patch header) and minor context changes to extra-patch-src_nv-freebsd.h. Thanks!

x11/nvidia-driver/files/patch-x11_driver_Makefile
2 ↗(On Diff #19676)

I believe this patch is currently useless (no-op): correct DRIVERDIR (under staging directory) is guaranteed to exist due to pre-install target. Also, it is created because the port depends on x11-servers/xorg-server. Lastly, this is a check for existing state of things: DESTDIR is populated by us and thus meaningless to check for any prerequisites.

We might revisit this logic in the future, but for now let's not keep adding more patches to the port.

x11/nvidia-driver/pkg-plist
57 ↗(On Diff #19676)

This line is bogus (see exactly the same entry on line 61). That's one of the reasons to keep this file sorted. Having it breaks packaging of legacy drivers.

In D7569#159468, @danfe wrote:

I have couple of more inline comments, take a look. NVVERSION checks look accurate, I didn't check every single version in-between, but didn't encounter mismatches so far and I don't want to delay the progress further.

Thanks. At least @junchoon_dec.sakura.ne.jp has tested every version, so it should be okay.

Now, about the patches, I'd appreciate if you do the following prior to final commit (relative to files/):

  • extra-patch-mk-nvidia.lib.mk should be renamed (moved) to patch-mk_nvidia.lib.mk:

svn mv extra-patch-mk-nvidia.lib.mk patch-mk_nvidia.lib.mk

This was already done. :-)

  • Three extra new patch files should be copied from their existing counter-parts (assuming clean checkout):

svn cp extra-patch-src-Makefile extra-patch-src_nvidia_Makefile
svn cp extra-patch-src-nv-freebsd.h extra-patch-src_nvidia_nv-freebsd.h
svn cp extra-patch-src-nv-misc.h extra-patch-src_nvidia_nv-misc.h

Then apply changes to files paths (patch header) and minor context changes to extra-patch-src_nv-freebsd.h. Thanks!

Ok.

cem marked 2 inline comments as done.
  • Remove useless patch.
  • Remove duplicate plist line.
  • Use 'svn cp' to make SVN think the nvidia/ patches originated from the non-nvidia/ versions.

Ok, anything else or is this okay, @danfe ?

mark_tranquillussoftware.co.uk added inline comments.
x11/nvidia-driver/Makefile
13 ↗(On Diff #19760)

This is now obsolete - nVidia have released 367.44 and 370.23

danfe edited edge metadata.

OK clear to land.

Let's just commit what we have tested now. Any remaining changes, small and big fixes (like new version update) should follow normally.

x11/nvidia-driver/Makefile
13 ↗(On Diff #19760)

I believe we can have existing, well tested version committed first. Updates will follow normally.

x11/nvidia-driver/Makefile
13 ↗(On Diff #19760)

Yeah, one step at a time. Thanks @danfe for review and Mark for letting us know about the new version.

This revision was automatically updated to reflect the committed changes.