Re: [PATCH] Don't use libudev for glx/dri3

[Emil Velikov <emil.l.velikov@gmail.com>]

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;
>  }
>