[PATCH] Improved autoconfig drivers matching
Emil Velikov
emil.l.velikov at gmail.com
Tue Jul 12 23:31:37 UTC 2016
On 23 July 2015 at 00:42, Karol Kosik <kkosik at nvidia.com> wrote:
> Implementation of new drivers matching algorithm. New approach
> doesn't add duplicate drivers and ease drivers matching phase.
>
> Signed-off-by: Karol Kosik <kkosik at nvidia.com>
> ---
> hw/xfree86/common/xf86AutoConfig.c | 124 +++++++++++++++++++----------------
> hw/xfree86/common/xf86MatchDrivers.h | 40 +++++++++++
> hw/xfree86/common/xf86pciBus.c | 52 ++++++---------
> hw/xfree86/common/xf86pciBus.h | 13 ++--
> hw/xfree86/common/xf86platformBus.c | 31 +++------
> hw/xfree86/common/xf86platformBus.h | 5 +-
> 6 files changed, 150 insertions(+), 115 deletions(-)
> create mode 100644 hw/xfree86/common/xf86MatchDrivers.h
>
> diff --git a/hw/xfree86/common/xf86AutoConfig.c b/hw/xfree86/common/xf86AutoConfig.c
> index 6b8d0eb..440434c 100644
> --- a/hw/xfree86/common/xf86AutoConfig.c
> +++ b/hw/xfree86/common/xf86AutoConfig.c
> @@ -37,6 +37,7 @@
> #include "xf86Parser.h"
> #include "xf86tokens.h"
> #include "xf86Config.h"
> +#include "xf86MatchDrivers.h"
> #include "xf86Priv.h"
> #include "xf86_OSlib.h"
> #include "xf86platformBus.h"
> @@ -89,7 +90,7 @@
> static const char **builtinConfig = NULL;
> static int builtinLines = 0;
>
> -static void listPossibleVideoDrivers(char *matches[], int nmatches);
> +static void listPossibleVideoDrivers(XF86MatchedDrivers *md);
>
> /*
> * A built-in config file is stored as an array of strings, with each string
> @@ -140,33 +141,58 @@ AppendToConfig(const char *s)
> AppendToList(s, &builtinConfig, &builtinLines);
> }
>
> +void
> +xf86AddMatchedDriver(XF86MatchedDrivers *md, const char *driver)
> +{
> + int j;
> + int nmatches = md->nmatches;
> +
> + for (j = 0; j < nmatches; ++j) {
> + if (xf86NameCmp(md->matches[j], driver) == 0) {
> + // Driver already in matched drivers
> + return;
> + }
> + }
> +
> + if (nmatches < MATCH_DRIVERS_LIMIT) {
> + md->matches[nmatches] = xnfstrdup(driver);
> + md->nmatches++;
> + }
> + else {
> + xf86Msg(X_WARNING, "Too many drivers registered, can't add %s\n", driver);
> + }
> +}
> +
> Bool
> xf86AutoConfig(void)
> {
> - char *deviceList[20];
> - char **p;
> + XF86MatchedDrivers md;
> + int i;
> const char **cp;
> char buf[1024];
> ConfigStatus ret;
>
> - listPossibleVideoDrivers(deviceList, 20);
> + listPossibleVideoDrivers(&md);
>
> - for (p = deviceList; *p; p++) {
> - snprintf(buf, sizeof(buf), BUILTIN_DEVICE_SECTION, *p, 0, *p);
> + for (i = 0; i < md.nmatches; i++) {
> + snprintf(buf, sizeof(buf), BUILTIN_DEVICE_SECTION,
> + md.matches[i], 0, md.matches[i]);
> AppendToConfig(buf);
> - snprintf(buf, sizeof(buf), BUILTIN_SCREEN_SECTION, *p, 0, *p, 0);
> + snprintf(buf, sizeof(buf), BUILTIN_SCREEN_SECTION,
> + md.matches[i], 0, md.matches[i], 0);
> AppendToConfig(buf);
> }
>
> AppendToConfig(BUILTIN_LAYOUT_SECTION_PRE);
> - for (p = deviceList; *p; p++) {
> - snprintf(buf, sizeof(buf), BUILTIN_LAYOUT_SCREEN_LINE, *p, 0);
> + for (i = 0; i < md.nmatches; i++) {
> + snprintf(buf, sizeof(buf), BUILTIN_LAYOUT_SCREEN_LINE,
> + md.matches[i], 0);
> AppendToConfig(buf);
> }
> AppendToConfig(BUILTIN_LAYOUT_SECTION_POST);
>
> - for (p = deviceList; *p; p++) {
> - free(*p);
> + for (i = 0; i < md.nmatches; i++) {
> + free(md.matches[i]);
> }
>
> xf86MsgVerb(X_DEFAULT, 0,
> @@ -190,22 +216,19 @@ xf86AutoConfig(void)
> }
>
> static void
> -listPossibleVideoDrivers(char *matches[], int nmatches)
> +listPossibleVideoDrivers(XF86MatchedDrivers *md)
> {
> int i;
>
> - for (i = 0; i < nmatches; i++) {
> - matches[i] = NULL;
> - }
> - i = 0;
> + md->nmatches = 0;
>
> #ifdef XSERVER_PLATFORM_BUS
> - i = xf86PlatformMatchDriver(matches, nmatches);
> + xf86PlatformMatchDriver(md);
> #endif
> #ifdef sun
> /* Check for driver type based on /dev/fb type and if valid, use
> it instead of PCI bus probe results */
> - if (xf86Info.consoleFd >= 0 && (i < (nmatches - 1))) {
> + if (xf86Info.consoleFd >= 0) {
> struct vis_identifier visid;
> const char *cp;
> int iret;
> @@ -231,7 +254,7 @@ listPossibleVideoDrivers(char *matches[], int nmatches)
>
> /* Special case from before the general case was set */
> if (strcmp(visid.name, "NVDAnvda") == 0) {
> - matches[i++] = xnfstrdup("nvidia");
> + xf86AddMatchedDriver(md, "nvidia");
> }
>
> /* General case - split into vendor name (initial all-caps
> @@ -241,55 +264,48 @@ listPossibleVideoDrivers(char *matches[], int nmatches)
> /* find end of all uppercase vendor section */
> }
> if ((cp != visid.name) && (*cp != '\0')) {
> - char *driverName = xnfstrdup(cp);
> char *vendorName = xnfstrdup(visid.name);
>
> vendorName[cp - visid.name] = '\0';
>
> - matches[i++] = vendorName;
> - matches[i++] = driverName;
> + xf86AddMatchedDriver(md, vendorName);
> + xf86AddMatchedDriver(md, cp);
> +
> + free(vendorName);
> }
> }
> }
> }
> #endif
> #ifdef __sparc__
> - if (i < (nmatches - 1))
> - {
> - char *sbusDriver = sparcDriverName();
> + char *sbusDriver = sparcDriverName();
>
> - if (sbusDriver)
> - matches[i++] = xnfstrdup(sbusDriver);
> - }
> + if (sbusDriver)
> + xf86AddMatchedDriver(md, sbusDriver);
> #endif
> #ifdef XSERVER_LIBPCIACCESS
> - if (i < (nmatches - 1))
> - i += xf86PciMatchDriver(&matches[i], nmatches - i);
> + xf86PciMatchDriver(md);
> #endif
>
> #if defined(__linux__)
> - matches[i++] = xnfstrdup("modesetting");
> + xf86AddMatchedDriver(md, "modesetting");
> #endif
>
> #if !defined(sun)
> /* Fallback to platform default frame buffer driver */
> - if (i < (nmatches - 1)) {
> #if !defined(__linux__) && defined(__sparc__)
> - matches[i++] = xnfstrdup("wsfb");
> + xf86AddMatchedDriver(md, "wsfb");
> #else
> - matches[i++] = xnfstrdup("fbdev");
> + xf86AddMatchedDriver(md, "fbdev");
> #endif
> - }
> #endif /* !sun */
>
> /* Fallback to platform default hardware */
> - if (i < (nmatches - 1)) {
> #if defined(__i386__) || defined(__amd64__) || defined(__hurd__)
> - matches[i++] = xnfstrdup("vesa");
> + xf86AddMatchedDriver(md, "vesa");
> #elif defined(__sparc__) && !defined(sun)
> - matches[i++] = xnfstrdup("sunffb");
> + xf86AddMatchedDriver(md, "sunffb");
> #endif
> - }
> }
>
> /* copy a screen section and enter the desired driver
> @@ -335,8 +351,8 @@ GDevPtr
> autoConfigDevice(GDevPtr preconf_device)
> {
> GDevPtr ptr = NULL;
> - char *matches[20]; /* If we have more than 20 drivers we're in trouble */
> - int num_matches = 0, num_screens = 0, i;
> + XF86MatchedDrivers md;
> + int num_screens = 0, i;
> screenLayoutPtr slp;
>
> if (!xf86configptr) {
> @@ -363,10 +379,10 @@ autoConfigDevice(GDevPtr preconf_device)
> }
> if (!ptr->driver) {
> /* get all possible video drivers and count them */
> - listPossibleVideoDrivers(matches, 20);
> - for (; matches[num_matches]; num_matches++) {
> + listPossibleVideoDrivers(&md);
> + for (i = 0; i < md.nmatches; i++) {
> xf86Msg(X_DEFAULT, "Matched %s as autoconfigured driver %d\n",
> - matches[num_matches], num_matches);
> + md.matches[i], i);
> }
>
> slp = xf86ConfigLayout.screens;
> @@ -376,12 +392,12 @@ autoConfigDevice(GDevPtr preconf_device)
> * minus one for the already existing first one
> * plus one for the terminating NULL */
> for (; slp[num_screens].screen; num_screens++);
> - xf86ConfigLayout.screens = xnfcalloc(num_screens + num_matches,
> + xf86ConfigLayout.screens = xnfcalloc(num_screens + md.nmatches,
> sizeof(screenLayoutRec));
> xf86ConfigLayout.screens[0] = slp[0];
>
> /* do the first match and set that for the original first screen */
> - ptr->driver = matches[0];
> + ptr->driver = md.matches[0];
> if (!xf86ConfigLayout.screens[0].screen->device) {
> xf86ConfigLayout.screens[0].screen->device = ptr;
> ptr->myScreenSection = xf86ConfigLayout.screens[0].screen;
> @@ -390,8 +406,8 @@ autoConfigDevice(GDevPtr preconf_device)
> /* for each other driver found, copy the first screen, insert it
> * into the list of screens and set the driver */
> i = 0;
> - while (i++ < num_matches) {
> - if (!copyScreen(slp[0].screen, ptr, i, matches[i]))
> + while (i++ < md.nmatches) {
> + if (!copyScreen(slp[0].screen, ptr, i, md.matches[i]))
> return NULL;
> }
>
> @@ -400,19 +416,17 @@ autoConfigDevice(GDevPtr preconf_device)
> *
> * TODO Handle rest of multiple screen sections */
> for (i = 1; i < num_screens; i++) {
> - xf86ConfigLayout.screens[i + num_matches] = slp[i];
> + xf86ConfigLayout.screens[i + md.nmatches] = slp[i];
> }
> - xf86ConfigLayout.screens[num_screens + num_matches - 1].screen =
> + xf86ConfigLayout.screens[num_screens + md.nmatches - 1].screen =
> NULL;
> free(slp);
> }
> else {
> /* layout does not have any screens, not much to do */
> - ptr->driver = matches[0];
> - for (i = 1; matches[i]; i++) {
> - if (matches[i] != matches[0]) {
> - free(matches[i]);
> - }
> + ptr->driver = md.matches[0];
> + for (i = 1; i < md.nmatches; i++) {
> + free(md.matches[i]);
> }
> }
> }
> diff --git a/hw/xfree86/common/xf86MatchDrivers.h b/hw/xfree86/common/xf86MatchDrivers.h
> new file mode 100644
> index 0000000..4663af4
> --- /dev/null
> +++ b/hw/xfree86/common/xf86MatchDrivers.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright © 2015 NVIDIA Corporation
> + *
> + * 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.
> + */
> +
> +#ifndef _xf86_match_drivers_h
> +#define _xf86_match_drivers_h
> +
> +#define MATCH_DRIVERS_LIMIT 20
> +
> +typedef struct _XF86MatchedDrivers {
> + char *matches[MATCH_DRIVERS_LIMIT];
> + int nmatches;
> +} XF86MatchedDrivers;
> +
> +/*
> + * prototypes
> + */
> +void xf86AddMatchedDriver(XF86MatchedDrivers *, const char *);
> +
> +#endif /* _xf86_match_drivers_h */
> +
> diff --git a/hw/xfree86/common/xf86pciBus.c b/hw/xfree86/common/xf86pciBus.c
> index 8158c2b..e81dc69 100644
> --- a/hw/xfree86/common/xf86pciBus.c
> +++ b/hw/xfree86/common/xf86pciBus.c
> @@ -1061,9 +1061,8 @@ xf86ConfigPciEntity(ScrnInfoPtr pScrn, int scrnFlag, int entityIndex,
> return pScrn;
> }
>
> -int
> -xf86VideoPtrToDriverList(struct pci_device *dev,
> - char *returnList[], int returnListMax)
> +void
> +xf86VideoPtrToDriverList(struct pci_device *dev, XF86MatchedDrivers *md)
> {
> int i;
>
> @@ -1266,10 +1265,9 @@ xf86VideoPtrToDriverList(struct pci_device *dev,
> default:
> break;
> }
> - for (i = 0; (i < returnListMax) && (driverList[i] != NULL); i++) {
> - returnList[i] = xnfstrdup(driverList[i]);
> + for (i = 0; driverList[i] != NULL; i++) {
> + xf86AddMatchedDriver(md, driverList[i]);
> }
> - return i; /* Number of entries added */
> }
>
> #ifdef __linux__
> @@ -1293,23 +1291,23 @@ xchomp(char *line)
> * don't export their PCI ID's properly. If distros don't end up using this
> * feature it can and should be removed because the symbol-based resolution
> * scheme should be the primary one */
> -int
> +void
> xf86MatchDriverFromFiles(uint16_t match_vendor, uint16_t match_chip,
> - char *matches[], int nmatches)
> + XF86MatchedDrivers *md)
> {
> DIR *idsdir;
> FILE *fp;
> struct dirent *direntry;
> - char *line = NULL;
> + char *line = NULL, *tmpMatch;
> size_t len;
> ssize_t read;
> char path_name[256], vendor_str[5], chip_str[5];
> uint16_t vendor, chip;
> - int i = 0, j;
> + int j;
>
> idsdir = opendir(PCI_TXT_IDS_PATH);
> if (!idsdir)
> - return 0;
> + return;
>
> xf86Msg(X_INFO,
> "Scanning %s directory for additional PCI ID's supported by the drivers\n",
> @@ -1360,10 +1358,10 @@ xf86MatchDriverFromFiles(uint16_t match_vendor, uint16_t match_chip,
> }
> }
> if (vendor == match_vendor && chip == match_chip) {
> - matches[i] =
> + tmpMatch =
> (char *) malloc(sizeof(char) *
> strlen(direntry->d_name) - 3);
> - if (!matches[i]) {
> + if (!tmpMatch) {
> xf86Msg(X_ERROR,
> "Could not allocate space for the module name. Exiting.\n");
> goto end;
> @@ -1373,16 +1371,17 @@ xf86MatchDriverFromFiles(uint16_t match_vendor, uint16_t match_chip,
> * taking off anything after the first '.' */
> for (j = 0; j < (strlen(direntry->d_name) - 3); j++) {
> if (direntry->d_name[j] == '.') {
> - matches[i][j] = '\0';
> + tmpMatch[j] = '\0';
> break;
> }
> else {
> - matches[i][j] = direntry->d_name[j];
> + tmpMatch[j] = direntry->d_name[j];
> }
> }
> + xf86AddMatchedDriver(md, tmpMatch);
> xf86Msg(X_INFO, "Matched %s from file name %s\n",
> - matches[i], direntry->d_name);
> - i++;
> + tmpMatch, direntry->d_name);
> + free(tmpMatch);
> }
> }
> else {
> @@ -1396,18 +1395,12 @@ xf86MatchDriverFromFiles(uint16_t match_vendor, uint16_t match_chip,
> end:
> free(line);
> closedir(idsdir);
> - return i;
> }
> #endif /* __linux__ */
>
> -/**
> - * @return The numbers of found devices that match with the current system
> - * drivers.
> - */
> -int
> -xf86PciMatchDriver(char *matches[], int nmatches)
> +void
> +xf86PciMatchDriver(XF86MatchedDrivers *md)
> {
> - int i = 0;
> struct pci_device *info = NULL;
> struct pci_device_iterator *iter;
>
> @@ -1422,15 +1415,12 @@ xf86PciMatchDriver(char *matches[], int nmatches)
> pci_iterator_destroy(iter);
> #ifdef __linux__
> if (info)
> - i += xf86MatchDriverFromFiles(info->vendor_id, info->device_id,
> - matches, nmatches);
> + xf86MatchDriverFromFiles(info->vendor_id, info->device_id, md);
> #endif
>
> - if ((info != NULL) && (i < nmatches)) {
> - i += xf86VideoPtrToDriverList(info, &(matches[i]), nmatches - i);
> + if (info != NULL) {
> + xf86VideoPtrToDriverList(info, md);
> }
> -
> - return i;
> }
>
> Bool
> diff --git a/hw/xfree86/common/xf86pciBus.h b/hw/xfree86/common/xf86pciBus.h
> index 45b5a0f..14ae976 100644
> --- a/hw/xfree86/common/xf86pciBus.h
> +++ b/hw/xfree86/common/xf86pciBus.h
> @@ -33,11 +33,13 @@
> #ifndef _XF86_PCI_BUS_H
> #define _XF86_PCI_BUS_H
>
> +#include "xf86MatchDrivers.h"
> +
> void xf86PciProbe(void);
> Bool xf86PciAddMatchingDev(DriverPtr drvp);
> Bool xf86PciProbeDev(DriverPtr drvp);
> void xf86PciIsolateDevice(const char *argument);
> -int xf86PciMatchDriver(char *matches[], int nmatches);
> +void xf86PciMatchDriver(XF86MatchedDrivers *md);
> Bool xf86PciConfigure(void *busData, struct pci_device *pDev);
> void xf86PciConfigureNewDev(void *busData, struct pci_device *pVideo,
> GDevRec * GDev, int *chipset);
> @@ -47,10 +49,9 @@ void xf86PciConfigureNewDev(void *busData, struct pci_device *pVideo,
> ((x)->func == (y)->func) && \
> ((x)->dev == (y)->dev))
>
> -int
> +void
> xf86MatchDriverFromFiles(uint16_t match_vendor, uint16_t match_chip,
> - char *matches[], int nmatches);
> -int
> -xf86VideoPtrToDriverList(struct pci_device *dev,
> - char *returnList[], int returnListMax);
> + XF86MatchedDrivers *md);
> +void
> +xf86VideoPtrToDriverList(struct pci_device *dev, XF86MatchedDrivers *md);
> #endif /* _XF86_PCI_BUS_H */
> diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c
> index f1e9423..c414ff0 100644
> --- a/hw/xfree86/common/xf86platformBus.c
> +++ b/hw/xfree86/common/xf86platformBus.c
> @@ -212,14 +212,10 @@ OutputClassMatches(const XF86ConfOutputClassPtr oclass, int index)
> return TRUE;
> }
>
> -static int
> -xf86OutputClassDriverList(int index, char *matches[], int nmatches)
> +static void
> +xf86OutputClassDriverList(int index, XF86MatchedDrivers *md)
> {
> XF86ConfOutputClassPtr cl;
> - int i = 0;
> -
> - if (nmatches == 0)
> - return 0;
>
> for (cl = xf86configptr->conf_outputclass_lst; cl; cl = cl->list.next) {
> if (OutputClassMatches(cl, index)) {
> @@ -229,24 +225,19 @@ xf86OutputClassDriverList(int index, char *matches[], int nmatches)
> cl->identifier, path);
> xf86Msg(X_NONE, "\tloading driver: %s\n", cl->driver);
>
> - matches[i++] = xstrdup(cl->driver);
> + xf86AddMatchedDriver(md, cl->driver);
> }
> -
> - if (i >= nmatches)
> - break;
> }
> -
> - return i;
> }
>
> /**
> * @return The numbers of found devices that match with the current system
> * drivers.
> */
> -int
> -xf86PlatformMatchDriver(char *matches[], int nmatches)
> +void
> +xf86PlatformMatchDriver(XF86MatchedDrivers *md)
> {
> - int i, j = 0;
> + int i;
> struct pci_device *info = NULL;
> int pass = 0;
>
> @@ -258,21 +249,19 @@ xf86PlatformMatchDriver(char *matches[], int nmatches)
> else if (!xf86IsPrimaryPlatform(&xf86_platform_devices[i]) && (pass == 0))
> continue;
>
> - j += xf86OutputClassDriverList(i, &matches[j], nmatches - j);
> + xf86OutputClassDriverList(i, md);
>
> info = xf86_platform_devices[i].pdev;
> #ifdef __linux__
> if (info)
> - j += xf86MatchDriverFromFiles(info->vendor_id, info->device_id,
> - &matches[j], nmatches - j);
> + xf86MatchDriverFromFiles(info->vendor_id, info->device_id, md);
> #endif
>
> - if ((info != NULL) && (j < nmatches)) {
> - j += xf86VideoPtrToDriverList(info, &(matches[j]), nmatches - j);
> + if (info != NULL) {
> + xf86VideoPtrToDriverList(info, md);
> }
> }
> }
> - return j;
> }
>
> int
> diff --git a/hw/xfree86/common/xf86platformBus.h b/hw/xfree86/common/xf86platformBus.h
> index a7335b9..0f0184f 100644
> --- a/hw/xfree86/common/xf86platformBus.h
> +++ b/hw/xfree86/common/xf86platformBus.h
> @@ -25,6 +25,7 @@
> #define XF86_PLATFORM_BUS_H
>
> #include "hotplug.h"
> +#include "xf86MatchDrivers.h"
>
Since xf86platformBus.h is part of the SDK, If we do this, then the
new header must become one as well (should be listed in sdk_HEADERS).
Alternatively we can forward declare XF86MatchedDrivers and include
the header in EXTRA_DIST. Not sure if the latter is a good idea
though, since the actual ABI will be undefined/private.
Or better yet, neither of the two exported symbols
(xf86PlatformDeviceCheckBusID, xf86PlatformMatchDriver) is used and
imho we can remove them. Seems that the header is used solely for the
ODEV management, which isn't platform devices specific and one can
just move those parts into a separate header and use _it_ in the SDK ?
But all that (everything but the sdk_HEADERS/EXTRA_DIST fix) is added
bogus, which shouldn't stop the patch from landing.
-Emil
More information about the xorg-devel
mailing list