Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Fri, Oct 12, 2018 at 06:46:37AM -0400, Frediano Ziglio wrote:
> >
> > Hi,
> >
> > > > When using qemu_console_get_head() it doesn't work correctly, it would
> > > > use the qxl card's data. It would work if spice-server would filter the
> > > > list to only include the entries for the given display channel before
> > > > calling the ->client_monitors_config() callback. But it doesn't, we get
> > > > the complete set.
> > >
> > > That's why I said is a bug, IMHO it should.
> >
> > Hmm, this should be easily detectable on qemu side I think. If
> > num_of_monitors (for a non-qxl card) is > 1, then we have an old,
> > non-filtering spice-server. If num_of_monitors == 1, then it is a new,
> > filtering spice-server. Or a single-head channel configuration, in
> > which case it doesn't matter whenever it filters or not.
> >
> > So this should be fixable without causing too much compatibility pain.
> >
> > cheers,
> > Gerd
> >
>
> Sorry, why fixing in Qemu is the bug is in spice-server?
> I really don't follow your reasoning.
I had a thinko in one of the previous messages.
qemu_console_get_head() is never correct, so qemu is buggy too.
It should be either qemu_console_get_index() (equals channel id), for
the non-filtering case, or just 0, for the filtering case, because every
virtio-gpu head has its own display channel. Patch below.
> For me Qemu should not change at all and spice-server should
> do the filtering. Is this not fixing everything?
Well, fixed qemu being able to deal with all spice-server versions
(fixed and unifixed) is certainly useful to reduce the damage caused
by this incompatible change.
The qxl version of that callback can stay as-is, the spice-server side
fix should make sure the current code continues to work when we start
supporting new configurations (qxl and non-qxl devices mixed).
cheers,
Gerd
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 2f8adb6b9f..52f8cb5ae1 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -674,10 +674,28 @@ static int
interface_client_monitors_config(QXLInstance *sin,
memset(, 0, sizeof(info));
-head = qemu_console_get_head(ssd->dcl.con);
-if (mc->num_of_monitors > head) {
-info.width = mc->monitors[head].width;
-info.height = mc->monitors[head].height;
+if (mc->num_of_monitors == 1) {
+/*
+ * New spice-server version which filters the list of monitors
+ * to only include those that belong to our display channel.
+ *
+ * single-head configuration (where filtering doesn't matter)
+ * takes this code path too.
+ */
+info.width = mc->monitors[0].width;
+info.height = mc->monitors[0].height;
+} else {
+/*
+ * Old spice-server which gives us all monitors, so we have to
+ * figure ourself which entry we need. Array index is the
+ * channel_id, which is the qemu console index, see
+ * qemu_spice_add_display_interface().
+ */
+head = qemu_console_get_index(ssd->dcl.con);
+if (mc->num_of_monitors > head) {
+info.width = mc->monitors[head].width;
+info.height = mc->monitors[head].height;
+}
}
trace_qemu_spice_ui_info(ssd->qxl.id, info.width, info.height);
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > Hi, > > > > When using qemu_console_get_head() it doesn't work correctly, it would > > > use the qxl card's data. It would work if spice-server would filter the > > > list to only include the entries for the given display channel before > > > calling the ->client_monitors_config() callback. But it doesn't, we get > > > the complete set. > > > > That's why I said is a bug, IMHO it should. > > Hmm, this should be easily detectable on qemu side I think. If > num_of_monitors (for a non-qxl card) is > 1, then we have an old, > non-filtering spice-server. If num_of_monitors == 1, then it is a new, > filtering spice-server. Or a single-head channel configuration, in > which case it doesn't matter whenever it filters or not. > > So this should be fixable without causing too much compatibility pain. > > cheers, > Gerd > Sorry, why fixing in Qemu is the bug is in spice-server? I really don't follow your reasoning. For me Qemu should not change at all and spice-server should do the filtering. Is this not fixing everything? Frediano
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
Hi, > > When using qemu_console_get_head() it doesn't work correctly, it would > > use the qxl card's data. It would work if spice-server would filter the > > list to only include the entries for the given display channel before > > calling the ->client_monitors_config() callback. But it doesn't, we get > > the complete set. > > That's why I said is a bug, IMHO it should. Hmm, this should be easily detectable on qemu side I think. If num_of_monitors (for a non-qxl card) is > 1, then we have an old, non-filtering spice-server. If num_of_monitors == 1, then it is a new, filtering spice-server. Or a single-head channel configuration, in which case it doesn't matter whenever it filters or not. So this should be fixable without causing too much compatibility pain. cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote: > > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote: > > > > > Ok. We probably should fix interface_client_monitors_config() to use > > > > > the channel_id instead of qemu_console_get_head() then. > > > > > > > > It's not that simple. This would break the QXL with multiple monitors > > > > per channel case. > > > > > > It is that simple. > > > > > > qxl doesn't use that code path and has its own version of the callback > > > (in qxl.c). Fixing it there is a bit more tricky. > > > > Ok.. and what's actually the problem using qemu_console_get_head()? It > > just doesn't feel right to me using channel_id as an index into this > > array. It is not the right index strictly speaking. > > Assume you have one qxl and one virtio-gpu device (one head each), for > example: > >00:02.0 qxl channel 0 >00:03.0 virtio-gpu channel 1 > > So the client will assign display 0 to qxl and display 1 to virtio-gpu. > In interface_client_monitors_config() we have to pick the correct array > entry. > > When using the channel_id it works correctly. > > When using qemu_console_get_head() it doesn't work correctly, it would > use the qxl card's data. It would work if spice-server would filter the > list to only include the entries for the given display channel before > calling the ->client_monitors_config() callback. But it doesn't, we get > the complete set. > That's why I said is a bug, IMHO it should. > > > > I think we should fix spice server to actually do the filtering and > > > > only send monitors_config that belongs to the device to the QXL > > > > interface. As Frediano mentioned, it looks more like a bug. > > > > > > Only problem is changing the callback semantics changes the library api. > > > We could add a second version of the callback which gets called with a > > > filtered list (if present). Not sure this is worth the effort though. > > > > That's right. But if we don't actually break any currently supported > > use cases, it might be fine? The only thing we would be breaking is > > the virtio-gpu, I think? Is that already something we don't want to > > break? > > It would break any multihead configuration which uses multiple display > channels. Yes, virtio-gpu is one case. Breaking that would not be very > nice, but maybe acceptable. The other case is multiple qxl devices > (i.e. windows guest style multihead). Breaking that is out of > question. > Windows is not a problem as it ignores the data passed to client_monitors_config. > cheers, > Gerd > Frediano
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Fri, 2018-10-12 at 11:27 +0200, Gerd Hoffmann wrote: > On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote: > > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote: > > > > > Ok. We probably should fix interface_client_monitors_config() to use > > > > > the channel_id instead of qemu_console_get_head() then. > > > > > > > > It's not that simple. This would break the QXL with multiple monitors > > > > per channel case. > > > > > > It is that simple. > > > > > > qxl doesn't use that code path and has its own version of the callback > > > (in qxl.c). Fixing it there is a bit more tricky. > > > > Ok.. and what's actually the problem using qemu_console_get_head()? It > > just doesn't feel right to me using channel_id as an index into this > > array. It is not the right index strictly speaking. > > Assume you have one qxl and one virtio-gpu device (one head each), for > example: > >00:02.0 qxl channel 0 >00:03.0 virtio-gpu channel 1 > > So the client will assign display 0 to qxl and display 1 to virtio-gpu. > In interface_client_monitors_config() we have to pick the correct array > entry. > > When using the channel_id it works correctly. > > When using qemu_console_get_head() it doesn't work correctly, it would > use the qxl card's data. It would work if spice-server would filter the > list to only include the entries for the given display channel before > calling the ->client_monitors_config() callback. But it doesn't, we get > the complete set. > > > > > I think we should fix spice server to actually do the filtering and > > > > only send monitors_config that belongs to the device to the QXL > > > > interface. As Frediano mentioned, it looks more like a bug. > > > > > > Only problem is changing the callback semantics changes the library api. > > > We could add a second version of the callback which gets called with a > > > filtered list (if present). Not sure this is worth the effort though. > > > > That's right. But if we don't actually break any currently supported > > use cases, it might be fine? The only thing we would be breaking is > > the virtio-gpu, I think? Is that already something we don't want to > > break? > > It would break any multihead configuration which uses multiple display > channels. Yes, virtio-gpu is one case. Breaking that would not be very > nice, but maybe acceptable. The other case is multiple qxl devices > (i.e. windows guest style multihead). Breaking that is out of > question. The windows QXL driver doesn't support the monitors_config callback, at least that's what Frediano said here: https://lists.freedesktop.org/archives/spice-devel/2018-October/045965.html So I don't think this is a problem and if breaking virtio-gpu would be acceptable, we should probably consider fixing the interface... Cheers, Lukas > cheers, > Gerd >
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote: > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote: > > > > Ok. We probably should fix interface_client_monitors_config() to use > > > > the channel_id instead of qemu_console_get_head() then. > > > > > > It's not that simple. This would break the QXL with multiple monitors > > > per channel case. > > > > It is that simple. > > > > qxl doesn't use that code path and has its own version of the callback > > (in qxl.c). Fixing it there is a bit more tricky. > > Ok.. and what's actually the problem using qemu_console_get_head()? It > just doesn't feel right to me using channel_id as an index into this > array. It is not the right index strictly speaking. Assume you have one qxl and one virtio-gpu device (one head each), for example: 00:02.0 qxl channel 0 00:03.0 virtio-gpu channel 1 So the client will assign display 0 to qxl and display 1 to virtio-gpu. In interface_client_monitors_config() we have to pick the correct array entry. When using the channel_id it works correctly. When using qemu_console_get_head() it doesn't work correctly, it would use the qxl card's data. It would work if spice-server would filter the list to only include the entries for the given display channel before calling the ->client_monitors_config() callback. But it doesn't, we get the complete set. > > > I think we should fix spice server to actually do the filtering and > > > only send monitors_config that belongs to the device to the QXL > > > interface. As Frediano mentioned, it looks more like a bug. > > > > Only problem is changing the callback semantics changes the library api. > > We could add a second version of the callback which gets called with a > > filtered list (if present). Not sure this is worth the effort though. > > That's right. But if we don't actually break any currently supported > use cases, it might be fine? The only thing we would be breaking is > the virtio-gpu, I think? Is that already something we don't want to > break? It would break any multihead configuration which uses multiple display channels. Yes, virtio-gpu is one case. Breaking that would not be very nice, but maybe acceptable. The other case is multiple qxl devices (i.e. windows guest style multihead). Breaking that is out of question. cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote: > > > Ok. We probably should fix interface_client_monitors_config() to use > > > the channel_id instead of qemu_console_get_head() then. > > > > It's not that simple. This would break the QXL with multiple monitors > > per channel case. > > It is that simple. > > qxl doesn't use that code path and has its own version of the callback > (in qxl.c). Fixing it there is a bit more tricky. Ok.. and what's actually the problem using qemu_console_get_head()? It just doesn't feel right to me using channel_id as an index into this array. It is not the right index strictly speaking. > > I think we should fix spice server to actually do the filtering and > > only send monitors_config that belongs to the device to the QXL > > interface. As Frediano mentioned, it looks more like a bug. > > Only problem is changing the callback semantics changes the library api. > We could add a second version of the callback which gets called with a > filtered list (if present). Not sure this is worth the effort though. That's right. But if we don't actually break any currently supported use cases, it might be fine? The only thing we would be breaking is the virtio-gpu, I think? Is that already something we don't want to break? > > > > A bit differently, as I said, but the configs are merged on the client, > > > > which should be an equivalent outcome. > > > > > > Indeed. So the qemu_spice_gl_monitor_config() function is correct then. > > > > I don't follow you reasoning for this conclusion... Is it correct > > because it sends a single monitor in the monitors_config? > > Yes (again, qxl doesn't run this code path so it'll only see the > one-monitor-per-channel cases). > > > Why is this > > function needed for the opengl case and not for regular virtio-gpu > > case? > > Not fully sure why opengl needs this. Maybe because the texture can be > larger than the actual scanout (for padding reasons) so we need to > communicate the scanout rectangle. > > > > Another story is linking display channels to device heads, i.e. > > > virtio-gpu registers one display channel per head, each channel > > > registers the same device path of course, and now you need to > > > figure in the guest agent which xrandr output is which head. > > > > This is actually the reason for the > > spice_qxl_device_monitor_add_display_id(qxl, device_display_id) > > function (using this opportunity to merge the other email thread > > discussion into one). > > Ah, ok. We should *not* call this thing display_id then, that'll be > confusing. Or at very least rename the function to something like ... > > spice_qxl_monitor_add_device_display_id() > ^ don't split this > > ... to make more clear this is something else than the display_id. Ok. > > > Simplest way would be to require display channels being registered in > > > order, so the channel with the smallest channel_id is head 0, ... > > > > I don't like this, you once again rely on implicit information derived > > from the order of registration of the channels. We should make this > > explicit, so that it doesn't cause trouble in the future... > > Fine with me too. > > I'd suggest to split the patches, one for the path and one for the > device_display_id thing (or whatever else we call this to make it less > likely being confused with display_id, even though I don't have a good > suggestion offhand). Sure, no problem. > cheers, > Gerd >
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > Ok. We probably should fix interface_client_monitors_config() to use > > the channel_id instead of qemu_console_get_head() then. > > It's not that simple. This would break the QXL with multiple monitors > per channel case. It is that simple. qxl doesn't use that code path and has its own version of the callback (in qxl.c). Fixing it there is a bit more tricky. > I think we should fix spice server to actually do the filtering and > only send monitors_config that belongs to the device to the QXL > interface. As Frediano mentioned, it looks more like a bug. Only problem is changing the callback semantics changes the library api. We could add a second version of the callback which gets called with a filtered list (if present). Not sure this is worth the effort though. > > > A bit differently, as I said, but the configs are merged on the client, > > > which should be an equivalent outcome. > > > > Indeed. So the qemu_spice_gl_monitor_config() function is correct then. > > I don't follow you reasoning for this conclusion... Is it correct > because it sends a single monitor in the monitors_config? Yes (again, qxl doesn't run this code path so it'll only see the one-monitor-per-channel cases). > Why is this > function needed for the opengl case and not for regular virtio-gpu > case? Not fully sure why opengl needs this. Maybe because the texture can be larger than the actual scanout (for padding reasons) so we need to communicate the scanout rectangle. > > Another story is linking display channels to device heads, i.e. > > virtio-gpu registers one display channel per head, each channel > > registers the same device path of course, and now you need to > > figure in the guest agent which xrandr output is which head. > > This is actually the reason for the > spice_qxl_device_monitor_add_display_id(qxl, device_display_id) > function (using this opportunity to merge the other email thread > discussion into one). Ah, ok. We should *not* call this thing display_id then, that'll be confusing. Or at very least rename the function to something like ... spice_qxl_monitor_add_device_display_id() ^ don't split this ... to make more clear this is something else than the display_id. > > Simplest way would be to require display channels being registered in > > order, so the channel with the smallest channel_id is head 0, ... > > I don't like this, you once again rely on implicit information derived > from the order of registration of the channels. We should make this > explicit, so that it doesn't cause trouble in the future... Fine with me too. I'd suggest to split the patches, one for the path and one for the device_display_id thing (or whatever else we call this to make it less likely being confused with display_id, even though I don't have a good suggestion offhand). cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Thu, 2018-10-11 at 15:43 +0200, Gerd Hoffmann wrote: > Hi, > > > > How does all this monitors_config work currently in case multiple > > > display channels are present? > > > > > > There is the QXLInterface->client_monitors_config() callback. > > > > > > There is the spice_qxl_monitors_config_async() function. > > > > > > Both are linked to a display channel. The server/client messages go > > > through the main channel though ... > > > > Not really, it actually is like this: > > > >display channel > > server --> client > > > > > > main channel > > client --> server > > > > So the monitors_configs go each on its own display channel to the > > client, the client puts it all together and sends back an aggregated > > list on the main channel. > > Ok. > > > > So what happens if a message from the client arrives? Is that > > > broadcasted as-is to all display channels? The current qemu code > > > (interface_client_monitors_config in spice-display.c, which is used with > > > virtio-gpu) simply uses the head as index into the array, so it looks > > > like spice-server doesn't do any filtering here (like only filling in > > > the monitors which belong to the display channel). > > > > Correct, the spice server doesn't do any filtering and sends the whole > > monitors_config to all the interfaces... > > Ok. We probably should fix interface_client_monitors_config() to use > the channel_id instead of qemu_console_get_head() then. It's not that simple. This would break the QXL with multiple monitors per channel case. I think we should fix spice server to actually do the filtering and only send monitors_config that belongs to the device to the QXL interface. As Frediano mentioned, it looks more like a bug. > > > spice_qxl_monitors_config_async() is never called with virtio-gpu, > > > except when opengl is enabled. In that case qemu simply sets > > > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] > > > (see qemu_spice_gl_monitor_config in spice-display.c). > > > > As a side note, I have no idea about opengl. I got a slight impression > > from some earlier conversation that this is actually not used, but I > > don't really know. > > Not by default, but when virgl is enabled (-spice gl=on -vga virtio). > > > My wild guess at this moment is, that if you don't call > > spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't > > send any monitors config, a simple one containing a single monitor for > > the surface is created somewhere along the way. > > Probably. > > > > Which would be > > > ok in case spice-server merges the configs from all channels before > > > sending it off to the client. Not sure this actually happens ... > > > > A bit differently, as I said, but the configs are merged on the client, > > which should be an equivalent outcome. > > Indeed. So the qemu_spice_gl_monitor_config() function is correct then. I don't follow you reasoning for this conclusion... Is it correct because it sends a single monitor in the monitors_config? Why is this function needed for the opengl case and not for regular virtio-gpu case? > > > > Interstingly enough, that seems to be the ID we want to have in the > > > > device_display_id attribute. I expect (still need to look that up, I'm > > > > out of time right now) that for virtio-gpu the ID is a bit different, > > > > > > Keep in mind that multiple display devices don't really work right now, > > > and possibly we need to fix not only spice but qemu too. > > > > What exactly do you mean by multiple devices not working? > > qxl + vgpu (which is why we are discussing this). > qxl + virtio-gpu will have simliar issues I think. Ok, I see. > > > I don't think we need the monitors_id in qemu then, qemu can simply use > > > the channel_id (except for the legacy qxl case). > > > > Ok. Given the fact that the monitor_ids actually come from the driver, > > QEMU should actually know them already, right? > > Yes. > > > No need to pass them back anyway? (except for the virtio-gpu case, > > which doesn't send monitors_config and so a single monitor_id = 0 is > > deduced in spice server) > > When virtio-gpu sends monitors_config monitor_id will be zero too, > because we have one channel per monitor. Right. > Another story is linking display channels to device heads, i.e. > virtio-gpu registers one display channel per head, each channel > registers the same device path of course, and now you need to > figure in the guest agent which xrandr output is which head. This is actually the reason for the spice_qxl_device_monitor_add_display_id(qxl, device_display_id) function (using this opportunity to merge the other email thread discussion into one). So for example if you have 3 heads on a virtio-gpu device, and you call it on qxl instance id (i.e. channel_id) = 2, the device_display_id argument will be 2 (as it is the head ID) and in the current state (which I agree with
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > So, do we need this add display_id function at all? > > You mean the addition of IDs in display_id = channel_id + monitor_id? No, I mean spice_qxl_set_path() is enough, the other function is not needed. cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
Hi, > > How does all this monitors_config work currently in case multiple > > display channels are present? > > > > There is the QXLInterface->client_monitors_config() callback. > > > > There is the spice_qxl_monitors_config_async() function. > > > > Both are linked to a display channel. The server/client messages go > > through the main channel though ... > > Not really, it actually is like this: > >display channel > server --> client > > > main channel > client --> server > > So the monitors_configs go each on its own display channel to the > client, the client puts it all together and sends back an aggregated > list on the main channel. Ok. > > So what happens if a message from the client arrives? Is that > > broadcasted as-is to all display channels? The current qemu code > > (interface_client_monitors_config in spice-display.c, which is used with > > virtio-gpu) simply uses the head as index into the array, so it looks > > like spice-server doesn't do any filtering here (like only filling in > > the monitors which belong to the display channel). > > Correct, the spice server doesn't do any filtering and sends the whole > monitors_config to all the interfaces... Ok. We probably should fix interface_client_monitors_config() to use the channel_id instead of qemu_console_get_head() then. > > spice_qxl_monitors_config_async() is never called with virtio-gpu, > > except when opengl is enabled. In that case qemu simply sets > > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] > > (see qemu_spice_gl_monitor_config in spice-display.c). > > As a side note, I have no idea about opengl. I got a slight impression > from some earlier conversation that this is actually not used, but I > don't really know. Not by default, but when virgl is enabled (-spice gl=on -vga virtio). > My wild guess at this moment is, that if you don't call > spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't > send any monitors config, a simple one containing a single monitor for > the surface is created somewhere along the way. Probably. > > Which would be > > ok in case spice-server merges the configs from all channels before > > sending it off to the client. Not sure this actually happens ... > > A bit differently, as I said, but the configs are merged on the client, > which should be an equivalent outcome. Indeed. So the qemu_spice_gl_monitor_config() function is correct then. > > > Interstingly enough, that seems to be the ID we want to have in the > > > device_display_id attribute. I expect (still need to look that up, I'm > > > out of time right now) that for virtio-gpu the ID is a bit different, > > > > Keep in mind that multiple display devices don't really work right now, > > and possibly we need to fix not only spice but qemu too. > > What exactly do you mean by multiple devices not working? qxl + vgpu (which is why we are discussing this). qxl + virtio-gpu will have simliar issues I think. > > I don't think we need the monitors_id in qemu then, qemu can simply use > > the channel_id (except for the legacy qxl case). > > Ok. Given the fact that the monitor_ids actually come from the driver, > QEMU should actually know them already, right? Yes. > No need to pass them back anyway? (except for the virtio-gpu case, > which doesn't send monitors_config and so a single monitor_id = 0 is > deduced in spice server) When virtio-gpu sends monitors_config monitor_id will be zero too, because we have one channel per monitor. Another story is linking display channels to device heads, i.e. virtio-gpu registers one display channel per head, each channel registers the same device path of course, and now you need to figure in the guest agent which xrandr output is which head. Simplest way would be to require display channels being registered in order, so the channel with the smallest channel_id is head 0, ... cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Thu, 2018-10-11 at 15:20 +0200, Gerd Hoffmann wrote: > > The plan was (and still is) to limit the use cases to the following > > two: > > > > * Legacy QXL on linux with multiple monitors per display channel, but > > only this single display channel. Multiple display channels are not > > supported in this case, so no streaming etc. > > > > * Limit the number of monitors per display channel to one for all other > > cases. > > Right, I mis-remembered. > > > With these limitations, the display_id = channel_id + monitor_id > > formula that is used on the client remains unique. > > And in case no qxl card is in use we even have display_id == channel_id. > > So, do we need this add display_id function at all? You mean the addition of IDs in display_id = channel_id + monitor_id? That's what is in the clients now, at least in remote_viewer and spicy, although in remote_viewer it's display_id = channel_id ? channel_id : monitor_id, which is equivalent if either of the numbers (or both) is always zero. We also still need to handle the "legacy" (it's still the majority in usage on linux though...) QXL case, and this formula works for both cases, so I'm not sure it's worth trying to replace it with something. Although we _should_ try to clean this mess up eventually... Cheers, Lukas > cheers, > Gerd >
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> The plan was (and still is) to limit the use cases to the following > two: > > * Legacy QXL on linux with multiple monitors per display channel, but > only this single display channel. Multiple display channels are not > supported in this case, so no streaming etc. > > * Limit the number of monitors per display channel to one for all other > cases. Right, I mis-remembered. > With these limitations, the display_id = channel_id + monitor_id > formula that is used on the client remains unique. And in case no qxl card is in use we even have display_id == channel_id. So, do we need this add display_id function at all? cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Thu, 2018-10-11 at 14:27 +0200, Gerd Hoffmann wrote: > > > So, if I remember correctly, Gerd recommended returning this value from > > > the function. But I think it needs more explanation here. What exactly > > > is a "monitor_id" supposed to represent? It is not used in your follow- > > > up qemu patch so it's hard to tell. > > > > It's supposed to be the actual monitor_id that we use in SPICE to > > identify the monitors. I've just spent quite some time looking up where > > the monitor_id actually comes from, and it's actually set all the way > > down in the driver (either xf86-video-qxl or the KMS driver in the > > kernel) and passed through the monitors_config functions to SPICE > > server. > > How does all this monitors_config work currently in case multiple > display channels are present? > > There is the QXLInterface->client_monitors_config() callback. > > There is the spice_qxl_monitors_config_async() function. > > Both are linked to a display channel. The server/client messages go > through the main channel though ... Not really, it actually is like this: display channel server --> client main channel client --> server So the monitors_configs go each on its own display channel to the client, the client puts it all together and sends back an aggregated list on the main channel. > So what happens if a message from the client arrives? Is that > broadcasted as-is to all display channels? The current qemu code > (interface_client_monitors_config in spice-display.c, which is used with > virtio-gpu) simply uses the head as index into the array, so it looks > like spice-server doesn't do any filtering here (like only filling in > the monitors which belong to the display channel). Correct, the spice server doesn't do any filtering and sends the whole monitors_config to all the interfaces... > spice_qxl_monitors_config_async() is never called with virtio-gpu, > except when opengl is enabled. In that case qemu simply sets > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] > (see qemu_spice_gl_monitor_config in spice-display.c). As a side note, I have no idea about opengl. I got a slight impression from some earlier conversation that this is actually not used, but I don't really know. My wild guess at this moment is, that if you don't call spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't send any monitors config, a simple one containing a single monitor for the surface is created somewhere along the way. > Which would be > ok in case spice-server merges the configs from all channels before > sending it off to the client. Not sure this actually happens ... A bit differently, as I said, but the configs are merged on the client, which should be an equivalent outcome. > > Interstingly enough, that seems to be the ID we want to have in the > > device_display_id attribute. I expect (still need to look that up, I'm > > out of time right now) that for virtio-gpu the ID is a bit different, > > Keep in mind that multiple display devices don't really work right now, > and possibly we need to fix not only spice but qemu too. What exactly do you mean by multiple devices not working? > > And yeah, I didn't use the id in the QEMU patches, as I didn't know > > how, I expect Gerd to have some grand plans for it :) > > IIRC the latest plan was to just keep things as is, plan with one > channel per monitor for all future things, and just not support > one-qxl-device-multihead in combination with multiple display channels. > > Is that correct? Correct. > I don't think we need the monitors_id in qemu then, qemu can simply use > the channel_id (except for the legacy qxl case). Ok. Given the fact that the monitor_ids actually come from the driver, QEMU should actually know them already, right? No need to pass them back anyway? (except for the virtio-gpu case, which doesn't send monitors_config and so a single monitor_id = 0 is deduced in spice server) Cheers, Lukas > cheers, > Gerd >
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > > > So, if I remember correctly, Gerd recommended returning this value from > > > the function. But I think it needs more explanation here. What exactly > > > is a "monitor_id" supposed to represent? It is not used in your follow- > > > up qemu patch so it's hard to tell. > > > > It's supposed to be the actual monitor_id that we use in SPICE to > > identify the monitors. I've just spent quite some time looking up where > > the monitor_id actually comes from, and it's actually set all the way > > down in the driver (either xf86-video-qxl or the KMS driver in the > > kernel) and passed through the monitors_config functions to SPICE > > server. > > How does all this monitors_config work currently in case multiple > display channels are present? > > There is the QXLInterface->client_monitors_config() callback. > > There is the spice_qxl_monitors_config_async() function. > > Both are linked to a display channel. The server/client messages go > through the main channel though ... > > So what happens if a message from the client arrives? Is that > broadcasted as-is to all display channels? The current qemu code > (interface_client_monitors_config in spice-display.c, which is used with > virtio-gpu) simply uses the head as index into the array, so it looks > like spice-server doesn't do any filtering here (like only filling in > the monitors which belong to the display channel). > Yes, it is a broadcast but is a bug that is working only because the message is handled only by Linux driver and in Linux configuration is expected that there is only a QXL card supporting multiple monitor (in this case the broadcast is equivalent to send only the monitor for the given QXL card). > spice_qxl_monitors_config_async() is never called with virtio-gpu, > except when opengl is enabled. In that case qemu simply sets > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] > (see qemu_spice_gl_monitor_config in spice-display.c). Which would be > ok in case spice-server merges the configs from all channels before > sending it off to the client. Not sure this actually happens ... > Yes, server send all the monitors together (or at least it should). So client sends and receives all the monitor sizes at once. > > Interstingly enough, that seems to be the ID we want to have in the > > device_display_id attribute. I expect (still need to look that up, I'm > > out of time right now) that for virtio-gpu the ID is a bit different, > > Keep in mind that multiple display devices don't really work right now, > and possibly we need to fix not only spice but qemu too. > What does not work at the moment? On Windows is normal to have multiple QXL cards and is working for instance. We use QXL card and vGPU and is not working that bad (beside the problem we are fixing here). > > And yeah, I didn't use the id in the QEMU patches, as I didn't know > > how, I expect Gerd to have some grand plans for it :) > > IIRC the latest plan was to just keep things as is, plan with one > channel per monitor for all future things, and just not support > one-qxl-device-multihead in combination with multiple display channels. > > Is that correct? > Well, I think depends form the definition of "things". As we are changing the code we are not "just keep things as is". But yes, for the future we plan to support only one monitor per channel so monitor_id == 0 (and so will be display_id == channel_id). I suppose by "things" in "just keep things as is" you mean the SPICE (client <-> server) protocol. > I don't think we need the monitors_id in qemu then, qemu can simply use > the channel_id (except for the legacy qxl case). > Yes, and the channel_id is provided by Qemu to spice-server (as id of the QXL interface) so there's no reason to ask to spice-server to provide back channel_id. > cheers, > Gerd > Frediano
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
Hi Gerd, On Wed, 2018-10-10 at 12:37 +0200, Gerd Hoffmann wrote: > Hi, > > > > + * Sets the hardware (e.g. PCI) path of the graphics device > > > represented by this QXL interface instance. > > > > So, this comment suggests that the caller might be able to provide a > > path that is not a PCI path. But the implementation below (mostly the > > debug output, I suppose...) assumes a PCI path. Do we need to support > > non-PCI paths? > > Certainly not for the initial revision, maybe never. > > But thanks to the "pci/" prefix we should be able to add support for > other paths later in case it turns out we need it. > > > > + * Returns: The actual SPICE server monitor_id associated with the > > > display > > > > So, if I remember correctly, Gerd recommended returning this value from > > the function. But I think it needs more explanation here. What exactly > > is a "monitor_id" supposed to represent? It is not used in your follow- > > up qemu patch so it's hard to tell. > > IIRC the plan was to ditch the global monior_id idea and use the > (channel_id, display_id) tuple everywhere ... Not sure what exactly you mean here by "global monitor_id". Not sure about "use the (channel_id, display_id)" either, it doesn't seem quite correct. The plan was (and still is) to limit the use cases to the following two: * Legacy QXL on linux with multiple monitors per display channel, but only this single display channel. Multiple display channels are not supported in this case, so no streaming etc. * Limit the number of monitors per display channel to one for all other cases. With these limitations, the display_id = channel_id + monitor_id formula that is used on the client remains unique. With one more condition, that I think I should add, and that is that monitor_id is always 0 for the multiple display channel case. It seems it may come up that the monitor_id could be non-zero, e.g. for the virtio-gpu case... So the IDs used are: monitors_config server -> client: (channel_id, monitor_id) monitors_config client -> server and possibly server -> vd_agent: display_id I hope it's clear like this :) Cheers, Lukas > cheers, > Gerd >
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > So, if I remember correctly, Gerd recommended returning this value from > > the function. But I think it needs more explanation here. What exactly > > is a "monitor_id" supposed to represent? It is not used in your follow- > > up qemu patch so it's hard to tell. > > It's supposed to be the actual monitor_id that we use in SPICE to > identify the monitors. I've just spent quite some time looking up where > the monitor_id actually comes from, and it's actually set all the way > down in the driver (either xf86-video-qxl or the KMS driver in the > kernel) and passed through the monitors_config functions to SPICE > server. How does all this monitors_config work currently in case multiple display channels are present? There is the QXLInterface->client_monitors_config() callback. There is the spice_qxl_monitors_config_async() function. Both are linked to a display channel. The server/client messages go through the main channel though ... So what happens if a message from the client arrives? Is that broadcasted as-is to all display channels? The current qemu code (interface_client_monitors_config in spice-display.c, which is used with virtio-gpu) simply uses the head as index into the array, so it looks like spice-server doesn't do any filtering here (like only filling in the monitors which belong to the display channel). spice_qxl_monitors_config_async() is never called with virtio-gpu, except when opengl is enabled. In that case qemu simply sets QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] (see qemu_spice_gl_monitor_config in spice-display.c). Which would be ok in case spice-server merges the configs from all channels before sending it off to the client. Not sure this actually happens ... > Interstingly enough, that seems to be the ID we want to have in the > device_display_id attribute. I expect (still need to look that up, I'm > out of time right now) that for virtio-gpu the ID is a bit different, Keep in mind that multiple display devices don't really work right now, and possibly we need to fix not only spice but qemu too. > And yeah, I didn't use the id in the QEMU patches, as I didn't know > how, I expect Gerd to have some grand plans for it :) IIRC the latest plan was to just keep things as is, plan with one channel per monitor for all future things, and just not support one-qxl-device-multihead in combination with multiple display channels. Is that correct? I don't think we need the monitors_id in qemu then, qemu can simply use the channel_id (except for the legacy qxl case). cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Tue, 2018-10-09 at 14:33 -0500, Jonathon Jongsma wrote:
> On Tue, 2018-10-09 at 15:10 +0200, Lukáš Hrázký wrote:
> > Adds two functions to let QEMU provide information to identify
> > graphics
> > devices and their monitors in the guest:
> >
> > * device path - The path identifying the device on the system (e.g.
> > PCI
> > path):
> > spice_qxl_device_set_path(...)
> >
> > * device display ID - The index of the monitor on the graphics
> > device:
> > spice_qxl_device_monitor_add_display_id(...)
> >
> > Signed-off-by: Lukáš Hrázký
> > ---
> > server/red-qxl.c | 67
> >
> > server/spice-qxl.h | 3 ++
> > server/spice-server.syms | 6
> > 3 files changed, 76 insertions(+)
> >
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..143228ca 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> > #include "red-qxl.h"
> >
> >
> > +#define MAX_PATH_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_path[MAX_PATH_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,67 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> > *qxl)
> > red_qxl_async_complete(qxl, cookie);
> > }
> >
> > +/**
> > + * spice_qxl_device_set_path:
>
> It seems to me that _set_device_path() might be a better name than
> _device_set_path().
I've tried to keep the prefix notation that is usually used in C to
denote 'object' hierarchy, so that it's roughly equivalent to
spice.qxl.device.set_path(). It seemed to me the way to go to keep the
naming consistent. But there is really no device 'object' here, so
maybe your way reads better.
> And maybe _address is better than _path?
Yeah, maybe. The contents of the string in it's current form is not
really much of an address, but path may be too generic and misleading.
> > + * @instance the QXL instance to set the path to
> > + * @device_path the path of the device this QXL instance belongs to
>
> It seems to me that if this is public API, the format of the
> @device_path should be documented a bit better?
You're right, I should add that. Lets first agree on the contents and
I'll document it better.
> > + *
> > + * Sets the hardware (e.g. PCI) path of the graphics device
> > represented by this QXL interface instance.
>
> So, this comment suggests that the caller might be able to provide a
> path that is not a PCI path. But the implementation below (mostly the
> debug output, I suppose...) assumes a PCI path. Do we need to support
> non-PCI paths?
I've missed the debug message below. At first I had the implementation
explicitly stating PCI, but then I realized there may eventually be
another interface, and the actual content of the variable starts with
"pci/", so that it allows to use something else in the future. So even
though now we don't have anything but PCI, we don't need and should not
limit the implementation to it.
> > + *
> > + */
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_device_set_path(QXLInstance *instance, const char
> > *device_path)
> > +{
> > +g_return_if_fail(device_path != NULL);
> > +
> > +size_t dp_len = strnlen(device_path, 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_path, device_path, MAX_PATH_LEN);
> > +
> > +g_debug("QXL Instance %d setting device PCI path \"%s\"",
> > instance->id, device_path);
> > +}
> > +
> > +/**
> > + * spice_qxl_device_monitor_add_display_id:
>
> I'm not sure that the word 'device' in this function adds much here. It
> is essentially operating on the QxlInstance, which may represent a
> single device, or may only represent part of a device (e.g. virtio-
> gpu). So I think including 'device' in the name actually makes things
> less clear. I'm also unclear why the word 'monitor' is here. As
> written, it could be interpreted as refering to a "device monitor",
> whatever that is. But it means that we have the word 'monitor' and
> 'display' within the same function name, which adds more confusion.
As above, I've tried to keep the prefix notation based on the object
hierarchy, although here it has gotten quite convoluted :) The word
"monitor" is there because it adds a display_id that belongs to a
certain monitor_id in the SPICE server. The function does have
problems, as you stated below.
> I would suggest a name like spice_qxl_add_device_display_id().
>
> But part of me also doesn't like the verb 'add' here either.
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
Hi, > > + * Sets the hardware (e.g. PCI) path of the graphics device > > represented by this QXL interface instance. > > So, this comment suggests that the caller might be able to provide a > path that is not a PCI path. But the implementation below (mostly the > debug output, I suppose...) assumes a PCI path. Do we need to support > non-PCI paths? Certainly not for the initial revision, maybe never. But thanks to the "pci/" prefix we should be able to add support for other paths later in case it turns out we need it. > > + * Returns: The actual SPICE server monitor_id associated with the > > display > > So, if I remember correctly, Gerd recommended returning this value from > the function. But I think it needs more explanation here. What exactly > is a "monitor_id" supposed to represent? It is not used in your follow- > up qemu patch so it's hard to tell. IIRC the plan was to ditch the global monior_id idea and use the (channel_id, display_id) tuple everywhere ... cheers, Gerd
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
On Tue, 2018-10-09 at 15:10 +0200, Lukáš Hrázký wrote:
> Adds two functions to let QEMU provide information to identify
> graphics
> devices and their monitors in the guest:
>
> * device path - The path identifying the device on the system (e.g.
> PCI
> path):
> spice_qxl_device_set_path(...)
>
> * device display ID - The index of the monitor on the graphics
> device:
> spice_qxl_device_monitor_add_display_id(...)
>
> Signed-off-by: Lukáš Hrázký
> ---
> server/red-qxl.c | 67
>
> server/spice-qxl.h | 3 ++
> server/spice-server.syms | 6
> 3 files changed, 76 insertions(+)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..143228ca 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
> #include "red-qxl.h"
>
>
> +#define MAX_PATH_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_path[MAX_PATH_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,67 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> *qxl)
> red_qxl_async_complete(qxl, cookie);
> }
>
> +/**
> + * spice_qxl_device_set_path:
It seems to me that _set_device_path() might be a better name than
_device_set_path(). And maybe _address is better than _path?
> + * @instance the QXL instance to set the path to
> + * @device_path the path of the device this QXL instance belongs to
It seems to me that if this is public API, the format of the
@device_path should be documented a bit better?
> + *
> + * Sets the hardware (e.g. PCI) path of the graphics device
> represented by this QXL interface instance.
So, this comment suggests that the caller might be able to provide a
path that is not a PCI path. But the implementation below (mostly the
debug output, I suppose...) assumes a PCI path. Do we need to support
non-PCI paths?
> + *
> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_device_set_path(QXLInstance *instance, const char
> *device_path)
> +{
> +g_return_if_fail(device_path != NULL);
> +
> +size_t dp_len = strnlen(device_path, 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_path, device_path, MAX_PATH_LEN);
> +
> +g_debug("QXL Instance %d setting device PCI path \"%s\"",
> instance->id, device_path);
> +}
> +
> +/**
> + * spice_qxl_device_monitor_add_display_id:
I'm not sure that the word 'device' in this function adds much here. It
is essentially operating on the QxlInstance, which may represent a
single device, or may only represent part of a device (e.g. virtio-
gpu). So I think including 'device' in the name actually makes things
less clear. I'm also unclear why the word 'monitor' is here. As
written, it could be interpreted as refering to a "device monitor",
whatever that is. But it means that we have the word 'monitor' and
'display' within the same function name, which adds more confusion.
I would suggest a name like spice_qxl_add_device_display_id().
But part of me also doesn't like the verb 'add' here either. Because
we're not really adding anything by calling this function. The caller
is simply informing the spice server that this qxl instance provides
the specified display. So I might even consider a name like
spice_qxl_provides_device_display_id()... Or maybe
_register_display_id(). Maybe there are better options, though.
> + * @instance the QXL instance to add the display ID to
> + * @device_display_id the display ID to add to the QXL instance
> + *
> + * Adds a #device_display_id to this QXL interface instance. The
> + * #device_display_id is an index (a sequence starting from 0) into
> the list of
> + * all the displays on the graphics device. This function should be
> called in
> + * sequence for all possible displays of the device. The ID of the
> first call
> + * will belong to #monitor_id 0, and so forth in the sequence.
> + *
> + * Since for some graphics devices (e.g. virtio-gpu) a QXL interface
> instance
> + * is created per each display of a single device, you can get e.g.
> the single
> + * following call on the third QXL interface instance for this
> device:
> + *
> + * spice_qxl_device_monitor_add_display_id(instance, 2);
> + *
> + * This tells you that the single monitor of this QXL interface
> actually
> + * belongs to the 3rd display of the underlying device.
> + *
> + * Returns: The actual SPICE server monitor_id associated with the
> display
So, if I remember correctly, Gerd recommended returning this value from
the function. But I think it needs
