[Spice-devel] [PATCH] common: Add a udev helper to identify GPU Vendor

[Frediano Ziglio]

Il giorno ven 15 set 2023 alle ore 01:33 Vivek Kasireddy
<vivek.kasireddy at intel.com> ha scritto:
>
> Given that libudev is widely available on many distros, we can
> use the relevant APIs to iterate over all the PCI devices on
> any given system to identify if a GPU is available by looking
> at the driver name associated with it.
>
> This capability (identifying GPU Vendor) is useful to determine
> whether to launch Gstreamer pipeline using h/w accelerated
> plugins. On systems where libudev is not available (Windows),

Nor MacOS I think.

> we'd have to make this determination based on the availability
> of the plugins in Gstreamer registry.
>
> Cc: Frediano Ziglio <freddy77 at gmail.com>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Dongwon Kim <dongwon.kim at intel.com>
> Cc: Hazwan Arif Mazlan <hazwan.arif.mazlan at intel.com>
> Cc: Jin Chung Teng <jin.chung.teng at intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> ---
>  common/meson.build |  2 ++
>  common/udev.c      | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  common/udev.h      | 12 ++++++++++
>  meson.build        |  7 ++++++
>  4 files changed, 81 insertions(+)
>  create mode 100644 common/udev.c
>  create mode 100644 common/udev.h
>
> diff --git a/common/meson.build b/common/meson.build
> index 09e3ea7..63bd2db 100644
> --- a/common/meson.build
> +++ b/common/meson.build
> @@ -39,6 +39,8 @@ spice_common_sources = [
>    'snd_codec.h',
>    'utils.c',
>    'utils.h',
> +  'udev.c',
> +  'udev.h',
>    'verify.h',
>    'recorder.h'
>  ]
> diff --git a/common/udev.c b/common/udev.c
> new file mode 100644
> index 0000000..995a37e
> --- /dev/null
> +++ b/common/udev.c
> @@ -0,0 +1,60 @@

Can you add license comments?

> +#include <config.h>
> +
> +#ifdef HAVE_UDEV
> +#include <libudev.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include "udev.h"
> +
> +#define INTEL_GFX_DRV_NAME "i915"
> +
> +static bool is_intel_gpu(const char *drv_name)
> +{
> +    return !strcmp(drv_name, INTEL_GFX_DRV_NAME);
> +}
> +
> +GpuVendor spice_udev_detect_gpu()
> +{
> +    struct udev *udev;
> +    struct udev_device *udev_dev;
> +    struct udev_enumerate *udev_enum;
> +    struct udev_list_entry *entry, *devices;
> +    const char *path, *driver;
> +    GpuVendor vendor = GPU_VENDOR_OTHER;
> +
> +    udev = udev_new();
> +    if (!udev) {
> +        return vendor;
> +    }
> +
> +    udev_enum = udev_enumerate_new(udev);
> +    if (udev_enum) {
> +        udev_enumerate_add_match_subsystem(udev_enum, "pci");
> +        udev_enumerate_scan_devices(udev_enum);
> +        devices = udev_enumerate_get_list_entry(udev_enum);
> +
> +        udev_list_entry_foreach(entry, devices) {
> +            path = udev_list_entry_get_name(entry);
> +            udev_dev = udev_device_new_from_syspath(udev, path);
> +
> +            driver = udev_device_get_driver(udev_dev);
> +            if (driver && is_intel_gpu(driver)) {
> +                vendor = GPU_VENDOR_INTEL;
> +                udev_device_unref(udev_dev);
> +                break;
> +            }
> +            udev_device_unref(udev_dev);
> +        }
> +        udev_enumerate_unref(udev_enum);
> +    }
> +    udev_unref(udev);
> +
> +    return vendor;
> +}
> +#else
> +GpuVendor spice_udev_detect_gpu()
> +{
> +    return GPU_VENDOR_UNKNOWN;
> +}
> +#endif
> +
> diff --git a/common/udev.h b/common/udev.h
> new file mode 100644
> index 0000000..f1ba0ec
> --- /dev/null
> +++ b/common/udev.h
> @@ -0,0 +1,12 @@
> +#ifndef H_SPICE_COMMON_UDEV
> +#define H_SPICE_COMMON_UDEV
> +
> +typedef enum {
> +    GPU_VENDOR_UNKNOWN,
> +    GPU_VENDOR_OTHER,
> +    GPU_VENDOR_INTEL,
> +} GpuVendor;
> +
> +GpuVendor spice_udev_detect_gpu();
> +

What if the machine has more than one GPU ?
Maybe a

bool spice_udev_has_intel_gpu(void);

function instead?

Note that in C something like void funcname() is K&R style, something
you don't want, you need to specify the argument list and types (in
this case "void").

Recently we use "#pragma once" for new headers but it's your call.

I would use something like

/* SPDX-License-Identifier: BSD-3-Clause */

#pragma once

#include <stdbool.h>
#include <spice/macros.h>

SPICE_BEGIN_DECLS

bool spice_udev_has_intel_gpu(void);

SPICE_END_DECLS

> +#endif
> diff --git a/meson.build b/meson.build
> index eeccecd..cafbf03 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -147,6 +147,13 @@ if smartcard_dep.found()
>    spice_common_config_data.set('USE_SMARTCARD', '1')
>  endif
>
> +#udev
> +udev_dep = dependency('libudev')
> +if udev_dep.found()
> +  spice_common_deps += udev_dep
> +  spice_common_config_data.set('HAVE_UDEV', '1')
> +endif
> +
>  #
>  # global C defines
>  #

Can you add autoconf support?

diff --git a/configure.ac b/configure.ac
index 0d4c22b..4fc4263 100644
--- a/configure.ac
+++ b/configure.ac
@@ -65,6 +65,8 @@ SPICE_COMMON_LIBS='$(PIXMAN_LIBS) $(GLIB2_LIBS)
$(OPUS_LIBS) $(OPENSSL_LIBS)'
 AC_SUBST(SPICE_COMMON_CFLAGS)
 AC_SUBST(SPICE_COMMON_LIBS)

+PKG_CHECK_MODULES([UDEV], [libudev], AC_DEFINE([HAVE_UDEV], [1],
[Define if we have libudev support]))
+
 # The End!
 AC_CONFIG_FILES([
   Makefile

Frediano