On Thu, Jan 9, 2014 at 10:36 PM, Eric Anholt <e...@anholt.net> wrote: > Rob Clark <robdcl...@gmail.com> writes: > >> From: Rob Clark <robcl...@freedesktop.org> >> >> All the various window system integration layers duplicate roughly the >> same code for figuring out device and driver name, pci-id's, etc. Which >> is sad. So extract it out into a loader util lib. > > Thanks for tackling this. It had been (low) on my list for a while.
cool, thanks for having a look >> static int >> droid_open_device(void) >> { >> @@ -773,7 +672,7 @@ dri2_initialize_android(_EGLDriver *drv, _EGLDisplay >> *dpy) >> goto cleanup_display; >> } > > forgot loader_set_logger here. Do we want to just move that to > egl_dri2.c instead of each platform_*? fwiw, default logger (if you don't call loader_set_logger()) is just printf. So in theory the ones that don't call loader_set_logger() either had no logging before or were just using printf. Hmm.. but that probably shouldn't be the case for the 'droid loader, so maybe I screwed that up. (disclaimer: the android loader in particular, I have no idea how to build/test.. so this one probably is the most likely one that I would have screwed up :-P) >> - dri2_dpy->driver_name = (char *) droid_get_driver_name(dri2_dpy->fd); >> + dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd, 0); >> if (dri2_dpy->driver_name == NULL) { >> err = "DRI2: failed to get driver name"; >> goto cleanup_device; > > >> diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader.h >> b/src/gallium/auxiliary/pipe-loader/pipe_loader.h >> index 444bdf1..e915c63 100644 >> --- a/src/gallium/auxiliary/pipe-loader/pipe_loader.h >> +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader.h >> @@ -44,6 +44,7 @@ struct pipe_screen; >> enum pipe_loader_device_type { >> PIPE_LOADER_DEVICE_SOFTWARE, >> PIPE_LOADER_DEVICE_PCI, >> + PIPE_LOADER_DEVICE_PLATFORM, >> NUM_PIPE_LOADER_DEVICE_TYPES >> }; >> > >> diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c >> b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c >> index 927fb24..fda0ab1 100644 >> --- a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c >> +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c >> @@ -190,17 +117,22 @@ boolean >> pipe_loader_drm_probe_fd(struct pipe_loader_device **dev, int fd) >> { >> struct pipe_loader_drm_device *ddev = >> CALLOC_STRUCT(pipe_loader_drm_device); >> - >> - ddev->base.type = PIPE_LOADER_DEVICE_PCI; >> + int vendor_id, chip_id; >> + >> + if (loader_get_pci_id_for_fd(fd, &vendor_id, &chip_id)) { >> + ddev->base.type = PIPE_LOADER_DEVICE_PCI; >> + ddev->base.u.pci.vendor_id = vendor_id; >> + ddev->base.u.pci.chip_id = chip_id; >> + } else { >> + ddev->base.type = PIPE_LOADER_DEVICE_PLATFORM; >> + } >> ddev->base.ops = &pipe_loader_drm_ops; >> ddev->fd = fd; >> >> pipe_loader_drm_x_auth(fd); >> >> - if (!find_drm_pci_id(ddev)) >> - goto fail; >> - >> - if (!find_drm_driver_name(ddev)) >> + ddev->base.driver_name = loader_get_driver_for_fd(fd, _LOADER_GALLIUM); >> + if (!ddev->base.driver_name) >> goto fail; >> >> *dev = &ddev->base; >> diff --git a/src/gallium/state_trackers/clover/core/device.cpp >> b/src/gallium/state_trackers/clover/core/device.cpp >> index e5e429a..76a49d0 100644 >> --- a/src/gallium/state_trackers/clover/core/device.cpp >> +++ b/src/gallium/state_trackers/clover/core/device.cpp >> @@ -63,6 +63,7 @@ device::type() const { >> case PIPE_LOADER_DEVICE_SOFTWARE: >> return CL_DEVICE_TYPE_CPU; >> case PIPE_LOADER_DEVICE_PCI: >> + case PIPE_LOADER_DEVICE_PLATFORM: >> return CL_DEVICE_TYPE_GPU; >> default: >> assert(0); >> @@ -74,6 +75,7 @@ cl_uint >> device::vendor_id() const { >> switch (ldev->type) { >> case PIPE_LOADER_DEVICE_SOFTWARE: >> + case PIPE_LOADER_DEVICE_PLATFORM: >> return 0; >> case PIPE_LOADER_DEVICE_PCI: >> return ldev->u.pci.vendor_id; > > These hunks look unrelated to the refactor and should be in a separate > commit enabling non-pci devices. I think I ended up including these in the first patch just because I needed some value to use in pipe_loader_drm_probe_fd() if loader_get_pci_id_for_fd() failed. I suppose what I could do is move the addition of new enum value into a patch that comes before this one. >> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c >> index b4b97ac..c13930c 100644 >> --- a/src/gbm/backends/dri/gbm_dri.c >> +++ b/src/gbm/backends/dri/gbm_dri.c >> @@ -44,6 +44,7 @@ >> #include "gbm_driint.h" >> >> #include "gbmint.h" >> +#include "loader.h" >> >> /* For importing wl_buffer */ >> #if HAVE_WAYLAND_PLATFORM >> @@ -270,7 +271,7 @@ dri_screen_create(struct gbm_dri_device *dri) >> const __DRIextension **extensions; >> int ret = 0; >> >> - dri->base.driver_name = dri_fd_get_driver_name(dri->base.base.fd); >> + dri->base.driver_name = loader_get_driver_for_fd(dri->base.base.fd, 0); >> if (dri->base.driver_name == NULL) >> return -1; > > Another missing set_logger. > >> diff --git a/src/loader/loader.c b/src/loader/loader.c >> new file mode 100644 >> index 0000000..3e69a59 >> --- /dev/null >> +++ b/src/loader/loader.c >> @@ -0,0 +1,264 @@ >> +/* >> + * Copyright (C) 2013 Rob Clark <robcl...@freedesktop.org> >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including the next >> + * paragraph) shall be included in all copies or substantial portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >> IN THE >> + * SOFTWARE. >> + * >> + * Authors: >> + * Rob Clark <robcl...@freedesktop.org> >> + */ >> + >> +#include <stdarg.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include "loader.h" >> + >> +#include <xf86drm.h> >> + >> +#define __IS_LOADER >> +#include "pci_ids/pci_id_driver_map.h" >> + >> +static void default_logger(int level, const char *fmt, ...) >> +{ >> + if (level >= _LOADER_WARNING) { >> + va_list args; >> + va_start(args, fmt); >> + vfprintf(stderr, fmt, args); >> + va_end(args); >> + fprintf(stderr, "\n"); >> + } >> +} >> + >> +static void (*log)(int level, const char *fmt, ...) = default_logger; >> + >> +#ifdef HAVE_LIBUDEV >> +#include <libudev.h> >> + >> +static inline struct udev_device * >> +udev_device_new_from_fd(struct udev *udev, int fd) >> +{ >> + struct udev_device *device; >> + struct stat buf; >> + >> + if (fstat(fd, &buf) < 0) { >> + log(_LOADER_WARNING, "EGL-DRI2: failed to stat fd %d", fd); >> + return NULL; >> + } >> + >> + device = udev_device_new_from_devnum(udev, 'c', buf.st_rdev); >> + if (device == NULL) { >> + log(_LOADER_WARNING, >> + "EGL-DRI2: could not create udev device for fd %d", fd); >> + return NULL; >> + } >> + >> + return device; >> +} >> + >> +int >> +loader_get_pci_id_for_fd(int fd, int *vendor_id, int *chip_id) >> +{ >> + struct udev *udev = NULL; >> + struct udev_device *device = NULL, *parent; >> + struct stat buf; >> + const char *pci_id; >> + >> + *chip_id = -1; >> + >> + 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, "could not get parent device"); >> + goto out; >> + } >> + >> + pci_id = udev_device_get_property_value(parent, "PCI_ID"); >> + if (pci_id == NULL || >> + sscanf(pci_id, "%x:%x", vendor_id, chip_id) != 2) { >> + log(_LOADER_WARNING, "malformed or no PCI ID"); >> + *chip_id = -1; >> + goto out; >> + } >> + >> +out: >> + if (device) >> + udev_device_unref(device); >> + if (udev) >> + udev_unref(udev); >> + >> + return (*chip_id >= 0); >> +} >> + >> +#elif defined(PIPE_OS_ANDROID) && !defined(_EGL_NO_DRM) > > I don't see gallium includes being used, so I don't think this code > would ever be built? yeah, the PIPE_OS_ANDROID case has definitely not been even compile tested yet. Android builds are something where I could probably use some help, if someone does have a setup to build android and let me know what is broken. I suspect also some android makefiles somewhere or other need to be updated to build the new files. > But it should probably be dumped in favor of a rebase of keithp's "don't > use udev" patch anyway, so I'm fine with things as is. ok, I should probably look at that.. I don't think it was merged yet when I first started on this. BR, -R >> +/* for i915 */ >> +#include <i915_drm.h> >> +/* for radeon */ >> +#include <radeon_drm.h> >> + >> +int >> +loader_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, "invalid drm fd"); >> + return FALSE; >> + } >> + if (!version->name) { >> + log(_LOADER_WARNING, "unable to determine the driver name"); >> + drmFreeVersion(version); >> + return FALSE; >> + } >> + >> + 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, "failed to get param for i915"); >> + *chip_id = -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, "failed to get info for radeon"); >> + *chip_id = -1; >> + } >> + } >> + 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; >> + } >> + >> + drmFreeVersion(version); >> + >> + return (*chip_id >= 0); >> +} >> + _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev