[Mesa-dev] [PATCH 02/16] loader: slim down loader_get_pci_id_for_fd implementation(s)

Jonathan Gray jsg at jsg.id.au
Sat Oct 15 02:05:42 UTC 2016


On Tue, Oct 11, 2016 at 07:31:46PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Currently mesa has three code paths in the loader - libudev, manual
> sysfs and drm ioctl one.
> 
> Considering the issues we had with libudev - strip those down in favour
> of the libdrm drm device API. The latter can be implemented in any way
> depending on the platform and can be reused by others.
> 
> Cc: Jonathan Gray <jsg at jsg.id.au>
> Cc: Jean-S??bastien P??dron <dumbbell at FreeBSD.org>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Jonathan, Jean-S??bastien I believe I've prodded you guys for a *BSD
> implementation of the drm*Device API a while back. With this commit
> you'll be 'forced' to prep some ;-)

It has been a while since I looked into that.  The design seemed to
assume that the user running code that called into libdrm had the
ability to enumerate pci busses.

On OpenBSD /dev/pci* is only read/writable by root.  /dev/drm* is
chowned after a user logs into a console.

We don't use filesystems for communicating with the kernel like linux
does so ioctls are really the best fit.  The loader parts used at the
moment use drm driver specific ioctls, hopefully a generic version of
those that can return the vid/pid, subids and function id numbers would
cover most of it.

