| Message ID | 1346088041-17062-2-git-send-email-sandmann@cs.au.dk |
|---|---|
| State | New |
| Headers | show |
On 08/27/12 19:20, Søren Sandmann Pedersen wrote: > From: Søren Sandmann Pedersen <ssp@redhat.com> > > The client_present field is a byte that is set of non-zero when a > client is connected and to zero when no client is connected. > > The client_capabilities[58] array contains 464 bits that indicate the > capabilities of the client. What is supposed to happen in case multiple clients are connected? How do you handle the race conditions, especially on capability downgrade? There might be not-yet processed commands in the command queue which the client is unable to handle, or existing surfaces using formats the client doesn't understand ... cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > On 08/27/12 19:20, Søren Sandmann Pedersen wrote: >> From: Søren Sandmann Pedersen <ssp@redhat.com> >> >> The client_present field is a byte that is set of non-zero when a >> client is connected and to zero when no client is connected. >> >> The client_capabilities[58] array contains 464 bits that indicate the >> capabilities of the client. > > What is supposed to happen in case multiple clients are connected? Is this case supported at all? If it is, I'd say that the guest should not be aware of it and the bits advertise should be interpreted as "these are the capabilities that spice-server will marshall on to the clients that are connected". Presumably spice-server would then set the bit vector to the intersection of all the clients. > How do you handle the race conditions, especially on capability > downgrade? There might be not-yet processed commands in the command > queue which the client is unable to handle, or existing surfaces using > formats the client doesn't understand ... Good question. I don't know of a good way to deal with the situation where the new client is unable to handle existing surfaces. I suppose in principle spice-server could emulate their existence, sending them as images, but I'm not familiar enough with spice-server to say whether that is feasible. For commands, would it work for spice-server to just process everything in the command ring after changing the capability bits (ie., in possibly brief moment before a new client connects)? It seems that would be a good thing to do even without capability bits. Søren
On 08/29/12 02:58, Søren Sandmann wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> On 08/27/12 19:20, Søren Sandmann Pedersen wrote: >>> From: Søren Sandmann Pedersen <ssp@redhat.com> >>> >>> The client_present field is a byte that is set of non-zero when a >>> client is connected and to zero when no client is connected. >>> >>> The client_capabilities[58] array contains 464 bits that indicate the >>> capabilities of the client. >> >> What is supposed to happen in case multiple clients are connected? > > Is this case supported at all? There is code for it, although disabled by default and nobody actively working in it as far I know. We should at least have a plan how to handle that situation ... > If it is, I'd say that the guest should not be aware of it and the bits > advertise should be interpreted as "these are the capabilities that > spice-server will marshall on to the clients that are > connected". Presumably spice-server would then set the bit vector to the > intersection of all the clients. Makes sense. >> How do you handle the race conditions, especially on capability >> downgrade? There might be not-yet processed commands in the command >> queue which the client is unable to handle, or existing surfaces using >> formats the client doesn't understand ... > > Good question. > > I don't know of a good way to deal with the situation where the new > client is unable to handle existing surfaces. We need a sensible solution here. If we can't handle capability downgrade at runtime the capability negotiation between guest and client doesn't make sense in the first place. Failing that we can add a a8 switch to qxl. When enabled qemu asks spice-server to enable a8, which in turn will raise the display channel minor version, basically requiring an updated spice client to connect. With a8 disabled old clients can connect too. > For commands, would it work for spice-server to just process everything > in the command ring after changing the capability bits (ie., in possibly > brief moment before a new client connects)? It seems that would be a > good thing to do even without capability bits. spice server could process (aka server-side rendering) all outstanding commands after updating capability bits and before starting to forward commands to the new client. Yes, that should work. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: >> I don't know of a good way to deal with the situation where the new >> client is unable to handle existing surfaces. > > We need a sensible solution here. If we can't handle capability > downgrade at runtime the capability negotiation between guest and client > doesn't make sense in the first place. > > Failing that we can add a a8 switch to qxl. When enabled qemu asks > spice-server to enable a8, which in turn will raise the display channel > minor version, basically requiring an updated spice client to connect. > With a8 disabled old clients can connect too. I think the same scheme as for commands could be used: - When a new client connects, if it doesn't understand a8, then the server won't send it any a8 surfaces. - The guest driver is expected to not refer to any such surfaces once it sees the change in capability bits, and may want to delete them. The race condition is handled by the server processing all commands after changing the capability bits, but before forwarding any commands to the new client. If I don't hear any objections to the above, I'll update the patches as follows: - When a new client connects, the capability bits are changed, followed by a processing of all commands in the ring. At this point new commands may be forwarded. - Add ifdefs to allow qemu to compile against older versions of spice. - Use 4 for the PCI revision instead of 5 I'll drop the NUM_SURFACES change for now. The guest driver probably need the ability to swap pixmaps in and out of video memory in any case, since X clients sometimes leak pixmaps and since no fixed number of surfaces is likely to be enough for all cases. Søren
diff --git a/spice/qxl_dev.h b/spice/qxl_dev.h index 1292767..a19cc67 100644 --- a/spice/qxl_dev.h +++ b/spice/qxl_dev.h @@ -49,6 +49,7 @@ enum { QXL_REVISION_STABLE_V06=0x02, QXL_REVISION_STABLE_V10=0x03, QXL_REVISION_STABLE_V12=0x04, + QXL_REVISION_STABLE_V13=0x05, }; #define QXL_DEVICE_ID_DEVEL 0x01ff @@ -148,7 +149,9 @@ typedef struct SPICE_ATTR_PACKED QXLRom { uint8_t slot_gen_bits; uint8_t slot_id_bits; uint8_t slot_generation; - uint8_t padding[3]; /* Padding to 32bit align */ + /* appended for qxl-5 */ + uint8_t client_present; + uint8_t client_capabilities[58]; } QXLRom; /* qxl-1 compat: fixed */ @@ -231,6 +234,7 @@ SPICE_RING_DECLARE(QXLReleaseRing, uint64_t, QXL_RELEASE_RING_SIZE); #define QXL_INTERRUPT_CURSOR (1 << 1) #define QXL_INTERRUPT_IO_CMD (1 << 2) #define QXL_INTERRUPT_ERROR (1 << 3) +#define QXL_INTERRUPT_CLIENT (1 << 4) /* qxl-1 compat: append only */ typedef struct SPICE_ATTR_PACKED QXLRam {
From: Søren Sandmann Pedersen <ssp@redhat.com> The client_present field is a byte that is set of non-zero when a client is connected and to zero when no client is connected. The client_capabilities[58] array contains 464 bits that indicate the capabilities of the client. Each bit corresponds to a SPICE_DISPLAY_CAP_* capability. In particular, if the client has capability C, then bit (C % 8) in byte (C / 8) is set. The capability bits only have a defined meaning when a client is connected, ie., when client_present is non-zero. The number 58 was chosen to fill out a cache line in QXLRom. A new QXL_INTERRUPT_CLIENT interrupt is defined, which will be raised whenever a client connects or disconnects. Finally, add a new QXL_REVISION_STABLE_V13 = 0x05 constant to indicate the presence of these new features. --- spice/qxl_dev.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)