diff mbox

Don't use libudev for glx/dri3

Message ID 1384736916-25285-1-git-send-email-keithp@keithp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Keith Packard Nov. 18, 2013, 1:08 a.m. UTC
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(-)

Comments

Emil Velikov Nov. 18, 2013, 1:31 a.m. UTC | #1
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;
>  }
>
Keith Packard Nov. 18, 2013, 2:43 a.m. UTC | #2
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...
Kenneth Graunke Nov. 18, 2013, 4:08 a.m. UTC | #3
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
Keith Packard Nov. 18, 2013, 6:03 a.m. UTC | #4
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.
Emil Velikov Nov. 18, 2013, 8:58 p.m. UTC | #5
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;
>  }
>
Ian Romanick Nov. 18, 2013, 9:59 p.m. UTC | #6
-----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-----
Keith Packard Nov. 19, 2013, 6:46 a.m. UTC | #7
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).
Eric Anholt Nov. 19, 2013, 6:41 p.m. UTC | #8
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.
Keith Packard Nov. 19, 2013, 7:41 p.m. UTC | #9
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?
Eric Anholt Nov. 19, 2013, 8:19 p.m. UTC | #10
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.
Kenneth Graunke Dec. 14, 2013, 2:33 a.m. UTC | #11
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.
Daniel Vetter Dec. 14, 2013, 10:37 a.m. UTC | #12
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
Eric Anholt Dec. 16, 2013, 7:19 p.m. UTC | #13
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)
Daniel Vetter Dec. 16, 2013, 7:57 p.m. UTC | #14
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 mbox

Patch

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