| Message ID | 20161201041843.44710-5-jsg@jsg.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 } /**
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(+)