Patchwork Add new client_present and client capabilities fields to QXLRom

login
register
mail settings
Submitter Søren Sandmann
Date Aug. 27, 2012, 5:20 p.m.
Message ID <1346088041-17062-2-git-send-email-sandmann@cs.au.dk>
Download mbox | patch
Permalink /patch/180268/
State New
Headers show

Comments

Søren Sandmann - Aug. 27, 2012, 5:20 p.m.
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(-)
Gerd Hoffmann - Aug. 28, 2012, 6:15 a.m.
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
Søren Sandmann - Aug. 29, 2012, 12:58 a.m.
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
Gerd Hoffmann - Aug. 29, 2012, 6 a.m.
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
Søren Sandmann - Aug. 29, 2012, 9:05 p.m.
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

Patch

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 {