On Wed, 2018-11-07 at 16:01 -0600, Jonathon Jongsma wrote:
> On Wed, 2018-11-07 at 11:49 +0100, Lukáš Hrázký wrote:
> > Adds a function to let QEMU provide information to identify graphics
> > devices and their monitors in the guest. The function
> > (spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
> > and monitor ID -> device display ID mapping of displays exposed by
> > given
> > QXL interface.
>
> The phrase "monitor ID" should be described / explained more explicitly
> in the commit log, I think. As far as I can tell, we're using the term
> "monitor ID" to mean a 0-based index of monitors that are associated
> with this QXL instance. Right?
I can add some description of what monitor ID is to the commit log, I
felt the detailed description is more useful in code and didn't want to
repeat myself here.
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > ---
> > server/red-qxl.c | 44 ++++++++++++++++++++++++++++++++++++++
> > server/spice-qxl.h | 46
> > ++++++++++++++++++++++++++++++++++++++++
> > server/spice-server.syms | 5 +++++
> > 3 files changed, 95 insertions(+)
> >
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..0ea424cd 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> > #include "red-qxl.h"
> >
> >
> > +#define MAX_DEVICE_ADDRESS_LEN 256
> > +#define MAX_MONITORS_COUNT 16
> > +
> > struct QXLState {
> > QXLWorker qxl_worker;
> > QXLInstance *qxl;
> > @@ -53,6 +56,9 @@ struct QXLState {
> > unsigned int max_monitors;
> > RedsState *reds;
> > RedWorker *worker;
> > + char device_address[MAX_DEVICE_ADDRESS_LEN];
> > + uint32_t device_display_ids[MAX_MONITORS_COUNT];
> > + size_t monitors_count; // length of ^^^
> >
> > pthread_mutex_t scanout_mutex;
> > SpiceMsgDisplayGlScanoutUnix scanout;
> > @@ -846,6 +852,44 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> > *qxl)
> > red_qxl_async_complete(qxl, cookie);
> > }
> >
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_set_device_info(QXLInstance *instance,
> > + const char *device_address,
> > + uint32_t device_display_id_start,
> > + uint32_t device_display_id_count)
> > +{
> > + g_return_if_fail(device_address != NULL);
> > +
> > + size_t da_len = strnlen(device_address, MAX_DEVICE_ADDRESS_LEN);
> > + if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
> > + spice_error("Device address too long: %lu > %u", da_len,
> > MAX_DEVICE_ADDRESS_LEN);
> > + return;
> > + }
> > +
> > + if (device_display_id_count > MAX_MONITORS_COUNT) {
> > + spice_error("Device display ID count (%u) is greater
> > thanbelieve limit %u",
> > + device_display_id_count,
> > + MAX_MONITORS_COUNT);
> > + return;
> > + }
> > +
> > + strncpy(instance->st->device_address, device_address,
> > MAX_DEVICE_ADDRESS_LEN);
> > +
> > + g_debug("QXL Instance %d setting device address \"%s\" and
> > monitor -> device display mapping:",
> > + instance->id,
> > + device_address);
> > +
> > + // store the mapping monitor_id -> device_display_id
> > + for (uint32_t monitor_id = 0; monitor_id <
> > device_display_id_count; ++monitor_id) {
>
> In theory, you could simply use instance->st->monitors_count here
> instead of a separate monitor_id variable. Probably clearer the way you
> wrote it though...
Right... :)
> > + uint32_t device_display_id = device_display_id_start +
> > monitor_id;
> > + instance->st->device_display_ids[monitor_id] =
> > device_display_id;
> > + g_debug(" monitor ID %u -> device display ID %u",
> > + monitor_id, device_display_id);
> > + }
> > +
> > + instance->st->monitors_count = device_display_id_count;
> > +}
> > +
> > void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> > {
> > QXLState *qxl_state;
> > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > index 0c4e75fc..547d3d93 100644
> > --- a/server/spice-qxl.h
> > +++ b/server/spice-qxl.h
> > @@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance
> > *instance,
> > uint32_t w, uint32_t h,
> > uint64_t cookie);
> >
> > +/* since spice 0.14.2 */
> > +
> > +/**
> > + * spice_qxl_set_device_info:
> > + * @instance the QXL instance to set the path to
> > + * @device_address the path of the device this QXL instance belongs
> > to
> > + * @device_display_id_start the starting device display ID of this
> > interface,
> > + * i.e. the one monitor ID 0 maps to
> > + * @device_display_id_count the total number of device display IDs
> > on this
> > + * interface
> > + *
> > + * Sets the device information for this QXL interface, i.e. the
> > hardware
> > + * address (e.g. PCI) of the graphics device and the IDs of the
> > displays of the
> > + * graphics device that are exposed by this interface (device
> > display IDs).
> > + *
> > + * The supported device address format is:
> > + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> > + *
> > + * The "pci" identifies the rest of the string as a PCI address. It
> > is the only
> > + * supported address at the moment, other identifiers can be
> > introduced later.
> > + * <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
> > + *
> > + * The device_display_id_{start,count} denotes the sequence of
> > device display
> > + * IDs that map to the zero-based sequence of monitor IDs provided
> > by monitors
> > + * config on this interface.
>
> Here you finally define "monitor ID". But it has been used above
> already. I wonder if there's any way to introduce this information
> earlier...
>
> > For example with device_display_id_start = 2 and
> > + * device_display_id_count = 3 you get the following mapping:
> > + * monitor_id -> device_display_id
> > + * 0 -> 2
> > + * 1 -> 3
> > + * 2 -> 4
> > + *
> > + * Note this example is unsupported in practice. The only supported
> > cases are
> > + * either a single device display ID (count = 1) or multiple device
> > display IDs
> > + * in a sequence starting from 0.
> > + */
>
>
> I do like having documentation in the header. However, I *think* that
> the reason that other projects like spice-gtk put the documentation in
> the .c file is because that's where gtkdoc tool that produces the API
> documentation expects it. We don't generate any API doc in spice server
> (but maybe we should?), and we don't use gtkdoc, so it probably doesn't
> matter here.
Well, there is almost no documentation throughout spice server.
Generating documentation is a good thing, but not nearly as much if
it's so incomplete. Also this is not an API for general public, so I
think being able to read documentation in the code is much more useful.
The good thing about generating something would be also validating the
format.
Anyway, documented spice server code... Pipe dream... :)
Cheers,
Lukas
> > +void spice_qxl_set_device_info(QXLInstance *instance,
> > + const char *device_address,
> > + uint32_t device_display_id_start,
> > + uint32_t device_display_id_count);
> > +
> > typedef struct QXLDevInitInfo {
> > uint32_t num_memslots_groups;
> > uint32_t num_memslots;
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index edf04a42..ac4d9b14 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -173,3 +173,8 @@ SPICE_SERVER_0.13.2 {
> > global:
> > spice_server_set_video_codecs;
> > } SPICE_SERVER_0.13.1;
> > +
> > +SPICE_SERVER_0.14.2 {
> > +global:
> > + spice_qxl_set_device_info;
> > +} SPICE_SERVER_0.13.2;
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel