Date: Sat, 11 Mar 2017 21:32:07 -0800 From: David Shao <davshao@gmail.com> To: freebsd-x11@freebsd.org Subject: libdrm and xf86drm.c Re: Upstreaming Proposal Message-ID: <CABZaEK4pi%2Bu9CQvdWwxofF2gw3VrLrzAOZpsAaL1b2SZTorAxg@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
TL;DR Stop trying to use libdevq and just copy from it a couple
of functions.
This is what needs to be done to upstream to libdrm FreeBSD
changes to xf86drm.c. Working code implementing these
changes exists and has been published.
Given a file descriptor fd, in the case of FreeBSD to a
device such as /dev/dri/card0 or /dev/dri/controlD64,
and assuming this fd is for some PCI graphics device,
libdrm's most complicated query is to fill in the fields
of a struct listing the numerical values for the devices's
domain, bus, device (slot), function, vendor id, device id,
subvendor id, subdevice id, and revision id.
The kernel knows this information.
The cleanest implementation is OpenBSD's which simply
defines an ioctl and a struct whose fields the kernel simply
fills in.
Especially for kernel drm, unused ioctl numbers seems scarce,
so this approach has not been taken in general for all OSes.
If one is not going to ioctl an file descriptor, what about
fstat-ing it? From fstat one can obtain at least three pieces
of information for the underlying device: major number, minor
number, and the device name. The only reason major and
minor manipulations occur in xf86drm.c is for Linux, and
that is because these numbers are used to lookup entries
under /sys. The BSDs should basically IGNORE all of the
major / minor code and concentrate on the device name.
For FreeBSD the code should parse the device name taking
care to extract the device name number at the end, for
example the 0 from /dev/dri/card0. Unfortunately the PCI
device information is split into two separate branches
of the sysctl tree, part under hw.dri and part under
dev.vgapci. The device name
number (0) should be used to look up under hw.dri.0 the
PCI device's domain, bus, slot, and function. Then this
information should be dragged over to the dev.vgapci branch
to search for the matching entry with the same values, after
which one reads off the vendor, device, subvendor, and
subdevice ids (and guesses the revision id). The code to implement
this is not that difficult, just a series of snprintf,
sscanf, and sysctl calls.
Unfortunately FreeBSD ports keeps insisting that what is the
content of only two fairly trivial functions should be obtained
only by linking with the libdevq project. This is what makes it
impossible for the patches to be upstreamed, first to mesa3d and now to libdrm.
The current drmBSDDeviceNameHack() misses the point
because of this need to hammer the code to use libdevq. There
are hardcoded constants and strings that already are defined
in more general form in libdrm. (And now that drm-next may
have introduced render nodes, how does "/dev/dri/renderD128"
compare as a string to "/dev/dri/controlD64"?) The FreeBSD
ports patch ignores the perfectly fine string comparison code
already in libdrm to determine the type of node in
static int drmGetNodeType(const char *name)
{
if (strncmp(name, DRM_PRIMARY_MINOR_NAME,
sizeof(DRM_PRIMARY_MINOR_NAME) - 1) == 0)
return DRM_NODE_PRIMARY;
if (strncmp(name, DRM_CONTROL_MINOR_NAME,
sizeof(DRM_CONTROL_MINOR_NAME ) - 1) == 0)
return DRM_NODE_CONTROL;
if (strncmp(name, DRM_RENDER_MINOR_NAME,
sizeof(DRM_RENDER_MINOR_NAME) - 1) == 0)
return DRM_NODE_RENDER;
return -EINVAL;
}
But what is most disheartening about the present
drmBSDDeviceNameHack() is that it fails to return separately
for its caller the one piece of information that actually
matters for FreeBSD: the device name number, probably 0.
Instead it just jams this number onto the hacked path name
returned to the caller expecting the caller to parse this name
yet again for the number.
Given that libdrm already has code to determine the node type, maybe
try something like this:
static int
drmBSDDeviceNameHack(const char *path, char *hacked_path,
int length, int node_type)
{
int start, number, base;
const char *errstr;
base = drmGetMinorBase(node_type);
if (node_type == DRM_NODE_RENDER) {
start = sizeof(DRM_RENDER_MINOR_NAME) - 1;
} else if (node_type == DRM_NODE_CONTROL) {
start = sizeof(DRM_CONTROL_MINOR_NAME) - 1;
} else {
start = sizeof(DRM_PRIMARY_MINOR_NAME) - 1;
}
number = strtonum(&(path[start]), 0, 256, &errstr) - base;
snprintf(hacked_path, length, "card%i", number);
return number;
}
Or perhaps change the libdrm node type parsing code to do all
the parsing in one shot.
Having obtained this device name number 0, we change to:
#if defined (__FreeBSD__) || defined(__DragonFly__)
ret = drmProcessPciDevice(&d, node, node_type, maj, number,
true, flags);
+#else
instead of using the useless minor number min.
Now because the domain, bus, slot, and function numbers
need to be dragged over to search dev.vgapci, we need an
internal API change of the form:
#if defined(__FreeBSD__)
static int drmParsePciDeviceInfoBSD(int maj, int min,
drmPciDeviceInfoPtr device,
drmPciBusInfoPtr info,
uint32_t flags)
#else
static int drmParsePciDeviceInfo(int maj, int min,
drmPciDeviceInfoPtr device,
uint32_t flags)
Working patches tested on an Intel IvyBridge integrated
graphics machine can be found in the pkgsrc-wip project
libdrm-dfbsd, patch name patch-ab. This patch is bloated
up by a tremendous number of drmMsg debugging lines
I added and also by code to deal with DragonFly not having
a well-defined idea of a major number.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CABZaEK4pi%2Bu9CQvdWwxofF2gw3VrLrzAOZpsAaL1b2SZTorAxg>