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@xxxxxxxxx>
> > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> > Cc: Hazwan Arif Mazlan <hazwan.arif.mazlan@xxxxxxxxx>
> > Cc: Jin Chung Teng <jin.chung.teng@xxxxxxxxx>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > ---
> > 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