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

[Kasireddy, Vivek]

Hi Frediano,

> > 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 ?
I think we can have the following situations:
- more than one GPU from different vendors:
In this case, user preference can be determined based on which Gstreamer
plugins are installed on the machine. In other words if the user wants to
use a specific GPU for this use-case, he needs to ensure that only the
plugins that work with that GPU are installed. 
- more than one GPU from same vendor:
AFAIK, in this case the same Gstreamer plugins would work with both
GPUs. Therefore, the onus is on Gstreamer to associate the current
"context" with the right GPU. I don't think there is anything else that
can be done at the Spice level for this case.

> Maybe a
> 
> bool spice_udev_has_intel_gpu(void);
I think doing it this way will prevent checking for this condition
in spice-gtk :
    if (vendor == GPU_VENDOR_INTEL ||
        vendor == GPU_VENDOR_UNKNOWN) {

> 
> 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
Ok, I'll incorporate your suggestions in v2.

> 
> > +#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?
Sure, will add it in v2.

Thanks,
Vivek

> 
> 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