Implement drmGetMinorNameForFD for systems without sysfs by adapting drm_get_device_name_for_fd() from the Mesa loader.
v2: use type parameter to select dev name instead of always using DRM_DEV_NAME
Signed-off-by: Jonathan Gray jsg@jsg.id.au --- xf86drm.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/xf86drm.c b/xf86drm.c index f117716..f93ebc0 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2818,7 +2818,40 @@ static char *drmGetMinorNameForFD(int fd, int type) out_close_dir: closedir(sysdir); #else -#warning "Missing implementation of drmGetMinorNameForFD" + struct stat sbuf; + char buf[PATH_MAX + 1]; + const char *dev_name; + unsigned int maj, min; + int n; + + if (fstat(fd, &sbuf)) + return NULL; + + maj = major(sbuf.st_rdev); + min = minor(sbuf.st_rdev); + + if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode)) + return NULL; + + switch (type) { + case DRM_NODE_PRIMARY: + dev_name = DRM_DEV_NAME; + break; + case DRM_NODE_CONTROL: + dev_name = DRM_CONTROL_DEV_NAME; + break; + case DRM_NODE_RENDER: + dev_name = DRM_RENDER_DEV_NAME; + break; + default: + return NULL; + }; + + n = snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, min); + if (n == -1 || n >= sizeof(buf)) + return NULL; + + return strdup(buf); #endif return NULL; }
Implement drmParseSubsystemType for OpenBSD by always returning DRM_BUS_PCI. No non-pci drm drivers are in the kernel and this is unlikely to change anytime soon as the existing ones aren't permissively licensed.
Signed-off-by: Jonathan Gray jsg@jsg.id.au --- xf86drm.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/xf86drm.c b/xf86drm.c index f93ebc0..35c1fc4 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2887,6 +2887,8 @@ static int drmParseSubsystemType(int maj, int min) return DRM_BUS_PCI;
return -EINVAL; +#elif defined(__OpenBSD__) + return DRM_BUS_PCI; #else #warning "Missing implementation of drmParseSubsystemType" return -EINVAL;
Implement drmParsePciDeviceInfo for OpenBSD by using the new DRM_IOCTL_GET_PCIINFO ioctl.
v2: adapt to drmParsePciDeviceInfo changes and use drmOpenMinor
Signed-off-by: Jonathan Gray jsg@jsg.id.au --- xf86drm.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/xf86drm.c b/xf86drm.c index 35c1fc4..6465953 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -102,6 +102,22 @@ #define DRM_MAJOR 226 /* Linux */ #endif
+#ifdef __OpenBSD__ +struct drm_pciinfo { + uint16_t domain; + uint8_t bus; + uint8_t dev; + uint8_t func; + uint16_t vendor_id; + uint16_t device_id; + uint16_t subvendor_id; + uint16_t subdevice_id; + uint8_t revision_id; +}; + +#define DRM_IOCTL_GET_PCIINFO DRM_IOR(0x15, struct drm_pciinfo) +#endif + #define DRM_MSG_VERBOSITY 3
#define memclear(s) memset(&s, 0, sizeof(s)) @@ -3061,6 +3077,31 @@ static int drmParsePciDeviceInfo(int maj, int min, return parse_config_sysfs_file(maj, min, device);
return 0; +#elif defined(__OpenBSD__) + struct drm_pciinfo pinfo; + int fd, type; + + type = drmGetMinorType(min); + if (type == -1) + return -ENODEV; + + fd = drmOpenMinor(min, 0, type); + if (fd < 0) + return -errno; + + if (drmIoctl(fd, DRM_IOCTL_GET_PCIINFO, &pinfo)) { + close(fd); + return -errno; + } + close(fd); + + device->vendor_id = pinfo.vendor_id; + device->device_id = pinfo.device_id; + device->revision_id = pinfo.revision_id; + device->subvendor_id = pinfo.subvendor_id; + device->subdevice_id = pinfo.subdevice_id; + + return 0; #else #warning "Missing implementation of drmParsePciDeviceInfo" return -EINVAL;
Implement drmParsePciBusInfo for OpenBSD by using the new DRM_IOCTL_GET_PCIINFO ioctl.
v2: use drmGetMinorType to get node type instead of always using DRM_NODE_PRIMARY.
Signed-off-by: Jonathan Gray jsg@jsg.id.au --- xf86drm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/xf86drm.c b/xf86drm.c index 6465953..ea24108 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2947,6 +2947,30 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info) info->func = func;
return 0; +#elif defined(__OpenBSD__) + struct drm_pciinfo pinfo; + int fd, type; + + type = drmGetMinorType(min); + if (type == -1) + return -ENODEV; + + fd = drmOpenMinor(min, 0, type); + if (fd < 0) + return -errno; + + if (drmIoctl(fd, DRM_IOCTL_GET_PCIINFO, &pinfo)) { + close(fd); + return -errno; + } + close(fd); + + info->domain = pinfo.domain; + info->bus = pinfo.bus; + info->dev = pinfo.dev; + info->func = pinfo.func; + + return 0; #else #warning "Missing implementation of drmParsePciBusInfo" return -EINVAL;
DRI devices on OpenBSD are not in their own directory. They reside in /dev with a large number of statically generated /dev nodes.
Avoid stat'ing all of /dev on OpenBSD by implementing this custom path.
v2: - use drmGetMinorType to get node type - adapt to drmProcessPciDevice changes - verify drmParseSubsystemType type is PCI - add a comment describing why this was added
Signed-off-by: Jonathan Gray jsg@jsg.id.au --- xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
diff --git a/xf86drm.c b/xf86drm.c index ea24108..9981be4 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__ + /* + * DRI device nodes on OpenBSD are not in their own directory, they reside + * in /dev along with a large number of statically generated /dev nodes. + * Avoid stat'ing all of /dev needlessly by implementing this custom path. + */ + drmDevicePtr d; + struct stat sbuf; + char node[PATH_MAX + 1]; + const char *dev_name; + int node_type, subsystem_type; + int maj, min, n, ret; + + if (fd == -1 || device == NULL) + return -EINVAL; + + if (fstat(fd, &sbuf)) + return -errno; + + maj = major(sbuf.st_rdev); + min = minor(sbuf.st_rdev); + + if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode)) + return -EINVAL; + + node_type = drmGetMinorType(min); + if (node_type == -1) + return -ENODEV; + + switch (node_type) { + case DRM_NODE_PRIMARY: + dev_name = DRM_DEV_NAME; + break; + case DRM_NODE_CONTROL: + dev_name = DRM_CONTROL_DEV_NAME; + break; + case DRM_NODE_RENDER: + dev_name = DRM_RENDER_DEV_NAME; + break; + default: + return -EINVAL; + }; + + n = snprintf(node, PATH_MAX, dev_name, DRM_DIR_NAME, min); + if (n == -1 || n >= PATH_MAX) + return -errno; + if (stat(node, &sbuf)) + return -EINVAL; + + subsystem_type = drmParseSubsystemType(maj, min); + if (subsystem_type != DRM_BUS_PCI) + return -ENODEV; + + ret = drmProcessPciDevice(&d, node, node_type, maj, min, true, flags); + if (ret) + return ret; + + *device = d; + + return 0; +#else drmDevicePtr *local_devices; drmDevicePtr d; DIR *sysdir; @@ -3357,6 +3418,7 @@ free_devices: free_locals: free(local_devices); return ret; +#endif }
/**
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
DRI devices on OpenBSD are not in their own directory. They reside in /dev with a large number of statically generated /dev nodes.
Avoid stat'ing all of /dev on OpenBSD by implementing this custom path.
v2:
- use drmGetMinorType to get node type
- adapt to drmProcessPciDevice changes
- verify drmParseSubsystemType type is PCI
- add a comment describing why this was added
Thanks for the update Jonathan.
I've pulled v2 in master, Emil
On Mon, Dec 05, 2016 at 05:56:40PM +0000, Emil Velikov wrote:
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
DRI devices on OpenBSD are not in their own directory. They reside in /dev with a large number of statically generated /dev nodes.
Avoid stat'ing all of /dev on OpenBSD by implementing this custom path.
v2:
- use drmGetMinorType to get node type
- adapt to drmProcessPciDevice changes
- verify drmParseSubsystemType type is PCI
- add a comment describing why this was added
Thanks for the update Jonathan.
I've pulled v2 in master, Emil
Thanks, going over what went in I see drmGetMinorNameForFD and the OpenBSD drmGetDevice2 paths need to be adjusted to have the correct minor for the control/render nodes.
ie
base = drmGetMinorBase(type); if (min < base) return error;
min -= base;
I'll send another patch for this.
And the common code could be split into a shared function?
drmGetDeviceNameFromFd2 would do the same thing as drmGetDeviceNameFromFd on OpenBSD as far as I can tell so that could be another shared function instead of the current "missing implementation" warning. Or should drmGetDeviceNameFromFd purposefully not handle render/control nodes?
On 6 December 2016 at 05:12, Jonathan Gray jsg@jsg.id.au wrote:
On Mon, Dec 05, 2016 at 05:56:40PM +0000, Emil Velikov wrote:
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
DRI devices on OpenBSD are not in their own directory. They reside in /dev with a large number of statically generated /dev nodes.
Avoid stat'ing all of /dev on OpenBSD by implementing this custom path.
v2:
- use drmGetMinorType to get node type
- adapt to drmProcessPciDevice changes
- verify drmParseSubsystemType type is PCI
- add a comment describing why this was added
Thanks for the update Jonathan.
I've pulled v2 in master, Emil
Thanks, going over what went in I see drmGetMinorNameForFD and the OpenBSD drmGetDevice2 paths need to be adjusted to have the correct minor for the control/render nodes.
ie
base = drmGetMinorBase(type); if (min < base) return error;
min -= base;
I'll send another patch for this.
And the common code could be split into a shared function?
I don't have a strong preference either way, bth.
drmGetDeviceNameFromFd2 would do the same thing as drmGetDeviceNameFromFd on OpenBSD as far as I can tell so that could be another shared function instead of the current "missing implementation" warning. Or should drmGetDeviceNameFromFd purposefully not handle render/control nodes?
drmGetDeviceNameFromFd has historically handled only card nodes, so I'd keep that as-is. The "2" should handle any node imaginable.
Please, spin the patches when you can and give the OpenBSD implementations a test. I'd like to get those in for the next release - in the next week or so. This way we can use the less annoying drmGetDevice[s]2 in Mesa :-)
Thanks Emil
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/
I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
On Thu, Jun 21, 2018 at 03:24:49PM +0100, Emil Velikov wrote:
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
There are no other devices under a sub-directory besides /dev/fd/. I don't think anyone is going to be onboard with changing things for drm nodes. Though drm device nodes names will have to be revisted when/if render nodes etc get supported. drmR drmC etc have not been proposed yet.
On 21 June 2018 at 16:32, Jonathan Gray jsg@jsg.id.au wrote:
On Thu, Jun 21, 2018 at 03:24:49PM +0100, Emil Velikov wrote:
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
There are no other devices under a sub-directory besides /dev/fd/. I don't think anyone is going to be onboard with changing things for drm nodes. Though drm device nodes names will have to be revisted when/if render nodes etc get supported. drmR drmC etc have not been proposed yet.
I see, that's enlighening.
Out of curiosity: personally, do you see any technical issues with a sub-directory approach?
Thanks Emil
On Tue, Jun 26, 2018 at 11:38:20AM +0100, Emil Velikov wrote:
On 21 June 2018 at 16:32, Jonathan Gray jsg@jsg.id.au wrote:
On Thu, Jun 21, 2018 at 03:24:49PM +0100, Emil Velikov wrote:
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
There are no other devices under a sub-directory besides /dev/fd/. I don't think anyone is going to be onboard with changing things for drm nodes. Though drm device nodes names will have to be revisted when/if render nodes etc get supported. drmR drmC etc have not been proposed yet.
I see, that's enlighening.
Out of curiosity: personally, do you see any technical issues with a sub-directory approach?
I get that you want a single path but it seems these functions were really designed assuming the approach was going to be a sub-directory.
Date: Tue, 26 Jun 2018 20:58:18 +1000 From: Jonathan Gray jsg@jsg.id.au
On Tue, Jun 26, 2018 at 11:38:20AM +0100, Emil Velikov wrote:
On 21 June 2018 at 16:32, Jonathan Gray jsg@jsg.id.au wrote:
On Thu, Jun 21, 2018 at 03:24:49PM +0100, Emil Velikov wrote:
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
There are no other devices under a sub-directory besides /dev/fd/. I don't think anyone is going to be onboard with changing things for drm nodes. Though drm device nodes names will have to be revisted when/if render nodes etc get supported. drmR drmC etc have not been proposed yet.
I see, that's enlighening.
Out of curiosity: personally, do you see any technical issues with a sub-directory approach?
I get that you want a single path but it seems these functions were really designed assuming the approach was going to be a sub-directory.
The bigger picture here is that this code is primarily used to query for information about an open drm file descriptor. The Linux implementation does a lot of filesystem groveling to achieve that. Especially the bits that descend into /sys are never going to work on OpenBSD. If a more OS-agnostic approach is wanted, I would say an ioctl to return the relevant information would be more appropriate. This is actually what we did for OpenBSD where we implemented DRM_IOCTL_GET_PCIINFO to implement drmParsePciBusInfo. Such an approach avoids the issue of mapping the file descriptor back to a file system path. Also note that mapping and open file descriptor to a file system path is fundamentally impossible on Unix without cheating. Although cheating is fairly easy for devices.
If mapping a file descriptor back to a filesystem path is necessary, OpenBSD actually implements devname(3) which uses a database to map device major/minor pairs back to a device name. This is actually a function that was introduced in 4.4BSD, and as far as I can tell it is still present in FreeBSD and NetBSD as well. We could change the OpenBSD implementation of drmGetDevice2() to use that, but it wouldn't really make the code simpler.
All in all, I think it is best to keep the Linux and OpenBSD code separate here.
Hi guys,
Above all, yes the current approach looks a bit funky. Given the constrains (cannot use ioctl and libudev) it's rather reasonable.
That said, ideas for improvements are always welcome.
On 26 June 2018 at 13:03, Mark Kettenis mark.kettenis@xs4all.nl wrote:
Date: Tue, 26 Jun 2018 20:58:18 +1000 From: Jonathan Gray jsg@jsg.id.au
On Tue, Jun 26, 2018 at 11:38:20AM +0100, Emil Velikov wrote:
On 21 June 2018 at 16:32, Jonathan Gray jsg@jsg.id.au wrote:
On Thu, Jun 21, 2018 at 03:24:49PM +0100, Emil Velikov wrote:
Hi Jonathan,
On 1 December 2016 at 04:18, Jonathan Gray jsg@jsg.id.au wrote:
--- a/xf86drm.c +++ b/xf86drm.c @@ -3248,6 +3248,67 @@ drm_device_validate_flags(uint32_t flags) */ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) { +#ifdef __OpenBSD__
- /*
* DRI device nodes on OpenBSD are not in their own directory, they reside* in /dev along with a large number of statically generated /dev nodes.* Avoid stat'ing all of /dev needlessly by implementing this custom path.*/I was in the area, fixing the odd bug and doing some cleanups and a question came to mind:
Is there some obstacle of placing the drm nodes under /dev/dri/? It would involve a check/update through the OpenBSD tree, yet no obvious downsides comes to mind. I think it would make things more distinct and obvious. IIRC the OpenBSD xserver does some checking which /dev node opened, the suggestion should help there.
What do you think? Emil
There are no other devices under a sub-directory besides /dev/fd/. I don't think anyone is going to be onboard with changing things for drm nodes. Though drm device nodes names will have to be revisted when/if render nodes etc get supported. drmR drmC etc have not been proposed yet.
I see, that's enlighening.
Out of curiosity: personally, do you see any technical issues with a sub-directory approach?
I get that you want a single path but it seems these functions were really designed assuming the approach was going to be a sub-directory.
Right, I should have said "Ignoring the actual implementation, do you see any technical issues..."
The bigger picture here is that this code is primarily used to query for information about an open drm file descriptor. The Linux implementation does a lot of filesystem groveling to achieve that. Especially the bits that descend into /sys are never going to work on OpenBSD. If a more OS-agnostic approach is wanted, I would say an ioctl to return the relevant information would be more appropriate. This is actually what we did for OpenBSD where we implemented DRM_IOCTL_GET_PCIINFO to implement drmParsePciBusInfo.
An ioctl approach has two issues: - linux aims to keep the uabi 'forever' as such adding extra ioctl's is fairly hard This particular instance was discussed and rejected with linux devs. - on linux the device (even the bus it's on) can be powered off Issuing an ioctl will wake up the device (which can be slow) even if you don't end up using that device.
Admittedly you'd want the power-off machinery in !linux at some point. GPUs power hungry beasts, even when they're not used ;-)
As I mentioned libudev (yes I know you're not using it), it's also a no-go since apps ship with their own version of it, causing all sorts of grief. We tried that in Mesa and after over a year of various fixes, I nuked it in favour of drmDevice.
Such an approach avoids the issue of mapping the file descriptor back to a file system path. Also note that mapping and open file descriptor to a file system path is fundamentally impossible on Unix without cheating. Although cheating is fairly easy for devices.
Agreed, I've skimmed through the code of lsof and ouch...
If mapping a file descriptor back to a filesystem path is necessary, OpenBSD actually implements devname(3) which uses a database to map device major/minor pairs back to a device name. This is actually a function that was introduced in 4.4BSD, and as far as I can tell it is still present in FreeBSD and NetBSD as well. We could change the OpenBSD implementation of drmGetDevice2() to use that, but it wouldn't really make the code simpler.
All in all, I think it is best to keep the Linux and OpenBSD code separate here.
While I can see the current approach seems foobar, my question was orthogonal.
Namely: is there a thread/documentation covering the technical reasons behind the lack of sub-directories in /dev/?
Thanks Emil
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Jonathan Gray -
Mark Kettenis