> 
> About the implementation itself feel free to use libudev/equivalent (we
> cannot do so on Linux, since due to fun interactions with Steam's one)
> or any other means available on your platform. Fwiw I would strongly
> suggest _against_ adding DRM specific ioctls just for this purpose but
> at the end of the day it's your code/userbase.
> 
> On the FreeBSD/DragonFly side this means that you no londer require a
> handful of patches for mesa, which is always a plus.
> ---
>  src/loader/loader.c | 172 +++++-----------------------------------------------
>  1 file changed, 16 insertions(+), 156 deletions(-)
> 
> diff --git a/src/loader/loader.c b/src/loader/loader.c
> index 3e60e4c..41ea016 100644
> --- a/src/loader/loader.c
> +++ b/src/loader/loader.c
> @@ -204,57 +204,6 @@ udev_device_new_from_fd(struct udev *udev, int fd)
>     return device;
>  }
>  
> -static int
> -libudev_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
> -{
> -   struct udev *udev = NULL;
> -   struct udev_device *device = NULL, *parent;
> -   const char *pci_id;
> -   UDEV_SYMBOL(struct udev *, udev_new, (void));
> -   UDEV_SYMBOL(struct udev_device *, udev_device_get_parent,
> -               (struct udev_device *));
> -   UDEV_SYMBOL(const char *, udev_device_get_property_value,
> -               (struct udev_device *, const char *));
> -   UDEV_SYMBOL(struct udev_device *, udev_device_unref,
> -               (struct udev_device *));
> -   UDEV_SYMBOL(struct udev *, udev_unref, (struct udev *));
> -
> -   *chip_id = -1;
> -
> -   if (dlsym_failed)
> -      return 0;
> -
> -   udev = udev_new();
> -   device = udev_device_new_from_fd(udev, fd);
> -   if (!device)
> -      goto out;
> -
> -   parent = udev_device_get_parent(device);
> -   if (parent == NULL) {
> -      log_(_LOADER_WARNING, "MESA-LOADER: could not get parent device\n");
> -      goto out;
> -   }
> -
> -   pci_id = udev_device_get_property_value(parent, "PCI_ID");
> -   if (pci_id == NULL) {
> -      log_(_LOADER_INFO, "MESA-LOADER: no PCI ID\n");
> -      *chip_id = -1;
> -      goto out;
> -   } else if (sscanf(pci_id, "%x:%x", vendor_id, chip_id) != 2) {
> -      log_(_LOADER_WARNING, "MESA-LOADER: malformed PCI ID\n");
> -      *chip_id = -1;
> -      goto out;
> -   }
> -
> -out:
> -   if (device)
> -      udev_device_unref(device);
> -   if (udev)
> -      udev_unref(udev);
> -
> -   return (*chip_id >= 0);
> -}
> -
>  static char *
>  get_render_node_from_id_path_tag(struct udev *udev,
>                                   char *id_path_tag,
> @@ -471,113 +420,32 @@ dev_node_from_fd(int fd, unsigned int *maj, unsigned int *min)
>  }
>  #endif
>  
> -#if defined(HAVE_SYSFS)
> -static int
> -sysfs_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
> -{
> -   unsigned int maj, min;
> -   FILE *f;
> -   char buf[0x40];
> -
> -   if (dev_node_from_fd(fd, &maj, &min) < 0) {
> -      *chip_id = -1;
> -      return 0;
> -   }
> -
> -   snprintf(buf, sizeof(buf), "/sys/dev/char/%d:%d/device/vendor", maj, min);
> -   if (!(f = fopen(buf, "r"))) {
> -      *chip_id = -1;
> -      return 0;
> -   }
> -   if (fscanf(f, "%x", vendor_id) != 1) {
> -      *chip_id = -1;
> -      fclose(f);
> -      return 0;
> -   }
> -   fclose(f);
> -   snprintf(buf, sizeof(buf), "/sys/dev/char/%d:%d/device/device", maj, min);
> -   if (!(f = fopen(buf, "r"))) {
> -      *chip_id = -1;
> -      return 0;
> -   }
> -   if (fscanf(f, "%x", chip_id) != 1) {
> -      *chip_id = -1;
> -      fclose(f);
> -      return 0;
> -   }
> -   fclose(f);
> -   return 1;
> -}
> -#endif
> -
>  #if defined(HAVE_LIBDRM)
> -/* for i915 */
> -#include <i915_drm.h>
> -/* for radeon */
> -#include <radeon_drm.h>
>  
>  static int
>  drm_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
>  {
> -   drmVersionPtr version;
> -
> -   *chip_id = -1;
> -
> -   version = drmGetVersion(fd);
> -   if (!version) {
> -      log_(_LOADER_WARNING, "MESA-LOADER: invalid drm fd\n");
> -      return 0;
> -   }
> -   if (!version->name) {
> -      log_(_LOADER_WARNING, "MESA-LOADER: unable to determine the driver name\n");
> -      drmFreeVersion(version);
> -      return 0;
> -   }
> -
> -   if (strcmp(version->name, "i915") == 0) {
> -      struct drm_i915_getparam gp;
> -      int ret;
> -
> -      *vendor_id = 0x8086;
> -
> -      memset(&gp, 0, sizeof(gp));
> -      gp.param = I915_PARAM_CHIPSET_ID;
> -      gp.value = chip_id;
> -      ret = drmCommandWriteRead(fd, DRM_I915_GETPARAM, &gp, sizeof(gp));
> -      if (ret) {
> -         log_(_LOADER_WARNING, "MESA-LOADER: failed to get param for i915\n");
> -	 *chip_id = -1;
> +   drmDevicePtr device;
> +   int ret;
> +
> +   if (drmGetDevice(fd, &device) == 0) {
> +      if (device->bustype == DRM_BUS_PCI) {
> +         *vendor_id = device->deviceinfo.pci->vendor_id;
> +         *chip_id = device->deviceinfo.pci->device_id;
> +         ret = 1;
>        }
> -   }
> -   else if (strcmp(version->name, "radeon") == 0) {
> -      struct drm_radeon_info info;
> -      int ret;
> -
> -      *vendor_id = 0x1002;
> -
> -      memset(&info, 0, sizeof(info));
> -      info.request = RADEON_INFO_DEVICE_ID;
> -      info.value = (unsigned long) chip_id;
> -      ret = drmCommandWriteRead(fd, DRM_RADEON_INFO, &info, sizeof(info));
> -      if (ret) {
> -         log_(_LOADER_WARNING, "MESA-LOADER: failed to get info for radeon\n");
> -	 *chip_id = -1;
> +      else {
> +         log_(_LOADER_WARNING, "MESA-LOADER: device is not located on the PCI bus\n");
> +         ret = 0;
>        }
> +      drmFreeDevice(&device);
>     }
> -   else if (strcmp(version->name, "nouveau") == 0) {
> -      *vendor_id = 0x10de;
> -      /* not used */
> -      *chip_id = 0;
> -   }
> -   else if (strcmp(version->name, "vmwgfx") == 0) {
> -      *vendor_id = 0x15ad;
> -      /* assume SVGA II */
> -      *chip_id = 0x0405;
> +   else {
> +      log_(_LOADER_WARNING, "MESA-LOADER: device is not located on the PCI bus\n");
> +      ret = 0;
>     }
>  
> -   drmFreeVersion(version);
> -
> -   return (*chip_id >= 0);
> +   return ret;
>  }
>  #endif
>  
> @@ -585,14 +453,6 @@ drm_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
>  int
>  loader_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id)
>  {
> -#if HAVE_LIBUDEV
> -   if (libudev_get_pci_id_for_fd(fd, vendor_id, chip_id))
> -      return 1;
> -#endif
> -#if HAVE_SYSFS
> -   if (sysfs_get_pci_id_for_fd(fd, vendor_id, chip_id))
> -      return 1;
> -#endif
>  #if HAVE_LIBDRM
>     if (drm_get_pci_id_for_fd(fd, vendor_id, chip_id))
>        return 1;
> -- 
> 2.10.0
> 


More information about the mesa-dev mailing list