| Message ID | 1384736916-25285-1-git-send-email-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/11/13 01:08, Keith Packard wrote: > libudev doesn't have a stable API/ABI, and if the application wants to use one > version, we'd best not load another into libGL. > > Signed-off-by: Keith Packard <keithp@keithp.com> > --- > Hi Keith, Firstly, I would like to apologise for the rather daft questions. With that said, looking through your patch I've noticed that (most of) your int functions are returning 0 or failure and 1 on success. Were those functions meant to return boolean/bool? After a quick look at the dri3 and dri2 code base I've noticed that there are a lot of cases where the function returns True/False, wrapped as int. I'm assuming that there is some reason behind these. Do you have some references were I can look a bit more on the subject ? Cheers, Emil > Sorry for the patch spam; I hadn't rebased in a while and there was a > configure.ac conflict. Here's a version which should apply cleanly to master. > > configure.ac | 8 ---- > src/glx/dri3_common.c | 104 +++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 90 insertions(+), 22 deletions(-) > > diff --git a/configure.ac b/configure.ac > index fb16338..656d9d0 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -821,9 +821,6 @@ xyesno) > PKG_CHECK_MODULES([DRI2PROTO], [dri2proto >= $DRI2PROTO_REQUIRED]) > GL_PC_REQ_PRIV="$GL_PC_REQ_PRIV libdrm >= $LIBDRM_REQUIRED" > if test x"$enable_dri3" = xyes; then > - if test x"$have_libudev" != xyes; then > - AC_MSG_ERROR([DRI3 requires libudev >= $LIBUDEV_REQUIRED]) > - fi > PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >= $DRI3PROTO_REQUIRED]) > PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED]) > fi > @@ -847,11 +844,6 @@ xyesno) > X11_INCLUDES="$X11_INCLUDES $DRIGL_CFLAGS" > GL_LIB_DEPS="$DRIGL_LIBS" > > - if test x"$enable_dri3$have_libudev" = xyesyes; then > - X11_INCLUDES="$X11_INCLUDES $LIBUDEV_CFLAGS" > - GL_LIB_DEPS="$GL_LIB_DEPS $LIBUDEV_LIBS" > - fi > - > # need DRM libs, $PTHREAD_LIBS, etc. > GL_LIB_DEPS="$GL_LIB_DEPS $LIBDRM_LIBS -lm $PTHREAD_LIBS $DLOPEN_LIBS" > GL_PC_LIB_PRIV="-lm $PTHREAD_LIBS $DLOPEN_LIBS" > diff --git a/src/glx/dri3_common.c b/src/glx/dri3_common.c > index c758f96..7330d79 100644 > --- a/src/glx/dri3_common.c > +++ b/src/glx/dri3_common.c > @@ -72,22 +72,41 @@ > #include "dri3_priv.h" > > #define DRIVER_MAP_DRI3_ONLY > + > #include "pci_ids/pci_id_driver_map.h" > > +static dev_t > +dri3_rdev_from_fd(int fd) > +{ > + struct stat buf; > + > + if (fstat(fd, &buf) < 0) { > + ErrorMessageF("DRI3: failed to stat fd %d", fd); > + return 0; > + } > + return buf.st_rdev; > +} > + > +/* > + * There are multiple udev library versions, and they aren't polite about > + * symbols, so just avoid using it until some glorious future when the udev > + * developers figure out how to not break things > + */ > + > +#define USE_UDEV 0 > +#if USE_UDEV > #include <libudev.h> > > static struct udev_device * > dri3_udev_device_new_from_fd(struct udev *udev, int fd) > { > struct udev_device *device; > - struct stat buf; > + dev_t rdev = dri3_rdev_from_fd(fd); > > - if (fstat(fd, &buf) < 0) { > - ErrorMessageF("DRI3: failed to stat fd %d", fd); > + if (rdev == 0) > return NULL; > - } > > - device = udev_device_new_from_devnum(udev, 'c', buf.st_rdev); > + device = udev_device_new_from_devnum(udev, 'c', rdev); > if (device == NULL) { > ErrorMessageF("DRI3: could not create udev device for fd %d", fd); > return NULL; > @@ -96,19 +115,20 @@ dri3_udev_device_new_from_fd(struct udev *udev, int fd) > return device; > } > > -char * > -dri3_get_driver_for_fd(int fd) > +static int > +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) > { > struct udev *udev; > struct udev_device *device, *parent; > const char *pci_id; > - char *driver = NULL; > - int vendor_id, chip_id, i, j; > + int ret = 0; > > udev = udev_new(); > + if (!udev) > + goto no_udev; > device = dri3_udev_device_new_from_fd(udev, fd); > if (device == NULL) > - return NULL; > + goto no_dev; > > parent = udev_device_get_parent(device); > if (parent == NULL) { > @@ -118,10 +138,69 @@ dri3_get_driver_for_fd(int fd) > > pci_id = udev_device_get_property_value(parent, "PCI_ID"); > if (pci_id == NULL || > - sscanf(pci_id, "%x:%x", &vendor_id, &chip_id) != 2) { > + sscanf(pci_id, "%x:%x", vendorp, chipp) != 2) { > ErrorMessageF("DRI3: malformed or no PCI ID"); > goto out; > } > + ret = 1; > + > +out: > + udev_device_unref(device); > +no_dev: > + udev_unref(udev); > +no_udev: > + return ret; > +} > +#else > + > +#define SYS_PATH_MAX 256 > + > +static int > +dri3_read_hex(dev_t rdev, char *entry, int *value) > +{ > + char path[SYS_PATH_MAX]; > + FILE *f; > + int r; > + > + snprintf(path, sizeof (path), "/sys/dev/char/%u:%u/device/%s", > + major(rdev), minor(rdev), entry); > + > + f = fopen(path,"r"); > + if (f == NULL) > + return 0; > + > + r = fscanf(f, "0x%x\n", value); > + fclose(f); > + if (r != 1) > + return 0; > + return 1; > +} > + > +static int > +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) > +{ > + dev_t rdev = dri3_rdev_from_fd(fd); > + > + if (!rdev) > + return 0; > + > + if (!dri3_read_hex(rdev, "vendor", vendorp)) > + return 0; > + if (!dri3_read_hex(rdev, "device", chipp)) > + return 0; > + return 1; > +} > + > +#endif > + > +char * > +dri3_get_driver_for_fd(int fd) > +{ > + char *driver = NULL; > + int vendor_id, chip_id, i, j; > + > + if (!dri3_get_pci_id_from_fd(fd, &vendor_id, &chip_id)) > + return NULL; > > for (i = 0; driver_map[i].driver; i++) { > if (vendor_id != driver_map[i].vendor_id) > @@ -139,8 +218,5 @@ dri3_get_driver_for_fd(int fd) > } > > out: > - udev_device_unref(device); > - udev_unref(udev); > - > return driver; > } >
Emil Velikov <emil.l.velikov@gmail.com> writes: > On 18/11/13 01:08, Keith Packard wrote: >> libudev doesn't have a stable API/ABI, and if the application wants to use one >> version, we'd best not load another into libGL. >> >> Signed-off-by: Keith Packard <keithp@keithp.com> >> --- >> > Hi Keith, > > Firstly, I would like to apologise for the rather daft questions. > > With that said, looking through your patch I've noticed that (most of) > your int functions are returning 0 or failure and 1 on success. Were > those functions meant to return boolean/bool? Sure, but I was too lazy to figure out which kind of bool belongs in that part of mesa...
On 11/17/2013 06:43 PM, Keith Packard wrote: > Emil Velikov <emil.l.velikov@gmail.com> writes: > >> On 18/11/13 01:08, Keith Packard wrote: >>> libudev doesn't have a stable API/ABI, and if the application wants to use one >>> version, we'd best not load another into libGL. >>> >>> Signed-off-by: Keith Packard <keithp@keithp.com> >>> --- >>> >> Hi Keith, >> >> Firstly, I would like to apologise for the rather daft questions. >> >> With that said, looking through your patch I've noticed that (most of) >> your int functions are returning 0 or failure and 1 on success. Were >> those functions meant to return boolean/bool? > > Sure, but I was too lazy to figure out which kind of bool belongs in > that part of mesa... For non-API facing stuff, you can just use stdbool.h's bool/true/false. --Ken
Kenneth Graunke <kenneth@whitecape.org> writes: > For non-API facing stuff, you can just use stdbool.h's > bool/true/false. tnx. I pushed that to my dri3 branch. Btw, that branch also has some gallium support for __DRIimageDriverExtension. I don't have any hardware to test with, so it's surely broken in some major ways, but it might show where the hooks need to be.
On 18/11/13 01:08, Keith Packard wrote: > libudev doesn't have a stable API/ABI, and if the application wants to use one > version, we'd best not load another into libGL. > > Signed-off-by: Keith Packard <keithp@keithp.com> > --- > Hi Keith, Did you had the chance to look at src/gallium/targets/egl-static/egl.c? It has a different implementation of drm_fd_get_pci_id, whenever udev is not available. AFAICS it goes back to the kernel via the relevant ioctl to retrieve the deviceid/chipid. Currently all but nouveau provide such information. I'm thinking that this approach might be more reasonable for those concerned with portability of the udev bits (think on *BSD). I'm not nitpicking, just thought you might find this interesting. Cheers, Emil > Sorry for the patch spam; I hadn't rebased in a while and there was a > configure.ac conflict. Here's a version which should apply cleanly to master. > > configure.ac | 8 ---- > src/glx/dri3_common.c | 104 +++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 90 insertions(+), 22 deletions(-) > > diff --git a/configure.ac b/configure.ac > index fb16338..656d9d0 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -821,9 +821,6 @@ xyesno) > PKG_CHECK_MODULES([DRI2PROTO], [dri2proto >= $DRI2PROTO_REQUIRED]) > GL_PC_REQ_PRIV="$GL_PC_REQ_PRIV libdrm >= $LIBDRM_REQUIRED" > if test x"$enable_dri3" = xyes; then > - if test x"$have_libudev" != xyes; then > - AC_MSG_ERROR([DRI3 requires libudev >= $LIBUDEV_REQUIRED]) > - fi > PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >= $DRI3PROTO_REQUIRED]) > PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED]) > fi > @@ -847,11 +844,6 @@ xyesno) > X11_INCLUDES="$X11_INCLUDES $DRIGL_CFLAGS" > GL_LIB_DEPS="$DRIGL_LIBS" > > - if test x"$enable_dri3$have_libudev" = xyesyes; then > - X11_INCLUDES="$X11_INCLUDES $LIBUDEV_CFLAGS" > - GL_LIB_DEPS="$GL_LIB_DEPS $LIBUDEV_LIBS" > - fi > - > # need DRM libs, $PTHREAD_LIBS, etc. > GL_LIB_DEPS="$GL_LIB_DEPS $LIBDRM_LIBS -lm $PTHREAD_LIBS $DLOPEN_LIBS" > GL_PC_LIB_PRIV="-lm $PTHREAD_LIBS $DLOPEN_LIBS" > diff --git a/src/glx/dri3_common.c b/src/glx/dri3_common.c > index c758f96..7330d79 100644 > --- a/src/glx/dri3_common.c > +++ b/src/glx/dri3_common.c > @@ -72,22 +72,41 @@ > #include "dri3_priv.h" > > #define DRIVER_MAP_DRI3_ONLY > + > #include "pci_ids/pci_id_driver_map.h" > > +static dev_t > +dri3_rdev_from_fd(int fd) > +{ > + struct stat buf; > + > + if (fstat(fd, &buf) < 0) { > + ErrorMessageF("DRI3: failed to stat fd %d", fd); > + return 0; > + } > + return buf.st_rdev; > +} > + > +/* > + * There are multiple udev library versions, and they aren't polite about > + * symbols, so just avoid using it until some glorious future when the udev > + * developers figure out how to not break things > + */ > + > +#define USE_UDEV 0 > +#if USE_UDEV > #include <libudev.h> > > static struct udev_device * > dri3_udev_device_new_from_fd(struct udev *udev, int fd) > { > struct udev_device *device; > - struct stat buf; > + dev_t rdev = dri3_rdev_from_fd(fd); > > - if (fstat(fd, &buf) < 0) { > - ErrorMessageF("DRI3: failed to stat fd %d", fd); > + if (rdev == 0) > return NULL; > - } > > - device = udev_device_new_from_devnum(udev, 'c', buf.st_rdev); > + device = udev_device_new_from_devnum(udev, 'c', rdev); > if (device == NULL) { > ErrorMessageF("DRI3: could not create udev device for fd %d", fd); > return NULL; > @@ -96,19 +115,20 @@ dri3_udev_device_new_from_fd(struct udev *udev, int fd) > return device; > } > > -char * > -dri3_get_driver_for_fd(int fd) > +static int > +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) > { > struct udev *udev; > struct udev_device *device, *parent; > const char *pci_id; > - char *driver = NULL; > - int vendor_id, chip_id, i, j; > + int ret = 0; > > udev = udev_new(); > + if (!udev) > + goto no_udev; > device = dri3_udev_device_new_from_fd(udev, fd); > if (device == NULL) > - return NULL; > + goto no_dev; > > parent = udev_device_get_parent(device); > if (parent == NULL) { > @@ -118,10 +138,69 @@ dri3_get_driver_for_fd(int fd) > > pci_id = udev_device_get_property_value(parent, "PCI_ID"); > if (pci_id == NULL || > - sscanf(pci_id, "%x:%x", &vendor_id, &chip_id) != 2) { > + sscanf(pci_id, "%x:%x", vendorp, chipp) != 2) { > ErrorMessageF("DRI3: malformed or no PCI ID"); > goto out; > } > + ret = 1; > + > +out: > + udev_device_unref(device); > +no_dev: > + udev_unref(udev); > +no_udev: > + return ret; > +} > +#else > + > +#define SYS_PATH_MAX 256 > + > +static int > +dri3_read_hex(dev_t rdev, char *entry, int *value) > +{ > + char path[SYS_PATH_MAX]; > + FILE *f; > + int r; > + > + snprintf(path, sizeof (path), "/sys/dev/char/%u:%u/device/%s", > + major(rdev), minor(rdev), entry); > + > + f = fopen(path,"r"); > + if (f == NULL) > + return 0; > + > + r = fscanf(f, "0x%x\n", value); > + fclose(f); > + if (r != 1) > + return 0; > + return 1; > +} > + > +static int > +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) > +{ > + dev_t rdev = dri3_rdev_from_fd(fd); > + > + if (!rdev) > + return 0; > + > + if (!dri3_read_hex(rdev, "vendor", vendorp)) > + return 0; > + if (!dri3_read_hex(rdev, "device", chipp)) > + return 0; > + return 1; > +} > + > +#endif > + > +char * > +dri3_get_driver_for_fd(int fd) > +{ > + char *driver = NULL; > + int vendor_id, chip_id, i, j; > + > + if (!dri3_get_pci_id_from_fd(fd, &vendor_id, &chip_id)) > + return NULL; > > for (i = 0; driver_map[i].driver; i++) { > if (vendor_id != driver_map[i].vendor_id) > @@ -139,8 +218,5 @@ dri3_get_driver_for_fd(int fd) > } > > out: > - udev_device_unref(device); > - udev_unref(udev); > - > return driver; > } >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/17/2013 06:43 PM, Keith Packard wrote: > Emil Velikov <emil.l.velikov@gmail.com> writes: > >> On 18/11/13 01:08, Keith Packard wrote: >>> libudev doesn't have a stable API/ABI, and if the application >>> wants to use one version, we'd best not load another into >>> libGL. >>> >>> Signed-off-by: Keith Packard <keithp@keithp.com> --- >>> >> Hi Keith, >> >> Firstly, I would like to apologise for the rather daft >> questions. >> >> With that said, looking through your patch I've noticed that >> (most of) your int functions are returning 0 or failure and 1 on >> success. Were those functions meant to return boolean/bool? > > Sure, but I was too lazy to figure out which kind of bool belongs > in that part of mesa... For future reference... for things not accesible to the application or some other library (with its own boolean type), the answer is always bool from stdbool.h. > _______________________________________________ dri-devel mailing > list dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iEYEARECAAYFAlKKjbcACgkQX1gOwKyEAw9O8gCeM1SQmZcOSaSxJy5yT42ItxQU 1UUAoI9Gl1ah9P1/0AG+huawKAPzF9V9 =d3i2 -----END PGP SIGNATURE-----
Emil Velikov <emil.l.velikov@gmail.com> writes: > On 18/11/13 01:08, Keith Packard wrote: >> libudev doesn't have a stable API/ABI, and if the application wants to use one >> version, we'd best not load another into libGL. >> >> Signed-off-by: Keith Packard <keithp@keithp.com> >> --- >> > Hi Keith, > > Did you had the chance to look at src/gallium/targets/egl-static/egl.c? > It has a different implementation of drm_fd_get_pci_id, whenever udev is > not available. Yeah, it's ugly in a different way from the udev technique... > AFAICS it goes back to the kernel via the relevant ioctl to retrieve the > deviceid/chipid. Currently all but nouveau provide such information. I'm > thinking that this approach might be more reasonable for those concerned > with portability of the udev bits (think on *BSD). I'd encourage some kind of standard IOCTL from DRM that returns the PCI-ID of the underlying device, rather than relying on the level of kludge present in either the udev (or my fake udev) method or the non-udev path in the egl code... > I'm not nitpicking, just thought you might find this interesting. Definitely interesting; it's almost what we want -- the kernel knows the information, there just isn't a clean way of getting it (and no way at all for some devices).
Keith Packard <keithp@keithp.com> writes: > libudev doesn't have a stable API/ABI, and if the application wants to use one > version, we'd best not load another into libGL. > > Signed-off-by: Keith Packard <keithp@keithp.com> This looks so simple it's definitely worth dropping udev, but I'd like to see the udev code actually dropped instead of #if 0ed.
Eric Anholt <eric@anholt.net> writes: > Keith Packard <keithp@keithp.com> writes: > >> libudev doesn't have a stable API/ABI, and if the application wants to use one >> version, we'd best not load another into libGL. >> >> Signed-off-by: Keith Packard <keithp@keithp.com> > > This looks so simple it's definitely worth dropping udev, but I'd like > to see the udev code actually dropped instead of #if 0ed. It's either that or make it a config option, in case libudev is useful on some other system someday?
Keith Packard <keithp@keithp.com> writes: > Eric Anholt <eric@anholt.net> writes: > >> Keith Packard <keithp@keithp.com> writes: >> >>> libudev doesn't have a stable API/ABI, and if the application wants to use one >>> version, we'd best not load another into libGL. >>> >>> Signed-off-by: Keith Packard <keithp@keithp.com> >> >> This looks so simple it's definitely worth dropping udev, but I'd like >> to see the udev code actually dropped instead of #if 0ed. > > It's either that or make it a config option, in case libudev is useful > on some other system someday? I hate build options.
On 11/18/2013 12:58 PM, Emil Velikov wrote: > On 18/11/13 01:08, Keith Packard wrote: >> libudev doesn't have a stable API/ABI, and if the application wants to use one >> version, we'd best not load another into libGL. >> >> Signed-off-by: Keith Packard <keithp@keithp.com> >> --- >> > Hi Keith, > > Did you had the chance to look at src/gallium/targets/egl-static/egl.c? > It has a different implementation of drm_fd_get_pci_id, whenever udev is > not available. > > AFAICS it goes back to the kernel via the relevant ioctl to retrieve the > deviceid/chipid. Currently all but nouveau provide such information. I'm > thinking that this approach might be more reasonable for those concerned > with portability of the udev bits (think on *BSD). > > I'm not nitpicking, just thought you might find this interesting. > > Cheers, > Emil Possibly. But looking at that code, it either: 1. Uses libudev 2. On Android only...strcmps the driver string to guess the family and then uses kernel ioctls... 3. Fails. The Android only nature makes me a bit wary.
On Sat, Dec 14, 2013 at 3:33 AM, Kenneth Graunke <kenneth@whitecape.org> wrote: > On 11/18/2013 12:58 PM, Emil Velikov wrote: >> On 18/11/13 01:08, Keith Packard wrote: >>> libudev doesn't have a stable API/ABI, and if the application wants to use one >>> version, we'd best not load another into libGL. >>> >>> Signed-off-by: Keith Packard <keithp@keithp.com> >>> --- >>> >> Hi Keith, >> >> Did you had the chance to look at src/gallium/targets/egl-static/egl.c? >> It has a different implementation of drm_fd_get_pci_id, whenever udev is >> not available. >> >> AFAICS it goes back to the kernel via the relevant ioctl to retrieve the >> deviceid/chipid. Currently all but nouveau provide such information. I'm >> thinking that this approach might be more reasonable for those concerned >> with portability of the udev bits (think on *BSD). >> >> I'm not nitpicking, just thought you might find this interesting. >> >> Cheers, >> Emil > > Possibly. But looking at that code, it either: > 1. Uses libudev > 2. On Android only...strcmps the driver string to guess the family and > then uses kernel ioctls... > 3. Fails. > > The Android only nature makes me a bit wary. The string mapping of the kernel driver name to a dri.so is what I kinda expected to be used as the generic thing. Imo we don't ever need the pci id in common code, drivers can load that through a private ioctl from the kernel (like i915 and radeon do) or load some other device id from somewhere (nouveau has that at the special reg offset 0 in the bar, also exposed through some ioctl iirc). That way we also avoid hassles on SoCs which lack enumerable buses. And due to abi guarantees those drm driver names will stay the same forever. Also there's the entire drmOpenByBusid stuff which gives you at least something reasonable for pci devices (not anywhere else though), so we could do that dance in reverse if we want to have a pci id based lookup. Anyway I've never read through the loader code, just figured it won't hurt if I drop my uninformed opinion ;-) Cheers, Daniel
Daniel Vetter <daniel@ffwll.ch> writes: > On Sat, Dec 14, 2013 at 3:33 AM, Kenneth Graunke <kenneth@whitecape.org> wrote: >> On 11/18/2013 12:58 PM, Emil Velikov wrote: >>> On 18/11/13 01:08, Keith Packard wrote: >>>> libudev doesn't have a stable API/ABI, and if the application wants to use one >>>> version, we'd best not load another into libGL. >>>> >>>> Signed-off-by: Keith Packard <keithp@keithp.com> >>>> --- >>>> >>> Hi Keith, >>> >>> Did you had the chance to look at src/gallium/targets/egl-static/egl.c? >>> It has a different implementation of drm_fd_get_pci_id, whenever udev is >>> not available. >>> >>> AFAICS it goes back to the kernel via the relevant ioctl to retrieve the >>> deviceid/chipid. Currently all but nouveau provide such information. I'm >>> thinking that this approach might be more reasonable for those concerned >>> with portability of the udev bits (think on *BSD). >>> >>> I'm not nitpicking, just thought you might find this interesting. >>> >>> Cheers, >>> Emil >> >> Possibly. But looking at that code, it either: >> 1. Uses libudev >> 2. On Android only...strcmps the driver string to guess the family and >> then uses kernel ioctls... >> 3. Fails. >> >> The Android only nature makes me a bit wary. > > The string mapping of the kernel driver name to a dri.so is what I > kinda expected to be used as the generic thing [snip] There is more than one userspace driver per kernel driver. (i915 vs i965, for example)
On Mon, Dec 16, 2013 at 11:19:56AM -0800, Eric Anholt wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > > On Sat, Dec 14, 2013 at 3:33 AM, Kenneth Graunke <kenneth@whitecape.org> wrote: > >> On 11/18/2013 12:58 PM, Emil Velikov wrote: > >>> On 18/11/13 01:08, Keith Packard wrote: > >>>> libudev doesn't have a stable API/ABI, and if the application wants to use one > >>>> version, we'd best not load another into libGL. > >>>> > >>>> Signed-off-by: Keith Packard <keithp@keithp.com> > >>>> --- > >>>> > >>> Hi Keith, > >>> > >>> Did you had the chance to look at src/gallium/targets/egl-static/egl.c? > >>> It has a different implementation of drm_fd_get_pci_id, whenever udev is > >>> not available. > >>> > >>> AFAICS it goes back to the kernel via the relevant ioctl to retrieve the > >>> deviceid/chipid. Currently all but nouveau provide such information. I'm > >>> thinking that this approach might be more reasonable for those concerned > >>> with portability of the udev bits (think on *BSD). > >>> > >>> I'm not nitpicking, just thought you might find this interesting. > >>> > >>> Cheers, > >>> Emil > >> > >> Possibly. But looking at that code, it either: > >> 1. Uses libudev > >> 2. On Android only...strcmps the driver string to guess the family and > >> then uses kernel ioctls... > >> 3. Fails. > >> > >> The Android only nature makes me a bit wary. > > > > The string mapping of the kernel driver name to a dri.so is what I > > kinda expected to be used as the generic thing > > [snip] > > There is more than one userspace driver per kernel driver. (i915 vs > i965, for example) Oh right ... so I guess we'd need a shim which figures out the details. Which is kinda what that hack in the gallium egl loader does. I guess I better hide in the kernel again ;-) -Daniel
diff --git a/configure.ac b/configure.ac index fb16338..656d9d0 100644 --- a/configure.ac +++ b/configure.ac @@ -821,9 +821,6 @@ xyesno) PKG_CHECK_MODULES([DRI2PROTO], [dri2proto >= $DRI2PROTO_REQUIRED]) GL_PC_REQ_PRIV="$GL_PC_REQ_PRIV libdrm >= $LIBDRM_REQUIRED" if test x"$enable_dri3" = xyes; then - if test x"$have_libudev" != xyes; then - AC_MSG_ERROR([DRI3 requires libudev >= $LIBUDEV_REQUIRED]) - fi PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >= $DRI3PROTO_REQUIRED]) PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >= $PRESENTPROTO_REQUIRED]) fi @@ -847,11 +844,6 @@ xyesno) X11_INCLUDES="$X11_INCLUDES $DRIGL_CFLAGS" GL_LIB_DEPS="$DRIGL_LIBS" - if test x"$enable_dri3$have_libudev" = xyesyes; then - X11_INCLUDES="$X11_INCLUDES $LIBUDEV_CFLAGS" - GL_LIB_DEPS="$GL_LIB_DEPS $LIBUDEV_LIBS" - fi - # need DRM libs, $PTHREAD_LIBS, etc. GL_LIB_DEPS="$GL_LIB_DEPS $LIBDRM_LIBS -lm $PTHREAD_LIBS $DLOPEN_LIBS" GL_PC_LIB_PRIV="-lm $PTHREAD_LIBS $DLOPEN_LIBS" diff --git a/src/glx/dri3_common.c b/src/glx/dri3_common.c index c758f96..7330d79 100644 --- a/src/glx/dri3_common.c +++ b/src/glx/dri3_common.c @@ -72,22 +72,41 @@ #include "dri3_priv.h" #define DRIVER_MAP_DRI3_ONLY + #include "pci_ids/pci_id_driver_map.h" +static dev_t +dri3_rdev_from_fd(int fd) +{ + struct stat buf; + + if (fstat(fd, &buf) < 0) { + ErrorMessageF("DRI3: failed to stat fd %d", fd); + return 0; + } + return buf.st_rdev; +} + +/* + * There are multiple udev library versions, and they aren't polite about + * symbols, so just avoid using it until some glorious future when the udev + * developers figure out how to not break things + */ + +#define USE_UDEV 0 +#if USE_UDEV #include <libudev.h> static struct udev_device * dri3_udev_device_new_from_fd(struct udev *udev, int fd) { struct udev_device *device; - struct stat buf; + dev_t rdev = dri3_rdev_from_fd(fd); - if (fstat(fd, &buf) < 0) { - ErrorMessageF("DRI3: failed to stat fd %d", fd); + if (rdev == 0) return NULL; - } - device = udev_device_new_from_devnum(udev, 'c', buf.st_rdev); + device = udev_device_new_from_devnum(udev, 'c', rdev); if (device == NULL) { ErrorMessageF("DRI3: could not create udev device for fd %d", fd); return NULL; @@ -96,19 +115,20 @@ dri3_udev_device_new_from_fd(struct udev *udev, int fd) return device; } -char * -dri3_get_driver_for_fd(int fd) +static int +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) { struct udev *udev; struct udev_device *device, *parent; const char *pci_id; - char *driver = NULL; - int vendor_id, chip_id, i, j; + int ret = 0; udev = udev_new(); + if (!udev) + goto no_udev; device = dri3_udev_device_new_from_fd(udev, fd); if (device == NULL) - return NULL; + goto no_dev; parent = udev_device_get_parent(device); if (parent == NULL) { @@ -118,10 +138,69 @@ dri3_get_driver_for_fd(int fd) pci_id = udev_device_get_property_value(parent, "PCI_ID"); if (pci_id == NULL || - sscanf(pci_id, "%x:%x", &vendor_id, &chip_id) != 2) { + sscanf(pci_id, "%x:%x", vendorp, chipp) != 2) { ErrorMessageF("DRI3: malformed or no PCI ID"); goto out; } + ret = 1; + +out: + udev_device_unref(device); +no_dev: + udev_unref(udev); +no_udev: + return ret; +} +#else + +#define SYS_PATH_MAX 256 + +static int +dri3_read_hex(dev_t rdev, char *entry, int *value) +{ + char path[SYS_PATH_MAX]; + FILE *f; + int r; + + snprintf(path, sizeof (path), "/sys/dev/char/%u:%u/device/%s", + major(rdev), minor(rdev), entry); + + f = fopen(path,"r"); + if (f == NULL) + return 0; + + r = fscanf(f, "0x%x\n", value); + fclose(f); + if (r != 1) + return 0; + return 1; +} + +static int +dri3_get_pci_id_from_fd(int fd, int *vendorp, int *chipp) +{ + dev_t rdev = dri3_rdev_from_fd(fd); + + if (!rdev) + return 0; + + if (!dri3_read_hex(rdev, "vendor", vendorp)) + return 0; + if (!dri3_read_hex(rdev, "device", chipp)) + return 0; + return 1; +} + +#endif + +char * +dri3_get_driver_for_fd(int fd) +{ + char *driver = NULL; + int vendor_id, chip_id, i, j; + + if (!dri3_get_pci_id_from_fd(fd, &vendor_id, &chip_id)) + return NULL; for (i = 0; driver_map[i].driver; i++) { if (vendor_id != driver_map[i].vendor_id) @@ -139,8 +218,5 @@ dri3_get_driver_for_fd(int fd) } out: - udev_device_unref(device); - udev_unref(udev); - return driver; }
libudev doesn't have a stable API/ABI, and if the application wants to use one version, we'd best not load another into libGL. Signed-off-by: Keith Packard <keithp@keithp.com> --- Sorry for the patch spam; I hadn't rebased in a while and there was a configure.ac conflict. Here's a version which should apply cleanly to master. configure.ac | 8 ---- src/glx/dri3_common.c | 104 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 90 insertions(+), 22 deletions(-)