Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
Hi,
> > I can see that it simplifies the logic in spice-server if we have a
> > single function call instead of two. So we could deprecate
> > spice_qxl_set_max_monitors() in favour of your
> > spice_qxl_set_device_info() variant.
> >
> > spice_qxl_set_max_monitors() would then basically do this:
> >
> > spice_qxl_set_max_monitors(qxl, max)
> > {
> > spice_qxl_set_device_info(qxl, NULL, 0, max);
> > }
>
> I can't actually do this, it does the wrong thing for the one-channel-
> per-head (virtio-gpu) case. For that case it would send all
> device_display_ids to 0 on all interfaces, but they need to be
> different numbers.
Well, qemu calls spice_qxl_set_max_monitors() only in case there are
multiple monitors in one channel, i.e. qxl.
cheers,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
>
> On Mon, 2018-11-05 at 14:08 +0100, Gerd Hoffmann wrote:
> > On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> > > On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > > > 2. Have a single function as follows:
> > > > >
> > > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > > >const char *device_address,
> > > > >uint32_t device_display_id_start,
> > > > >uint32_t device_display_id_count);
> > > >
> > > > How about:
> > > >
> > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > >const char *device_address,
> > > >uint32_t device_display_id);
> > > >
> > > > I don't think we need start+count:
> > > >
> > > > * For single-head devices device_display_id will be zero.
> > > > * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> > > >device_display_id will enumerate the heads (so everybody can figure
> > > >which channel is which head).
> > > > * For one-channel-per-device multihead devices (i.e. qxl/linux)
> > > >device_display_id will be zero too. Number of heads is set via
> > > >spice_qxl_set_max_monitors().
> > >
> > > That requires nontrivial and unexpected logic for the one-channel-per-
> > > device multihead devices case. The API should be doing what it says and
> > > the dumber the better, this seems too smart to me...
> >
> > Well, the device_display_id_count argument is redundant with
> > spice_qxl_set_max_monitors(). That isn't a great API either.
> >
> > I can see that it simplifies the logic in spice-server if we have a
> > single function call instead of two. So we could deprecate
> > spice_qxl_set_max_monitors() in favour of your
> > spice_qxl_set_device_info() variant.
> >
> > spice_qxl_set_max_monitors() would then basically do this:
> >
> > spice_qxl_set_max_monitors(qxl, max)
> > {
> > spice_qxl_set_device_info(qxl, NULL, 0, max);
> > }
>
> I can't actually do this, it does the wrong thing for the one-channel-
> per-head (virtio-gpu) case. For that case it would send all
> device_display_ids to 0 on all interfaces, but they need to be
> different numbers.
>
This function is only called for QXL cards devices so no virtio-gpu,
no cirrus, no svga or anything else.
> For backwards compatibility when spice_qxl_set_device_info() is not
> called I think we need to fallback to the old behaviour, as we are
> missing the necessary information.
>
> I'll send v3 with a single function using start + count and deprecate
> the set_max_monitors one.
>
> Cheers,
> Lukas
>
> > cheers,
> > Gerd
> >
>
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
On Mon, 2018-11-05 at 14:08 +0100, Gerd Hoffmann wrote:
> On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> > On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > > 2. Have a single function as follows:
> > > >
> > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > >const char *device_address,
> > > >uint32_t device_display_id_start,
> > > >uint32_t device_display_id_count);
> > >
> > > How about:
> > >
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >const char *device_address,
> > >uint32_t device_display_id);
> > >
> > > I don't think we need start+count:
> > >
> > > * For single-head devices device_display_id will be zero.
> > > * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> > >device_display_id will enumerate the heads (so everybody can figure
> > >which channel is which head).
> > > * For one-channel-per-device multihead devices (i.e. qxl/linux)
> > >device_display_id will be zero too. Number of heads is set via
> > >spice_qxl_set_max_monitors().
> >
> > That requires nontrivial and unexpected logic for the one-channel-per-
> > device multihead devices case. The API should be doing what it says and
> > the dumber the better, this seems too smart to me...
>
> Well, the device_display_id_count argument is redundant with
> spice_qxl_set_max_monitors(). That isn't a great API either.
>
> I can see that it simplifies the logic in spice-server if we have a
> single function call instead of two. So we could deprecate
> spice_qxl_set_max_monitors() in favour of your
> spice_qxl_set_device_info() variant.
>
> spice_qxl_set_max_monitors() would then basically do this:
>
> spice_qxl_set_max_monitors(qxl, max)
> {
> spice_qxl_set_device_info(qxl, NULL, 0, max);
> }
I can't actually do this, it does the wrong thing for the one-channel-
per-head (virtio-gpu) case. For that case it would send all
device_display_ids to 0 on all interfaces, but they need to be
different numbers.
For backwards compatibility when spice_qxl_set_device_info() is not
called I think we need to fallback to the old behaviour, as we are
missing the necessary information.
I'll send v3 with a single function using start + count and deprecate
the set_max_monitors one.
Cheers,
Lukas
> cheers,
> Gerd
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > 2. Have a single function as follows:
> > >
> > > void spice_qxl_set_device_info(QXLInstance *instance,
> > >const char *device_address,
> > >uint32_t device_display_id_start,
> > >uint32_t device_display_id_count);
> >
> > How about:
> >
> > void spice_qxl_set_device_info(QXLInstance *instance,
> >const char *device_address,
> >uint32_t device_display_id);
> >
> > I don't think we need start+count:
> >
> > * For single-head devices device_display_id will be zero.
> > * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> >device_display_id will enumerate the heads (so everybody can figure
> >which channel is which head).
> > * For one-channel-per-device multihead devices (i.e. qxl/linux)
> >device_display_id will be zero too. Number of heads is set via
> >spice_qxl_set_max_monitors().
>
> That requires nontrivial and unexpected logic for the one-channel-per-
> device multihead devices case. The API should be doing what it says and
> the dumber the better, this seems too smart to me...
Well, the device_display_id_count argument is redundant with
spice_qxl_set_max_monitors(). That isn't a great API either.
I can see that it simplifies the logic in spice-server if we have a
single function call instead of two. So we could deprecate
spice_qxl_set_max_monitors() in favour of your
spice_qxl_set_device_info() variant.
spice_qxl_set_max_monitors() would then basically do this:
spice_qxl_set_max_monitors(qxl, max)
{
spice_qxl_set_device_info(qxl, NULL, 0, max);
}
cheers,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote: > > > 2. Have a single function as follows: > > > > > > void spice_qxl_set_device_info(QXLInstance *instance, > > >const char *device_address, > > >uint32_t device_display_id_start, > > >uint32_t device_display_id_count); > > > > How about: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id); > > > > I don't think we need start+count: > > > > * For single-head devices device_display_id will be zero. > > * For one-channel-per-head multihead devices (i.e. virtio-gpu) > >device_display_id will enumerate the heads (so everybody can figure > >which channel is which head). > > * For one-channel-per-device multihead devices (i.e. qxl/linux) > >device_display_id will be zero too. Number of heads is set via > >spice_qxl_set_max_monitors(). > > That requires nontrivial and unexpected logic for the one-channel-per- > device multihead devices case. The API should be doing what it says and > the dumber the better, this seems too smart to me... > > That said, I don't find it significantly worse than the other options > (none of which seems great), so I'd just like we reached some consesus > and be done with it... > > Cheers, > Lukas > > > cheers, > > Gerd > > Personally I prefer the single function API, either with or without count. About the not trivial for compatibility you'll have to support Qemu versions that either: - does not call new APIs or spice_qxl_set_max_monitors. A QXL card will have the maximum possible number of monitors in the guest; - does not call new APIs but call spice_qxl_set_max_monitors. A QXL card will have a maximum of monitor specified by spice_qxl_set_max_monitors in the guest; - call new APIs (the choice in this case to call spice_qxl_set_max_monitors or not is yours). Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote: > > 2. Have a single function as follows: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id_start, > >uint32_t device_display_id_count); > > How about: > > void spice_qxl_set_device_info(QXLInstance *instance, >const char *device_address, >uint32_t device_display_id); > > I don't think we need start+count: > > * For single-head devices device_display_id will be zero. > * For one-channel-per-head multihead devices (i.e. virtio-gpu) >device_display_id will enumerate the heads (so everybody can figure >which channel is which head). > * For one-channel-per-device multihead devices (i.e. qxl/linux) >device_display_id will be zero too. Number of heads is set via >spice_qxl_set_max_monitors(). That requires nontrivial and unexpected logic for the one-channel-per- device multihead devices case. The API should be doing what it says and the dumber the better, this seems too smart to me... That said, I don't find it significantly worse than the other options (none of which seems great), so I'd just like we reached some consesus and be done with it... Cheers, Lukas > cheers, > Gerd > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
On Mon, Nov 05, 2018 at 03:46:37AM -0500, Frediano Ziglio wrote: > > > > > 2. Have a single function as follows: > > > > > > void spice_qxl_set_device_info(QXLInstance *instance, > > >const char *device_address, > > >uint32_t device_display_id_start, > > >uint32_t device_display_id_count); > > > > How about: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id); > > > > I don't think we need start+count: > > > > * For single-head devices device_display_id will be zero. > > * For one-channel-per-head multihead devices (i.e. virtio-gpu) > >device_display_id will enumerate the heads (so everybody can figure > >which channel is which head). > > * For one-channel-per-device multihead devices (i.e. qxl/linux) > >device_display_id will be zero too. Number of heads is set via > >spice_qxl_set_max_monitors(). > > > > cheers, > > Gerd > > > > What about "Console VNC" case? Passing a dummy (like -1) value for > device_display_id ? > Kind of "I don't know which output is." I'd use zero, assuming in case only one of multiple heads shows up as vnc console it'll be the primary head. But using -1 kind-of makes sense too. cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
On Mon, 2018-11-05 at 03:46 -0500, Frediano Ziglio wrote: > > > > > 2. Have a single function as follows: > > > > > > void spice_qxl_set_device_info(QXLInstance *instance, > > >const char *device_address, > > >uint32_t device_display_id_start, > > >uint32_t device_display_id_count); > > > > How about: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id); > > > > I don't think we need start+count: > > > > * For single-head devices device_display_id will be zero. > > * For one-channel-per-head multihead devices (i.e. virtio-gpu) > >device_display_id will enumerate the heads (so everybody can figure > >which channel is which head). > > * For one-channel-per-device multihead devices (i.e. qxl/linux) > >device_display_id will be zero too. Number of heads is set via > >spice_qxl_set_max_monitors(). > > > > cheers, > > Gerd > > > > What about "Console VNC" case? Passing a dummy (like -1) value for > device_display_id ? > Kind of "I don't know which output is." I'm not sure -1 will be practically helpful for anything? What will happen if we use 0 for it? We still aren't sure how we want to handle the VNC console anyway, should we wait untill we know? Maybe I should use int32_t instead of uint32_t to have the option to use -1 if we need it later? Cheers, Lukas > Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> > > 2. Have a single function as follows: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id_start, > >uint32_t device_display_id_count); > > How about: > > void spice_qxl_set_device_info(QXLInstance *instance, >const char *device_address, >uint32_t device_display_id); > > I don't think we need start+count: > > * For single-head devices device_display_id will be zero. > * For one-channel-per-head multihead devices (i.e. virtio-gpu) >device_display_id will enumerate the heads (so everybody can figure >which channel is which head). > * For one-channel-per-device multihead devices (i.e. qxl/linux) >device_display_id will be zero too. Number of heads is set via >spice_qxl_set_max_monitors(). > > cheers, > Gerd > What about "Console VNC" case? Passing a dummy (like -1) value for device_display_id ? Kind of "I don't know which output is." Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> 2. Have a single function as follows: > > void spice_qxl_set_device_info(QXLInstance *instance, >const char *device_address, >uint32_t device_display_id_start, >uint32_t device_display_id_count); How about: void spice_qxl_set_device_info(QXLInstance *instance, const char *device_address, uint32_t device_display_id); I don't think we need start+count: * For single-head devices device_display_id will be zero. * For one-channel-per-head multihead devices (i.e. virtio-gpu) device_display_id will enumerate the heads (so everybody can figure which channel is which head). * For one-channel-per-device multihead devices (i.e. qxl/linux) device_display_id will be zero too. Number of heads is set via spice_qxl_set_max_monitors(). cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
Hello,
On Mon, 2018-10-22 at 13:46 +0200, Lukáš Hrázký wrote:
> Hello,
>
> On Thu, 2018-10-18 at 03:16 -0400, Frediano Ziglio wrote:
> > >
> > > Adds two functions to let QEMU provide information to identify graphics
> > > devices and their monitors in the guest:
> > >
> > > * device address - The path identifying the device on the system (e.g. PCI
> > > path):
> > > spice_qxl_set_device_address(...)
> > >
> > > * device display ID - The index of the monitor on the graphics device:
> > > spice_qxl_monitor_set_device_display_id(...)
> >
> > This seems to indicate that this device is bound in some way to the previous
> > information but having 2 APIs make more fragile, potentially one could
> > call a function and not the other or in different order or mismatch them.
>
> Not sure what you mean by "previous information".
>
> Yes, you need to call both functions... The order doesn't matter and
> you can always send wrong data in a single function as well. I agree in
> this regard a single function would be better but don't find it a big
> issue. Note that the functions are called in different places in QEMU
> too.
>
> ...
>
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > > *device_address)
> > > +{
> > >
> > > ...
> > >
> > > + */
> > > +SPICE_GNUC_VISIBLE
> > > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> > > + uint32_t monitor_id,
> > > + uint32_t device_display_id)
> >
> > I still don't understand why, as suggested by Gerd, we need another function
> > instead of 2 additional parameters to the above API specifying start and
> > number, this API looks much more prone to errors.
>
> Possibly. I (and I think Jonathon too?) didn't like the start and
> number API, this was proposed as an alternative.
this topic of the API functions should be resolved. I think the options
are:
1. Keep the two functions proposed in this patch:
void spice_qxl_set_device_address(QXLInstance *instance,
const char *device_address);
void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
uint32_t monitor_id,
uint32_t device_display_id);
2. Have a single function as follows:
void spice_qxl_set_device_info(QXLInstance *instance,
const char *device_address,
uint32_t device_display_id_start,
uint32_t device_display_id_count);
The semantics of the single function in 2. (with a sort-of placeholder
name) would be setting the device_address for the QXLInstance as well
as setting a mapping of monitor IDs to device display IDs of
[0, count-1] -> [start, start+count-1].
The usage of 1. in QEMU is what's in patch 2/2. The usage of 2. would
mean calling the new single function in the two places where
spice_qxl_monitor_set_device_display_id() is now called in patch 2/2.
The code that composes the device_address would need to be put in a
function somewhere in QEMU (not sure where yet) that can then be called
from the two places where it's needed.
I'm still somewhat more inclined to keep the two functions, as their
API seems a bit cleaner to me and you can call/not call a single
function in a wrong way too. Frediano prefers a single function.
Jonathon, Gerd?
Thanks,
Lukas
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
Hello,
On Thu, 2018-10-18 at 03:16 -0400, Frediano Ziglio wrote:
> >
> > Adds two functions to let QEMU provide information to identify graphics
> > devices and their monitors in the guest:
> >
> > * device address - The path identifying the device on the system (e.g. PCI
> > path):
> > spice_qxl_set_device_address(...)
> >
> > * device display ID - The index of the monitor on the graphics device:
> > spice_qxl_monitor_set_device_display_id(...)
>
> This seems to indicate that this device is bound in some way to the previous
> information but having 2 APIs make more fragile, potentially one could
> call a function and not the other or in different order or mismatch them.
Not sure what you mean by "previous information".
Yes, you need to call both functions... The order doesn't matter and
you can always send wrong data in a single function as well. I agree in
this regard a single function would be better but don't find it a big
issue. Note that the functions are called in different places in QEMU
too.
> >
> > Signed-off-by: Lukáš Hrázký
> > ---
> > server/red-qxl.c | 89
> > server/spice-qxl.h | 5 +++
> > server/spice-server.syms | 6 +++
> > 3 files changed, 100 insertions(+)
> >
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..0b2043e1 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> > #include "red-qxl.h"
> >
> >
> > +#define MAX_PATH_LEN 256
>
> Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
> confusing, maybe MAX_DEVICE_PATH_LEN would be better.
Will do.
> > +#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_PATH_LEN];
> > +uint32_t device_display_ids[MAX_MONITORS_COUNT];
> > +size_t monitors_count; // length of ^^^
>
> size_t look a bit too much for a number of item in a small array.
It's the go-to type for indexes for me, I'm not used to doing these
little optimizations. If you think it matters, I can change it to like
uint8_t...
> >
> > pthread_mutex_t scanout_mutex;
> > SpiceMsgDisplayGlScanoutUnix scanout;
> > @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
> > red_qxl_async_complete(qxl, cookie);
> > }
> >
> > +/**
> > + * spice_qxl_set_device_address:
> > + * @instance the QXL instance to set the path to
> > + * @device_address the path of the device this QXL instance belongs to
> > + *
> > + * Sets the hardware address (e.g. PCI) of the graphics device represented
> > by
> > + * this QXL interface instance.
> > + *
> > + * The supported format is:
> > + * "pci//./.../."
> > + *
> > + * The "pci" identifies the rest of the string as a PCI adress. It is the
> > only
>
> typo: adress
>
> > + * supported address at the moment, other identifiers can be introduced
> > later.
> > + * is the PCI domain, followed by . of any PCI
> > bridges
> > + * in the chain leading to the device. The last . is the
> > + * graphics device.
>
> Maybe better to specify also the encoding, like decimal/hexadecimal and number
> of digits.
Right, I'll improve on that.
> > + */
>
> I would prefer documentation in the header so people don't have to open
> the source to get it, also considering the the header is public.
Sure, makes sense. I just did it the same way as it's done in spice-
gtk, which has the docstrings in .c files for some reason...
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_set_device_address(QXLInstance *instance, const char
> > *device_address)
> > +{
> > +g_return_if_fail(device_address != NULL);
> > +
> > +size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
> > +if (dp_len >= MAX_PATH_LEN) {
> > +spice_error("PCI device path too long: %lu > %u", dp_len,
> > MAX_PATH_LEN);
> > +return;
> > +}
> > +
> > +strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
> > +
> > +g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> > device_address);
> > +}
> > +
> > +/**
> > + * spice_qxl_monitor_set_device_display_id:
> > + * @instance the QXL instance to set the device display ID to
> > + * @monitor_id the SPICE monitor ID to set the device display ID to
> > + * @device_display_id the actual ID of the display (output) on the graphics
> > device
> > + *
> > + * Sets the device display ID for a given monitor ID in a QXL instance. The
> > + * monitor IDs are expected and required to be a consecutive sequence
> > starting
> > + * at 0. The function requires the calls to be made in the sequence to
> > prevent
> > + * holes.
> > + *
> > + * The requirement for the monitor ID to be a sequence starting from 0
> > comes
> > + * from the mechanism of generating a single display_id
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> > + * supported address at the moment, other identifiers can be introduced > > later. > > + * is the PCI domain, followed by . of any PCI > > bridges > > + * in the chain leading to the device. The last . is the > > + * graphics device. > > Maybe better to specify also the encoding, like decimal/hexadecimal and number > of digits. lspci style (i.e. slot is two digits hex, function is one digit). > > +/** > > + * spice_qxl_monitor_set_device_display_id: > > + * @instance the QXL instance to set the device display ID to > > + * @monitor_id the SPICE monitor ID to set the device display ID to > > + * @device_display_id the actual ID of the display (output) on the graphics > > device Hmm, why do we need the monitor_id here? The reason to have this function is to allow the guest agent figure which channel is which head in case we have multiple display channels registered for one device. For linux-qxl multihead we don't need this as all heads use the same display channel, right? > > +SPICE_GNUC_VISIBLE > > +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance, > > + uint32_t monitor_id, > > + uint32_t device_display_id) > > I still don't understand why, as suggested by Gerd, we need another function > instead of 2 additional parameters to the above API specifying start and > number, this API looks much more prone to errors. We can also just add a device_display_id parameter to the other function, that should work too. > Also there's no much documentation for this "device display ID" in the code, > potentially can be generated with something like: It's the head index, starting at zero. cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
>
> Adds two functions to let QEMU provide information to identify graphics
> devices and their monitors in the guest:
>
> * device address - The path identifying the device on the system (e.g. PCI
> path):
> spice_qxl_set_device_address(...)
>
> * device display ID - The index of the monitor on the graphics device:
> spice_qxl_monitor_set_device_display_id(...)
This seems to indicate that this device is bound in some way to the previous
information but having 2 APIs make more fragile, potentially one could
call a function and not the other or in different order or mismatch them.
>
> Signed-off-by: Lukáš Hrázký
> ---
> server/red-qxl.c | 89
> server/spice-qxl.h | 5 +++
> server/spice-server.syms | 6 +++
> 3 files changed, 100 insertions(+)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0b2043e1 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
> #include "red-qxl.h"
>
>
> +#define MAX_PATH_LEN 256
Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
confusing, maybe MAX_DEVICE_PATH_LEN would be better.
> +#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_PATH_LEN];
> +uint32_t device_display_ids[MAX_MONITORS_COUNT];
> +size_t monitors_count; // length of ^^^
size_t look a bit too much for a number of item in a small array.
>
> pthread_mutex_t scanout_mutex;
> SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
> red_qxl_async_complete(qxl, cookie);
> }
>
> +/**
> + * spice_qxl_set_device_address:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs to
> + *
> + * Sets the hardware address (e.g. PCI) of the graphics device represented
> by
> + * this QXL interface instance.
> + *
> + * The supported format is:
> + * "pci//./.../."
> + *
> + * The "pci" identifies the rest of the string as a PCI adress. It is the
> only
typo: adress
> + * supported address at the moment, other identifiers can be introduced
> later.
> + * is the PCI domain, followed by . of any PCI
> bridges
> + * in the chain leading to the device. The last . is the
> + * graphics device.
Maybe better to specify also the encoding, like decimal/hexadecimal and number
of digits.
> + */
I would prefer documentation in the header so people don't have to open
the source to get it, also considering the the header is public.
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_address(QXLInstance *instance, const char
> *device_address)
> +{
> +g_return_if_fail(device_address != NULL);
> +
> +size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
> +if (dp_len >= MAX_PATH_LEN) {
> +spice_error("PCI device path too long: %lu > %u", dp_len,
> MAX_PATH_LEN);
> +return;
> +}
> +
> +strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
> +
> +g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> device_address);
> +}
> +
> +/**
> + * spice_qxl_monitor_set_device_display_id:
> + * @instance the QXL instance to set the device display ID to
> + * @monitor_id the SPICE monitor ID to set the device display ID to
> + * @device_display_id the actual ID of the display (output) on the graphics
> device
> + *
> + * Sets the device display ID for a given monitor ID in a QXL instance. The
> + * monitor IDs are expected and required to be a consecutive sequence
> starting
> + * at 0. The function requires the calls to be made in the sequence to
> prevent
> + * holes.
> + *
> + * The requirement for the monitor ID to be a sequence starting from 0 comes
> + * from the mechanism of generating a single display_id from channel_id and
> + * monitor_id on the client:
> + *
> + * display_id = channel_id + monitor_id
> + *
No, the monitor_id sequence is defined as a sequence from 0, has nothing
to do with display_id. Also this comment seems to indicate that this new
interface is bad designed having the same limit, which is not.
> + * This is unambiguous either if there is only a single channel with
> multiple
> + * monitors ("legacy" QXL on linux case) or multiple channels with only a
> + * single monitor. Also both channel_id and monitor_id need to be a sequence
> + * starting from 0, otherwise there is still a possibility of collisions.
They are both defined (channel_id and monitor_id) as sequences starting from 0.
I don't see why need to be specified here.
> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> + uint32_t monitor_id,
> +
