Bug 222375 - [linux][linsysfs] Support for libdrm/Mesa
Summary: [linux][linsysfs] Support for libdrm/Mesa
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-09-16 19:56 UTC by Greg V
Modified: 2017-09-18 19:53 UTC (History)
1 user (show)

See Also:


Attachments
linsysfs_drm.patch (8.18 KB, patch)
2017-09-16 19:56 UTC, Greg V
no flags Details | Diff
linsysfs_drm.patch (v2) (7.91 KB, patch)
2017-09-17 17:11 UTC, Greg V
no flags Details | Diff
linsysfs_drm.patch (v3) (7.75 KB, patch)
2017-09-17 17:15 UTC, Greg V
no flags Details | Diff
linsysfs_drm.patch (v4) (7.75 KB, patch)
2017-09-17 19:57 UTC, Greg V
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg V 2017-09-16 19:56:45 UTC
Created attachment 186436 [details]
linsysfs_drm.patch

This patch exposes more information about PCI devices (and GPUs in particular) via linsysfs (and linux_common).

This allows unmodified modern 64-bit Linux libdrm to work, which allows modern Linux Mesa to work. Tested with an Ubuntu 16.04 chroot + amdgpu from graphics/drm-next-kmod.

libdrm code that reads these files: https://cgit.freedesktop.org/mesa/drm/tree/xf86drm.c?id=libdrm-2.4.83
Comment 1 Conrad Meyer freebsd_committer 2017-09-17 16:52:24 UTC
Patch mostly looks good to me.

I'd remove the useless comments above fillers for the simple data/device/vendor/etc ones (but keep for uevent_foo and vgapci).

I'd replace strncpy() in linsysfs_fill_vgapci with strlcpy().  I don't think this routine behaves correctly at the limit due to strncpy() and the "%s" format string use.

(Also, strncpy() unnecessarily zero-fills the remainder of the buffer, which isn't needed on every loop iteration.)

This module should use its own malloc type, rather than M_TEMP, but that matches the existing pattern so it doesn't need to be changed.

Indentation seems funky on line 351 in the new file, but that might just be bugzilla.

MAXPATHLEN is hugely excessive for chardevname.
Comment 2 Greg V 2017-09-17 17:11:04 UTC
Created attachment 186477 [details]
linsysfs_drm.patch (v2)

(In reply to Conrad Meyer from comment #1)

Nice, didn't know strlcpy was available in the kernel, I just assumed it wasn't.

Uploaded new version.
Comment 3 Greg V 2017-09-17 17:15:55 UTC
Created attachment 186478 [details]
linsysfs_drm.patch (v3)

Oops, forgot to remove the sys/ctype.h include from linux_util — it was for handling differently named symlinks in /dev/dri (finding the first digit), but that wasn't necessary (and didn't even work).
Comment 4 Conrad Meyer freebsd_committer 2017-09-17 17:23:58 UTC
Indentation here is still wrong:

+			if (devclass != NULL)
+			name = devclass_get_name(devclass);

Otherwise, LGTM.  And, thanks! :-)
Comment 5 Greg V 2017-09-17 19:57:09 UTC
Created attachment 186483 [details]
linsysfs_drm.patch (v4)

Oh, there. Fixed.
Comment 6 commit-hook freebsd_committer 2017-09-17 23:40:51 UTC
A commit references this bug:

Author: cem
Date: Sun Sep 17 23:40:16 UTC 2017
New revision: 323692
URL: https://svnweb.freebsd.org/changeset/base/323692

Log:
  linsysfs(5): Add support for recent libdrm

  Expose more information about PCI devices (and GPUs in particular) via
  linsysfs to libdrm.

  This allows unmodified modern 64-bit Linux libdrm to work, which allows
  modern Linux Mesa to work.  The submitter reports that he tested the change
  with an Ubuntu 16.04 chroot + amdgpu from graphics/drm-next-kmod.

  PR:		222375
  Submitted by:	Greg V <greg AT unrelenting.technology>

Changes:
  head/sys/compat/linsysfs/linsysfs.c
  head/sys/compat/linux/linux_util.c
Comment 7 Greg V 2017-09-18 19:53:11 UTC
BTW, forgot to say — for anyone actually trying to run Linux games and stuff: you'll probably need to set LIBGL_DRI3_DISABLE=1 in your Linux environment. For some reason only DRI2 works with Linux apps for me (even though DRI3 works fine for native apps).