[Mesa-dev] [PATCH 1/2] loader: refactor duplicated code into loader util lib
Rob Clark
robdclark at gmail.com
Fri Jan 10 05:14:19 PST 2014
On Thu, Jan 9, 2014 at 10:36 PM, Eric Anholt <eric at anholt.net> wrote:
> Rob Clark <robdclark at gmail.com> writes:
>
>> From: Rob Clark <robclark at 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 <robclark at 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 <robclark at 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);
>> +}
>> +
More information about the mesa-dev
mailing list