[Spice-devel] [PATCH spice-streaming-agent 6/8] Interface + implementation of getting device display info

Frediano Ziglio fziglio at redhat.com
Fri Jan 11 15:18:35 UTC 2019


> On Fri, 2019-01-11 at 06:13 -0500, Frediano Ziglio wrote:
> > > 
> > > Adds an interface method to the FrameCapture class to get the device
> > > display info (device address and device display id) for each display of
> > > the graphics device that is captured.
> > > 
> > > Also adds functions to the API implementing this functionality for X11
> > > in variants with and without DRM (the non-DRM version is rather limited
> > > and may not work for more complex setups) as well as some helper
> > > functions to make it easier for plugins to implement this and avoid code
> > > duplication.
> > > 
> > > Implements the new interface method for the two built-in plugins
> > > (mjpeg-fallback and gst-plugin).
> > > 
> > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > ---
> > >  configure.ac                                  |   2 +
> > >  include/spice-streaming-agent/Makefile.am     |   2 +
> > >  .../spice-streaming-agent/display-info.hpp    |  52 +++
> > >  .../spice-streaming-agent/frame-capture.hpp   |  13 +
> > >  .../spice-streaming-agent/x-display-info.hpp  |  57 ++++
> > >  src/Makefile.am                               |   7 +
> > >  src/display-info.cpp                          | 100 ++++++
> > >  src/gst-plugin.cpp                            |  14 +
> > >  src/mjpeg-fallback.cpp                        |  17 +-
> > >  src/spice-streaming-agent.cpp                 |  13 +
> > >  src/unittests/Makefile.am                     |   6 +
> > >  src/utils.cpp                                 |  45 +++
> > >  src/utils.hpp                                 |   4 +
> > >  src/x-display-info.cpp                        | 301 ++++++++++++++++++
> > >  14 files changed, 631 insertions(+), 2 deletions(-)
> > >  create mode 100644 include/spice-streaming-agent/display-info.hpp
> > >  create mode 100644 include/spice-streaming-agent/x-display-info.hpp
> > >  create mode 100644 src/display-info.cpp
> > >  create mode 100644 src/utils.cpp
> > >  create mode 100644 src/x-display-info.cpp
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index e66e6d8..fd18efe 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -34,8 +34,10 @@ SPICE_PROTOCOL_MIN_VER=0.12.14
> > >  PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >=
> > >  $SPICE_PROTOCOL_MIN_VER])
> > >  AC_SUBST([SPICE_PROTOCOL_MIN_VER])
> > >  
> > > +PKG_CHECK_MODULES(DRM, libdrm)
> > >  PKG_CHECK_MODULES(X11, x11)
> > >  PKG_CHECK_MODULES(XFIXES, xfixes)
> > > +PKG_CHECK_MODULES(XRANDR, xrandr)
> > >  
> > >  PKG_CHECK_MODULES(JPEG, libjpeg, , [
> > >      AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > diff --git a/include/spice-streaming-agent/Makefile.am
> > > b/include/spice-streaming-agent/Makefile.am
> > > index bcd679b..1125fb4 100644
> > > --- a/include/spice-streaming-agent/Makefile.am
> > > +++ b/include/spice-streaming-agent/Makefile.am
> > > @@ -1,8 +1,10 @@
> > >  NULL =
> > >  public_includedir = $(includedir)/spice-streaming-agent
> > >  public_include_HEADERS = \
> > > +	display-info.hpp \
> > >  	error.hpp \
> > >  	frame-capture.hpp \
> > >  	plugin.hpp \
> > > +	x-display-info.hpp \
> > 
> > I would use "x11-" prefix instead of just "x-".
> 
> Ok.
> 
> > >  	$(NULL)
> > >  
> > > diff --git a/include/spice-streaming-agent/display-info.hpp
> > > b/include/spice-streaming-agent/display-info.hpp
> > > new file mode 100644
> > > index 0000000..f16212b
> > > --- /dev/null
> > > +++ b/include/spice-streaming-agent/display-info.hpp
> > > @@ -0,0 +1,52 @@
> > > +/* \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#ifndef SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
> > > +#define SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
> > > +
> > > +#include <vector>
> > > +#include <string>
> > > +
> > > +
> > > +namespace spice __attribute__ ((visibility ("default"))) {
> > > +namespace streaming_agent {
> > > +
> > > +/**
> > > + * Lists graphics cards listed in the DRM sybsystem in /sys/class/drm.
> > > + * Throws an instance of Error in case of an I/O error.
> > > + *
> > > + * @return a vector of paths of all graphics cards present in
> > > /sys/class/drm
> > > + */
> > > +std::vector<std::string> list_cards();
> > > +
> > > +/**
> > > + * Reads a single number in hex format from a file.
> > > + * Throws an instance of Error in case of an I/O or parsing error.
> > > + *
> > > + * @param path the path to the file
> > > + * @return the number parsed from the file
> > > + */
> > > +uint32_t read_hex_number_from_file(const std::string &path);
> > > +
> > > +/**
> > > + * Resolves any symlinks and then extracts the PCI path from the
> > > canonical
> > > path
> > > + * to a card. Returns the path in the following format:
> > > + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> > > + *
> > > + * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI
> > > bridges
> > > + * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
> > > + * graphics device. All of <DOMAIN>, <SLOT>, <FUNCTION> are hexadecimal
> > > numbers
> > > + * with the following number of digits:
> > > + *   <DOMAIN>: 4
> > > + *   <SLOT>: 2
> > > + *   <FUNCTION>: 1
> > > + *
> > > + * @param device_path the path to the card
> > > + * @return the device address
> > > + */
> > > +std::string get_device_address(const std::string &card_path);
> > > +
> > > +}} // namespace spice::streaming_agent
> > > +
> > > +#endif // SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
> > > diff --git a/include/spice-streaming-agent/frame-capture.hpp
> > > b/include/spice-streaming-agent/frame-capture.hpp
> > > index 51a2987..81415e5 100644
> > > --- a/include/spice-streaming-agent/frame-capture.hpp
> > > +++ b/include/spice-streaming-agent/frame-capture.hpp
> > > @@ -6,7 +6,11 @@
> > >   */
> > >  #ifndef SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
> > >  #define SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
> > > +
> > > +#include <cstdint>
> > >  #include <cstdio>
> > > +#include <string>
> > > +#include <vector>
> > >  
> > >  #include <spice/enums.h>
> > >  
> > > @@ -29,6 +33,13 @@ struct FrameInfo
> > >      bool stream_start;
> > >  };
> > >  
> > > +struct DeviceDisplayInfo
> > > +{
> > > +    uint32_t id;
> > > +    std::string device_address;
> > > +    uint32_t device_display_id;
> > > +};
> > > +
> > >  /*!
> > >   * Pure base class implementing the frame capture
> > >   */
> > > @@ -52,6 +63,8 @@ public:
> > >       * Get video codec used to encode last frame
> > >       */
> > >      virtual SpiceVideoCodecType VideoCodecType() const = 0;
> > > +
> > > +    virtual std::vector<DeviceDisplayInfo> get_device_display_info()
> > > const =
> > > 0;
> > >  protected:
> > >      FrameCapture() = default;
> > >      FrameCapture(const FrameCapture&) = delete;
> > > diff --git a/include/spice-streaming-agent/x-display-info.hpp
> > > b/include/spice-streaming-agent/x-display-info.hpp
> > > new file mode 100644
> > > index 0000000..a5da225
> > > --- /dev/null
> > > +++ b/include/spice-streaming-agent/x-display-info.hpp
> > > @@ -0,0 +1,57 @@
> > > +/* \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#ifndef SPICE_STREAMING_AGENT_X_DISPLAY_INFO_HPP
> > > +#define SPICE_STREAMING_AGENT_X_DISPLAY_INFO_HPP
> > > +
> > > +#include <spice-streaming-agent/frame-capture.hpp>
> > > +
> > > +#include <X11/Xlib.h>
> > > +
> > > +
> > > +namespace spice __attribute__ ((visibility ("default"))) {
> > > +namespace streaming_agent {
> > > +
> > > +/**
> > > + * Looks up device display info by listing the xrandr outputs of
> > > @display
> > > and
> > > + * comparing them to card output names found under the DRM subsystem in
> > > + * /sys/class/drm.
> > > + *
> > > + * Throws an Error in case of error or when the DRM outputs cannot be
> > > found.
> > > + *
> > > + * @param display the X display to find the info for
> > > + * @return a vector of DeviceDisplayInfo structs containing the
> > > information
> > > of
> > > + * all the outputs used by the display
> > > + */
> > > +std::vector<DeviceDisplayInfo> get_device_display_info_drm(Display
> > > *display);
> > > +
> > > +/**
> > > + * Attempts to get the device display info without using DRM. It looks
> > > up
> > > the
> > > + * first card in /sys/class/drm that doesn't have its outputs listed
> > > (assuming
> > > + * that if the card in use has DRM outputs, it would be found using the
> > > + * get_display_info function) and simply uses the xrandr output index as
> > > the
> > > + * device_display_id. This can obviously incorrectly match the device
> > > address
> > > + * and the device_display_ids of two unrelated devices.
> > > + *
> > > + * Unfortunately, without DRM, there is no way to match xrandr objects
> > > with
> > > the
> > > + * real device.
> > > + *
> > > + * @param display the X display to find the info for
> > > + * @return a vector of DeviceDisplayInfo structs containing the
> > > information
> > > of
> > > + * all the outputs used by the display
> > > + */
> > > +std::vector<DeviceDisplayInfo> get_device_display_info_no_drm(Display
> > > *display);
> > > +
> > > +/**
> > > + * Lists xrandr outputs and returns their names in a vector of strings.
> > > + *
> > > + * @param display the X display
> > > + * @param window the X window
> > 
> > Which window? Any or the root? What if the window spawn multiple screens?
> 
> Tbh. no idea, I haven't found documentation for the
> XRRGetScreenResources function. I'll assume it is the root window and
> put that in the doc.
> 

Not much sure either, I miss some X11 knowledge. Not sure why we need a window
too. But I'm fine with it, I would just state to pass the root one.

> > > + * @return A vector of xrandr output names
> > > + */
> > > +std::vector<std::string> get_xrandr_outputs(Display *display, Window
> > > window);
> > > +
> > > +}} // namespace spice::streaming_agent
> > > +
> > > +#endif // SPICE_STREAMING_AGENT_X_DISPLAY_INFO_HPP
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index bae3f9d..80a1767 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -16,9 +16,11 @@ AM_CPPFLAGS = \
> > >  	-DSPICE_STREAMING_AGENT_PROGRAM \
> > >  	-I$(top_srcdir)/include \
> > >  	-DPLUGINSDIR=\"$(pkglibdir)/plugins\" \
> > > +	$(DRM_CFLAGS) \
> > >  	$(SPICE_PROTOCOL_CFLAGS) \
> > >  	$(X11_CFLAGS) \
> > >  	$(XFIXES_CFLAGS) \
> > > +	$(XRANDR_CFLAGS) \
> > >  	$(NULL)
> > >  
> > >  AM_CFLAGS = \
> > > @@ -50,8 +52,10 @@ spice_streaming_agent_LDADD = \
> > >  	-ldl \
> > >  	-lpthread \
> > >  	libstreaming-utils.a \
> > > +	$(DRM_LIBS) \
> > >  	$(X11_LIBS) \
> > >  	$(XFIXES_LIBS) \
> > > +	$(XRANDR_LIBS) \
> > >  	$(JPEG_LIBS) \
> > >  	$(NULL)
> > >  
> > > @@ -61,6 +65,7 @@ spice_streaming_agent_SOURCES = \
> > >  	concrete-agent.hpp \
> > >  	cursor-updater.cpp \
> > >  	cursor-updater.hpp \
> > > +	display-info.cpp \
> > >  	frame-log.cpp \
> > >  	frame-log.hpp \
> > >  	mjpeg-fallback.cpp \
> > > @@ -69,7 +74,9 @@ spice_streaming_agent_SOURCES = \
> > >  	jpeg.hpp \
> > >  	stream-port.cpp \
> > >  	stream-port.hpp \
> > > +	utils.cpp \
> > >  	utils.hpp \
> > > +	x-display-info.cpp \
> > >  	$(NULL)
> > >  
> > >  if HAVE_GST
> > > diff --git a/src/display-info.cpp b/src/display-info.cpp
> > > new file mode 100644
> > > index 0000000..1f5e497
> > > --- /dev/null
> > > +++ b/src/display-info.cpp
> > > @@ -0,0 +1,100 @@
> > > +/* \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include <spice-streaming-agent/display-info.hpp>
> > > +
> > > +#include "utils.hpp"
> > > +#include <spice-streaming-agent/error.hpp>
> > > +
> > > +#include <algorithm>
> > > +#include <cstring>
> > > +#include <fstream>
> > > +#include <limits.h>
> > > +
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +
> > > +namespace {
> > > +
> > > +std::string get_card_real_path(const std::string &card_path)
> > > +{
> > > +    char real_path_buf[PATH_MAX];
> > > +    if (realpath(card_path.c_str(), real_path_buf) == NULL) {
> > > +        throw Error("Failed to realpath \"" + card_path + "\": " +
> > > strerror(errno));
> > > +    }
> > > +
> > > +    return real_path_buf;
> > > +}
> > 
> > Not much related to a "card", just calling real_path.
> > Planning to extend it?
> 
> No, but it is a local function anyway, doesn't matter much. Want me to
> rename it?
> 

No, fine with current name.

> > > +
> > > +} // namespace
> > > +
> > > +std::vector<std::string> list_cards()
> > > +{
> > > +    std::string glob_path = "/sys/class/drm/card*";
> > > +    auto globs = utils::glob(glob_path);
> > > +
> > > +    globs.erase(std::remove_if(globs.begin(), globs.end(), [](const
> > > std::string &glob) {
> > > +            // if the path contains "-", it is an output, not a card,
> > > filter
> > > it out
> > > +            return glob.find("-") != glob.npos;
> > > +        }),
> > > +        globs.end()
> > > +    );
> > > +
> > > +    return globs;
> > > +}
> > > +
> > > +uint32_t read_hex_number_from_file(const std::string &path)
> > > +{
> > > +    uint32_t res;
> > > +    std::ifstream vf(path);
> > > +
> > > +    if (vf.fail()) {
> > > +        throw Error("Failed to open " + path + ": " +
> > > std::strerror(errno));
> > > +    }
> > > +
> > > +    if (!(vf >> std::hex >> res)) {
> > > +        throw Error("Failed to read from " + path + ": " +
> > > std::strerror(errno));
> > > +    }
> > > +
> > > +    return res;
> > > +}
> > > +
> > > +std::string get_device_address(const std::string &card_path)
> > > +{
> > > +    std::string real_path = get_card_real_path(card_path);
> > > +
> > > +    // real_path is e.g. /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> > > +    std::string device_address = "pci/";
> > > +
> > > +    const std::string path_prefix = "/sys/devices/pci";
> > > +
> > > +    // if real_path doesn't start with path_prefix
> > > +    if (real_path.compare(0, path_prefix.length(), path_prefix)) {
> > > +        throw Error("Invalid device path \"" + real_path + "\"");
> > > +    }
> > > +
> > > +    // /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> > > +    //                 ^ ptr
> > > +    const char *ptr = real_path.c_str() + path_prefix.length();
> > > +
> > > +    // copy the domain
> > > +    device_address += std::string(ptr, 4);
> > > +
> > > +    // /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> > > +    //                         ^ ptr
> > > +    ptr += 8;
> > > +
> > > +    uint32_t domain, bus, slot, function, n;
> > > +    while (sscanf(ptr, "%x:%x:%x.%x/%n", &domain, &bus, &slot,
> > > &function,
> > > &n) == 4) {
> > 
> > %n does not increment the counter and this could be an issue.
> > If ptr points to "0:0:0.0" n is not written which could make
> > ptr not much deterministic
> > 
> > > +        char pci_node[6];
> > > +        snprintf(pci_node, 6, "/%02x.%01x", slot, function);
> > > +        device_address += pci_node;
> > > +        ptr += n;
> > 
> > here, if n is uninitialized or from a previous string ptr can
> > became invalid. Would be different if the string were
> > "%x:%x:%x.%x%n", in this case if sscanf returns 4 it always write n.
> 
> Ok, so if I understand correctly, changing the pattern to
> "/%x:%x:%x.%x%n" will work as well?
> 

Yes, I suppose you need to change code to start ptr with a "/", I
didn't check if is possible, from your example
("/sys/devices/pci0000:00/0000:00:02.0/drm/card0") it is.

> > > +    }
> > > +
> > > +    return device_address;
> > > +}
> > > +
> > > +}} // namespace spice::streaming_agent
> > > diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> > > index 097ef9d..b98b30e 100644
> > > --- a/src/gst-plugin.cpp
> > > +++ b/src/gst-plugin.cpp
> > > @@ -23,6 +23,8 @@
> > >  
> > >  #include <spice-streaming-agent/plugin.hpp>
> > >  #include <spice-streaming-agent/frame-capture.hpp>
> > > +#include <spice-streaming-agent/x-display-info.hpp>
> > > +
> > >  
> > >  #define gst_syslog(priority, str, ...) syslog(priority, "Gstreamer
> > >  plugin: "
> > >  str, ## __VA_ARGS__);
> > >  
> > > @@ -76,6 +78,7 @@ public:
> > >      SpiceVideoCodecType VideoCodecType() const override {
> > >          return settings.codec;
> > >      }
> > > +    std::vector<DeviceDisplayInfo> get_device_display_info() const
> > > override;
> > >  private:
> > >      void free_sample();
> > >      GstElement *get_encoder_plugin(const GstreamerEncoderSettings
> > >      &settings,
> > >      GstCapsUPtr &sink_caps);
> > > @@ -402,6 +405,17 @@ FrameInfo GstreamerFrameCapture::CaptureFrame()
> > >      return info;
> > >  }
> > >  
> > > +std::vector<DeviceDisplayInfo>
> > > GstreamerFrameCapture::get_device_display_info() const
> > > +{
> > > +    try {
> > > +        return get_device_display_info_drm(dpy);
> > > +    } catch (const std::exception &e) {
> > > +        syslog(LOG_WARNING, "Failed to get device info using DRM: %s.
> > > Using
> > > no-DRM fallback.",
> > > +               e.what());
> > > +        return get_device_display_info_no_drm(dpy);
> > > +    }
> > > +}
> > > +
> > >  FrameCapture *GstreamerPlugin::CreateCapture()
> > >  {
> > >      return new GstreamerFrameCapture(settings);
> > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > index 8081007..246c305 100644
> > > --- a/src/mjpeg-fallback.cpp
> > > +++ b/src/mjpeg-fallback.cpp
> > > @@ -7,6 +7,9 @@
> > >  #include <config.h>
> > >  #include "mjpeg-fallback.hpp"
> > >  
> > > +#include "jpeg.hpp"
> > > +#include <spice-streaming-agent/x-display-info.hpp>
> > > +
> > >  #include <cstring>
> > >  #include <exception>
> > >  #include <stdexcept>
> > > @@ -15,8 +18,6 @@
> > >  #include <syslog.h>
> > >  #include <X11/Xlib.h>
> > >  
> > > -#include "jpeg.hpp"
> > > -
> > >  using namespace spice::streaming_agent;
> > >  
> > >  static inline uint64_t get_time()
> > > @@ -40,6 +41,7 @@ public:
> > >      SpiceVideoCodecType VideoCodecType() const override {
> > >          return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > >      }
> > > +    std::vector<DeviceDisplayInfo> get_device_display_info() const
> > > override;
> > >  private:
> > >      MjpegSettings settings;
> > >      Display *dpy;
> > > @@ -137,6 +139,17 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
> > >      return info;
> > >  }
> > >  
> > > +std::vector<DeviceDisplayInfo>
> > > MjpegFrameCapture::get_device_display_info()
> > > const
> > > +{
> > > +    try {
> > > +        return get_device_display_info_drm(dpy);
> > > +    } catch (const std::exception &e) {
> > > +        syslog(LOG_WARNING, "Failed to get device info using DRM: %s.
> > > Using
> > > no-DRM fallback.",
> > > +               e.what());
> > > +        return get_device_display_info_no_drm(dpy);
> > > +    }
> > > +}
> > > +
> > >  FrameCapture *MjpegPlugin::CreateCapture()
> > >  {
> > >      return new MjpegFrameCapture(settings);
> > > diff --git a/src/spice-streaming-agent.cpp
> > > b/src/spice-streaming-agent.cpp
> > > index 1eb8e1b..3024d98 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -217,6 +217,19 @@ do_capture(StreamPort &stream_port, FrameLog
> > > &frame_log)
> > >              throw std::runtime_error("cannot find a suitable capture
> > >              system");
> > >          }
> > >  
> > > +        try {
> > > +            std::vector<DeviceDisplayInfo> display_info =
> > > capture->get_device_display_info();
> > > +            syslog(LOG_DEBUG, "Got device info of %lu devices from the
> > > plugin", display_info.size());
> > > +            for (const auto &info : display_info) {
> > > +                syslog(LOG_DEBUG, "   id %u: device address %s, device
> > > display id: %u",
> > > +                       info.id,
> > > +                       info.device_address.c_str(),
> > > +                       info.device_display_id);
> > > +            }
> > > +        } catch (const Error &e) {
> > > +            syslog(LOG_ERR, "Error while getting device info: %s",
> > > e.what());
> > > +        }
> > > +
> > >          while (!quit_requested && streaming_requested) {
> > >              if (++frame_count % 100 == 0) {
> > >                  syslog(LOG_DEBUG, "SENT %d frames", frame_count);
> > > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > > index 1ae5a07..1b82c56 100644
> > > --- a/src/unittests/Makefile.am
> > > +++ b/src/unittests/Makefile.am
> > > @@ -5,6 +5,7 @@ AM_CPPFLAGS = \
> > >  	-I$(top_srcdir)/include \
> > >  	-I$(top_srcdir)/src \
> > >  	-I$(top_srcdir)/src/unittests \
> > > +	$(DRM_CFLAGS) \
> > >  	$(SPICE_PROTOCOL_CFLAGS) \
> > >  	$(NULL)
> > >  
> > > @@ -43,13 +44,18 @@ hexdump_LDADD = \
> > >  
> > >  test_mjpeg_fallback_SOURCES = \
> > >  	test-mjpeg-fallback.cpp \
> > > +	../display-info.cpp \
> > >  	../jpeg.cpp \
> > >  	../mjpeg-fallback.cpp \
> > > +	../utils.cpp \
> > > +	../x-display-info.cpp \
> > >  	$(NULL)
> > >  
> > >  test_mjpeg_fallback_LDADD = \
> > > +	$(DRM_LIBS) \
> > >  	$(X11_LIBS) \
> > >  	$(JPEG_LIBS) \
> > > +	$(XRANDR_LIBS) \
> > >  	$(NULL)
> > >  
> > >  test_stream_port_SOURCES = \
> > > diff --git a/src/utils.cpp b/src/utils.cpp
> > > new file mode 100644
> > > index 0000000..254fb29
> > > --- /dev/null
> > > +++ b/src/utils.cpp
> > > @@ -0,0 +1,45 @@
> > > +/* \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include "utils.hpp"
> > > +
> > > +#include <spice-streaming-agent/error.hpp>
> > > +
> > > +#include <glob.h>
> > > +#include <string.h>
> > > +#include <stdexcept>
> > > +
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +namespace utils {
> > > +
> > > +std::vector<std::string> glob(const std::string& pattern) {
> > 
> > missing \n
> 
> Ah, old habbits. Will fix.
> 
> > > +    glob_t glob_result{};
> > > +
> > > +    std::vector<std::string> filenames;
> > > +
> > > +    int ret = glob(pattern.c_str(), GLOB_ERR, NULL, &glob_result);
> > > +
> > > +    if(ret != 0) {
> > > +        globfree(&glob_result);
> > > +
> > > +        if (ret == GLOB_NOMATCH) {
> > > +            return filenames;
> > > +        }
> > > +
> > > +        throw Error("glob(" + pattern + ") failed with return value " +
> > > std::to_string(ret) +
> > > +                    ": " + strerror(errno));
> > > +    }
> > > +
> > > +    for(size_t i = 0; i < glob_result.gl_pathc; ++i) {
> > > +        filenames.push_back(glob_result.gl_pathv[i]);
> > > +    }
> > > +
> > > +    globfree(&glob_result);
> > > +
> > > +    return filenames;
> > > +}
> > > +
> > > +}}} // namespace spice::streaming_agent::utils
> > > diff --git a/src/utils.hpp b/src/utils.hpp
> > > index 64cb538..8c3b04e 100644
> > > --- a/src/utils.hpp
> > > +++ b/src/utils.hpp
> > > @@ -5,6 +5,8 @@
> > >  #ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> > >  #define SPICE_STREAMING_AGENT_UTILS_HPP
> > >  
> > > +#include <vector>
> > > +#include <string>
> > >  #include <syslog.h>
> > >  
> > >  
> > > @@ -12,6 +14,8 @@ namespace spice {
> > >  namespace streaming_agent {
> > >  namespace utils {
> > >  
> > > +std::vector<std::string> glob(const std::string& pattern);
> > > +
> > >  template<class T>
> > >  const T &syslog(const T &error) noexcept
> > >  {
> > > diff --git a/src/x-display-info.cpp b/src/x-display-info.cpp
> > > new file mode 100644
> > > index 0000000..ee223ea
> > > --- /dev/null
> > > +++ b/src/x-display-info.cpp
> > > @@ -0,0 +1,301 @@
> > > +/* \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include <spice-streaming-agent/x-display-info.hpp>
> > > +
> > > +#include <spice-streaming-agent/display-info.hpp>
> > > +#include "utils.hpp"
> > > +#include <spice-streaming-agent/error.hpp>
> > > +
> > > +#include <algorithm>
> > > +#include <cstring>
> > > +#include <map>
> > > +#include <fcntl.h>
> > > +#include <sys/stat.h>
> > > +#include <unistd.h>
> > > +#include <X11/extensions/Xrandr.h>
> > > +#include <xf86drm.h>
> > > +#include <xf86drmMode.h>
> > > +
> > > +
> > > +namespace spice {
> > > +namespace streaming_agent {
> > > +
> > > +namespace {
> > > +
> > > +constexpr uint32_t pci_vendor_id_redhat = 0x1b36;
> > > +constexpr uint32_t pci_device_id_qxl = 0x0100;
> > > +
> > > +struct OutputInfo {
> > > +    std::string output_name;
> > > +    std::string card_path;
> > > +    uint32_t card_vendor_id;
> > > +    uint32_t card_device_id;
> > > +    uint32_t device_display_id;
> > > +};
> > > +
> > > +static const std::map<uint32_t, std::string> modesetting_output_names =
> > > {
> > > +    {DRM_MODE_CONNECTOR_Unknown, "None"},
> > > +    {DRM_MODE_CONNECTOR_VGA, "VGA"},
> > > +    {DRM_MODE_CONNECTOR_DVII, "DVI-I"},
> > > +    {DRM_MODE_CONNECTOR_DVID, "DVI-D"},
> > > +    {DRM_MODE_CONNECTOR_DVIA, "DVI-A"},
> > > +    {DRM_MODE_CONNECTOR_Composite, "Composite"},
> > > +    {DRM_MODE_CONNECTOR_SVIDEO, "SVIDEO"},
> > > +    {DRM_MODE_CONNECTOR_LVDS, "LVDS"},
> > > +    {DRM_MODE_CONNECTOR_Component, "Component"},
> > > +    {DRM_MODE_CONNECTOR_9PinDIN, "DIN"},
> > > +    {DRM_MODE_CONNECTOR_DisplayPort, "DP"},
> > > +    {DRM_MODE_CONNECTOR_HDMIA, "HDMI"},
> > > +    {DRM_MODE_CONNECTOR_HDMIB, "HDMI-B"},
> > > +    {DRM_MODE_CONNECTOR_TV, "TV"},
> > > +    {DRM_MODE_CONNECTOR_eDP, "eDP"},
> > > +    {DRM_MODE_CONNECTOR_VIRTUAL, "Virtual"},
> > > +    {DRM_MODE_CONNECTOR_DSI, "DSI"},
> > > +    {DRM_MODE_CONNECTOR_DPI, "DPI"},
> > > +};
> > > +
> > > +// Connector type names from qxl driver
> > > +static const std::map<uint32_t, std::string> qxl_output_names = {
> > > +    {DRM_MODE_CONNECTOR_Unknown, "None"},
> > > +    {DRM_MODE_CONNECTOR_VGA, "VGA"},
> > > +    {DRM_MODE_CONNECTOR_DVII, "DVI"},
> > > +    {DRM_MODE_CONNECTOR_DVID, "DVI"},
> > > +    {DRM_MODE_CONNECTOR_DVIA, "DVI"},
> > > +    {DRM_MODE_CONNECTOR_Composite, "Composite"},
> > > +    {DRM_MODE_CONNECTOR_SVIDEO, "S-video"},
> > > +    {DRM_MODE_CONNECTOR_LVDS, "LVDS"},
> > > +    {DRM_MODE_CONNECTOR_Component, "CTV"},
> > > +    {DRM_MODE_CONNECTOR_9PinDIN, "DIN"},
> > > +    {DRM_MODE_CONNECTOR_DisplayPort, "DisplayPort"},
> > > +    {DRM_MODE_CONNECTOR_HDMIA, "HDMI"},
> > > +    {DRM_MODE_CONNECTOR_HDMIB, "HDMI"},
> > > +    {DRM_MODE_CONNECTOR_TV, "TV"},
> > > +    {DRM_MODE_CONNECTOR_eDP, "eDP"},
> > > +    {DRM_MODE_CONNECTOR_VIRTUAL, "Virtual"},
> > > +};
> > > +
> > > +std::string get_drm_name(const std::map<uint32_t, std::string>
> > > &name_map,
> > > +                         uint32_t connector_type,
> > > +                         uint32_t connector_type_id)
> > > +{
> > > +    auto name_it = name_map.find(connector_type);
> > > +    if (name_it == name_map.end()) {
> > > +        throw Error("Could not find DRM connector name for type " +
> > > std::to_string(connector_type));
> > > +    }
> > > +
> > > +    return name_it->second + "-" + std::to_string(connector_type_id);
> > > +}
> > > +
> > > +class DrmOutputGetter
> > > +{
> > > +public:
> > > +    DrmOutputGetter(const char* card_path)
> > > +    {
> > > +        drm_fd = open(card_path, O_RDWR);
> > > +        if (drm_fd < 0) {
> > > +            throw Error(std::string("Unable to open file %s: ") +
> > > strerror(errno));
> > > +        }
> > > +
> > > +        drm_resources = drmModeGetResources(drm_fd);
> > > +
> > > +        if (drm_resources == nullptr) {
> > > +            close(drm_fd);
> > > +            throw Error(std::string("Unable to get DRM resources for
> > > card ")
> > > + card_path);
> > > +        }
> > > +    }
> > > +
> > > +    ~DrmOutputGetter()
> > > +    {
> > > +        drmModeFreeResources(drm_resources);
> > > +        close(drm_fd);
> > > +    }
> > > +
> > > +    std::vector<std::string> get_output_names(const std::map<uint32_t,
> > > std::string> &names_map,
> > > +                                              bool decrement = false)
> > > +    {
> > > +        std::vector<std::string> result;
> > > +
> > > +        for (size_t i = 0; i < drm_resources->count_connectors; ++i) {
> > > +            drmModeConnectorPtr conn = drmModeGetConnector(drm_fd,
> > > drm_resources->connectors[i]);
> > > +
> > > +            if (conn == nullptr) {
> > > +                throw Error(std::string("Unable to get DRM connector: ")
> > > +
> > > strerror(errno));
> > > +            }
> > > +
> > > +            uint32_t cti = conn->connector_type_id;
> > > +            if (decrement) {
> > > +                cti--;
> > > +            }
> > > +            result.push_back(get_drm_name(names_map,
> > > conn->connector_type,
> > > cti));
> > > +        }
> > > +
> > > +        return result;
> > > +    }
> > > +
> > > +    int drm_fd = -1;
> > > +    drmModeResPtr drm_resources = nullptr;
> > 
> > Are these supposed to be public?
> 
> I didn't pay much attention, as this is just a local boilerplate helper
> class, its purpose isn't to hide anything, just RAII. I can private
> them though.
> 

If it does not hurt why not.
Usually if you are not encapsulating you can use "struct" instead of
"class", in C++ the main difference if the default protection (I
suppose is just question of style).

> > > +};
> > > +
> > > +std::vector<OutputInfo> get_outputs()
> > > +{
> > > +    std::vector<OutputInfo> result;
> > > +
> > > +    for (uint8_t card_id = 0; card_id < 10; ++card_id) {
> > > +        std::string card_name = "card" + std::to_string(card_id);
> > > +
> > > +        char drm_path[64];
> > > +        snprintf(drm_path, sizeof(drm_path), DRM_DEV_NAME, DRM_DIR_NAME,
> > > card_id);
> > > +
> > > +        struct stat stat_buf;
> > > +        if (stat(drm_path, &stat_buf) != 0) {
> > > +            if (errno == ENOENT) {
> > > +                // we're done searching
> > > +                break;
> > > +            }
> > > +            throw Error(std::string("Error accessing DRM node for card
> > > ") +
> > > drm_path);
> > > +        }
> > > +
> > > +        std::string sys_path = "/sys/class/drm/" + card_name;
> > > +
> > > +        uint32_t vendor_id = read_hex_number_from_file(sys_path +
> > > "/device/vendor");
> > > +        uint32_t device_id = read_hex_number_from_file(sys_path +
> > > "/device/device");
> > > +
> > > +        std::vector<std::string> outputs;
> > > +
> > > +        if (vendor_id == pci_vendor_id_redhat && device_id ==
> > > pci_device_id_qxl) {
> > > +            // to find out whether QXL output names need decrementing,
> > > look
> > > for
> > > +            // an output called Virtual-0 in /sys/class/drm and if we
> > > find
> > > one,
> > > +            // decrement the number in the output names
> > > +            const auto globs =
> > > utils::glob("/sys/class/drm/card*-Virtual-0");
> > > +            outputs =
> > > DrmOutputGetter(drm_path).get_output_names(qxl_output_names, globs.size()
> > > >
> > > 0);
> > > +        } else {
> > > +            outputs =
> > > DrmOutputGetter(drm_path).get_output_names(modesetting_output_names);
> > > +        }
> > > +
> > > +        for (uint32_t i = 0; i < outputs.size(); ++i) {
> > > +            result.push_back({outputs[i],
> > > +                              sys_path,
> > > +                              vendor_id,
> > > +                              device_id,
> > > +                              i});
> > > +        }
> > > +    }
> > > +
> > > +    return result;
> > > +}
> > > +
> > > +} // namespace
> > > +
> > > +std::vector<std::string> get_xrandr_outputs(Display *display, Window
> > > window)
> > > +{
> > > +    XRRScreenResources *screen_resources =
> > > XRRGetScreenResources(display,
> > > window);
> > > +    std::vector<std::string> result;
> > > +    for (int i = 0; i < screen_resources->noutput; ++i) {
> > > +        XRROutputInfo *output_info = XRRGetOutputInfo(display,
> > > +                                                      screen_resources,
> > > +
> > > screen_resources->outputs[i]);
> > > +
> > > +        result.emplace_back(output_info->name);
> > > +
> > > +        XRRFreeOutputInfo(output_info);
> > > +    }
> > > +
> > > +    XRRFreeScreenResources(screen_resources);
> > > +    return result;
> > > +}
> > > +
> > > +std::vector<DeviceDisplayInfo> get_device_display_info_drm(Display
> > > *display)
> > > +{
> > > +    auto outputs = get_outputs();
> > > +
> > > +    // sorting by output name, may not be necessary (the list should be
> > > sorted out of glob)
> > > +    std::sort(outputs.begin(), outputs.end(), [](const OutputInfo &oi1,
> > > const OutputInfo &oi2) {
> > > +            return oi1.output_name > oi2.output_name;
> > > +        }
> > > +    );
> > > +
> > > +    // Check if the output names of QXL cards start at Virtual-1 and if
> > > so,
> > > +    // subtract one from each of them
> > > +    for (auto &output : outputs) {
> > > +        // if we find a QXL card
> > > +        if (output.card_vendor_id == pci_vendor_id_redhat &&
> > > +            output.card_device_id == pci_device_id_qxl)
> > > +        {
> > > +            // if the first name is Virtual-0, we know we are good, quit
> > > the
> > > loop
> > > +            if (output.output_name == "Virtual-0") {
> > > +                break;
> > > +            }
> > > +
> > > +            // if the name starts with "Virtual-"
> > > +            if (!output.output_name.compare(0, 8, "Virtual-")) {
> > > +                // decrement the last digit by one
> > > +                // fails if the number at the end is more than one
> > > digit,
> > > +                // which would imply multiple QXL devices, an
> > > unsupported
> > > case
> > > +                output.output_name[output.output_name.length() - 1]--;
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    std::map<std::string, OutputInfo> output_map;
> > > +    for (const auto &output : outputs) {
> > > +        output_map[output.output_name] = output;
> > > +    }
> > > +
> > > +    auto xrandr_outputs = get_xrandr_outputs(display,
> > > RootWindow(display,
> > > XDefaultScreen(display)));
> > > +
> > > +    std::vector<DeviceDisplayInfo> result;
> > > +
> > > +    for (uint32_t i = 0; i < xrandr_outputs.size(); ++i) {
> > > +        const auto &output = xrandr_outputs[i];
> > > +        const auto it = output_map.find(output);
> > > +        if (it == output_map.end()) {
> > > +            throw Error("Could not find card for output " + output);
> > > +        }
> > > +
> > > +        std::string device_address =
> > > get_device_address(it->second.card_path);
> > > +
> > > +        result.push_back({i, device_address,
> > > it->second.device_display_id});
> > > +    }
> > > +
> > > +    return result;
> > > +}
> > > +
> > > +std::vector<DeviceDisplayInfo> get_device_display_info_no_drm(Display
> > > *display)
> > > +{
> > > +    // look for the first card that doesn't list its outputs (and
> > > therefore
> > > +    // doesn't support DRM), that's the best we can do...
> > > +    const auto globs = utils::glob("/sys/class/drm/card*");
> > > +
> > > +    std::string found_card_path;
> > > +    for (const auto &path : globs) {
> > > +        std::string card_output = path.substr(path.rfind("/") + 1,
> > > path.npos);
> > > +        auto dash_it = card_output.find("-");
> > > +        // if this file is a card (and not an output)
> > > +        if (dash_it == card_output.npos) {
> > > +            // if we already have a found card, this means it doesn't
> > > have
> > > any outputs,
> > > +            // we have found our card
> > > +            if (!found_card_path.empty()) {
> > > +                break;
> > > +            }
> > > +            found_card_path = card_output;
> > > +        } else {
> > > +            found_card_path = "";
> > > +        }
> > > +    }
> > > +
> > > +    std::string device_address = get_device_address(found_card_path);
> > > +
> > > +    auto xrandr_outputs = get_xrandr_outputs(display,
> > > RootWindow(display,
> > > XDefaultScreen(display)));
> > > +
> > > +    std::vector<DeviceDisplayInfo> result;
> > > +
> > > +    for (uint32_t i = 0; i < xrandr_outputs.size(); ++i) {
> > > +        result.push_back({i, device_address, i});
> > > +    }
> > > +
> > > +    return result;
> > > +}
> > > +
> > > +}} // namespace spice::streaming_agent
> > 
> > Frediano
> 


More information about the Spice-devel mailing list