[Mesa-stable] [PATCH v3] loader: add special logic to distinguish nouveau from nouveau_vieux

[Emil Velikov]

On 17/03/14 22:23, Ilia Mirkin wrote:
> There are a lot of different pci ids supported by nouveau, and more are
> added all the time. The relevant distinguisher between drivers is the
> chipset id.
> 
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: "10.1" <mesa-stable at lists.freedesktop.org>
> ---
> 
> Tested with additional prints thrown in to make sure that the right chipset id
> is detected, etc. Tested on NV96 and NV05 (both in the same machine, running
> on different displays).
> 
I absolutely love this approach, it will be very useful if/when other
drivers decide to more away from those explicit lists.

A couple trivial nits below

>  src/glx/dri2_glx.c                                 | 20 +++----
>  src/loader/Makefile.sources                        |  3 +-
>  src/loader/loader.c                                |  5 +-
>  src/loader/pci_id_driver_map.c                     | 66 ++++++++++++++++++++++
>  .../pci_ids => src/loader}/pci_id_driver_map.h     |  7 ++-
>  5 files changed, 88 insertions(+), 13 deletions(-)
>  create mode 100644 src/loader/pci_id_driver_map.c
>  rename {include/pci_ids => src/loader}/pci_id_driver_map.h (90%)
> 
> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
> index 5a960b0..b61422f 100644
> --- a/src/glx/dri2_glx.c
> +++ b/src/glx/dri2_glx.c
> @@ -1200,6 +1200,16 @@ dri2CreateScreen(int screen, struct glx_display * priv)
>        goto handle_error;
>     }
>  
> +   if (drmGetMagic(psc->fd, &magic)) {
> +      ErrorMessageF("failed to get magic\n");
> +      goto handle_error;
> +   }
> +
> +   if (!DRI2Authenticate(priv->dpy, RootWindow(priv->dpy, screen), magic)) {
> +      ErrorMessageF("failed to authenticate magic %d\n", magic);
> +      goto handle_error;
> +   }
> +
>     /* If Mesa knows about the appropriate driver for this fd, then trust it.
>      * Otherwise, default to the server's value.
>      */
> @@ -1231,16 +1241,6 @@ dri2CreateScreen(int screen, struct glx_display * priv)
>        goto handle_error;
>     }
>  
> -   if (drmGetMagic(psc->fd, &magic)) {
> -      ErrorMessageF("failed to get magic\n");
> -      goto handle_error;
> -   }
> -
> -   if (!DRI2Authenticate(priv->dpy, RootWindow(priv->dpy, screen), magic)) {
> -      ErrorMessageF("failed to authenticate magic %d\n", magic);
> -      goto handle_error;
> -   }
> -
>     if (psc->dri2->base.version >= 4) {
>        psc->driScreen =
>           psc->dri2->createNewScreen2(screen, psc->fd,
> diff --git a/src/loader/Makefile.sources b/src/loader/Makefile.sources
> index 51a64ea..1a1345f 100644
> --- a/src/loader/Makefile.sources
> +++ b/src/loader/Makefile.sources
> @@ -1,2 +1,3 @@
>  LOADER_C_FILES := \
> -	loader.c
> \ No newline at end of file
> +	loader.c \
> +	pci_id_driver_map.c
> diff --git a/src/loader/loader.c b/src/loader/loader.c
> index 811f8a2..e343f4a 100644
> --- a/src/loader/loader.c
> +++ b/src/loader/loader.c
> @@ -78,7 +78,7 @@
>  #endif
>  
>  #define __IS_LOADER
> -#include "pci_ids/pci_id_driver_map.h"
> +#include "pci_id_driver_map.h"
>  
>  static void default_logger(int level, const char *fmt, ...)
>  {
> @@ -352,6 +352,9 @@ loader_get_driver_for_fd(int fd, unsigned driver_types)
>        if (!(driver_types & driver_map[i].driver_types))
>           continue;
>  
> +      if (driver_map[i].predicate && !driver_map[i].predicate(fd))
> +         continue;
> +
>        if (driver_map[i].num_chips_ids == -1) {
>           driver = strdup(driver_map[i].driver);
>           goto out;
> diff --git a/src/loader/pci_id_driver_map.c b/src/loader/pci_id_driver_map.c
> new file mode 100644
> index 0000000..0eb8d13
> --- /dev/null
> +++ b/src/loader/pci_id_driver_map.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright 2014 Ilia Mirkin
> + *
> + * 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.
> + */
> +
> +#include <string.h>
I take it that the header was required for memset(), which is no longer
used/present.

> +#include <stdio.h>
> +
> +#ifndef __NOT_HAVE_DRM_H
> +
> +#include <xf86drm.h>
> +#include <libdrm/nouveau_drm.h>
Can you drop libdrm from the above so that it reads
#include <nouveau_drm.h>

The file is provided by libdrm, which dictates the location while the
Makefile makes use of LIBDRM_CFLAGS.

The nouveau code is rather inconsistent with those but it would be great
to start paving in the right direction.

With that changed, the patch is
Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

Thanks
-Emil

> +
> +int is_nouveau(int fd);
> +int is_nouveau_vieux(int fd);
> +
> +static int
> +nouveau_chipset(int fd)
> +{
> +   struct drm_nouveau_getparam gp = { NOUVEAU_GETPARAM_CHIPSET_ID, 0 };
> +   int ret;
> +
> +   ret = drmCommandWriteRead(fd, DRM_NOUVEAU_GETPARAM, &gp, sizeof(gp));
> +   if (ret)
> +      return -1;
> +
> +   return gp.value;
> +}
> +
> +int
> +is_nouveau(int fd)
> +{
> +   return nouveau_chipset(fd) >= 0x30;
> +}
> +
> +int
> +is_nouveau_vieux(int fd)
> +{
> +   int chipset = nouveau_chipset(fd);
> +   return chipset > 0 && chipset < 0x30;
> +}
> +
> +#else
> +
> +int is_nouveau(int fd) { return 0; }
> +int is_nouveau_vieux(int fd) { return 0; }
> +
> +#endif
> diff --git a/include/pci_ids/pci_id_driver_map.h b/src/loader/pci_id_driver_map.h
> similarity index 90%
> rename from include/pci_ids/pci_id_driver_map.h
> rename to src/loader/pci_id_driver_map.h
> index db9e07f..e367c76 100644
> --- a/include/pci_ids/pci_id_driver_map.h
> +++ b/src/loader/pci_id_driver_map.h
> @@ -59,12 +59,16 @@ static const int vmwgfx_chip_ids[] = {
>  #undef CHIPSET
>  };
>  
> +int is_nouveau(int fd);
> +int is_nouveau_vieux(int fd);
> +
>  static const struct {
>     int vendor_id;
>     const char *driver;
>     const int *chip_ids;
>     int num_chips_ids;
>     unsigned driver_types;
> +   int (*predicate)(int fd);
>  } driver_map[] = {
>     { 0x8086, "i915", i915_chip_ids, ARRAY_SIZE(i915_chip_ids), _LOADER_DRI | _LOADER_GALLIUM },
>     { 0x8086, "i965", i965_chip_ids, ARRAY_SIZE(i965_chip_ids), _LOADER_DRI | _LOADER_GALLIUM },
> @@ -73,7 +77,8 @@ static const struct {
>     { 0x1002, "r300", r300_chip_ids, ARRAY_SIZE(r300_chip_ids), _LOADER_GALLIUM },
>     { 0x1002, "r600", r600_chip_ids, ARRAY_SIZE(r600_chip_ids), _LOADER_GALLIUM },
>     { 0x1002, "radeonsi", radeonsi_chip_ids, ARRAY_SIZE(radeonsi_chip_ids), _LOADER_GALLIUM},
> -   { 0x10de, "nouveau", NULL, -1,  _LOADER_GALLIUM  },
> +   { 0x10de, "nouveau", NULL, -1,  _LOADER_GALLIUM, is_nouveau },
> +   { 0x10de, "nouveau_vieux", NULL, -1,  _LOADER_DRI, is_nouveau_vieux },
>     { 0x15ad, "vmwgfx", vmwgfx_chip_ids, ARRAY_SIZE(vmwgfx_chip_ids), _LOADER_GALLIUM },
>     { 0x0000, NULL, NULL, 0 },
>  };
>