[Qemu-devel] Add ability to advertise client capabilities to QXL device

[Qemu-devel] Add ability to advertise client capabilities to QXL device

[Søren Sandmann Pedersen]

Hi, 

The following patches add the ability for spice-server to advertise
the capabilities of connected clients to guests. They do this through
adding some new fields to QXLRom:

 - whether a client is present

 - a bit field to indicate which SPICE_DISPLAY_CAP_* capabilities the
   client has.

The patches affect both spice and qemu, and I'm not totally sure how
the dependencies between those are usually managed. If I did it wrong,
please let me know.


Soren

[Qemu-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann Pedersen]

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(-)

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 {
-- 
1.7.4

[Qemu-devel] [PATCH] Add new set_client_capabilities() interface to QXLInstance

[Søren Sandmann Pedersen]

From: Søren Sandmann Pedersen <ssp@redhat.com>

A new interface

  set_client_capabilities (QXLInstance *qin,
  			   uint8_t client_present,
  			   uint8_t caps[58]);

is added to QXLInstance, and spice server is changed to call it
whenever a client connects or disconnects. The QXL device in response
is expected to update the client capability bits in the ROM of the
device and raise the QXL_INTERRUPT_CLIENT interrupt.
---
 server/red_worker.c |   24 ++++++++++++++++++++++++
 server/spice.h      |    3 +++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index bd6de1c..6f97d6b 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10289,6 +10289,23 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
     spice_info("jpeg %s", display_channel->enable_jpeg ? "enabled" : "disabled");
     spice_info("zlib-over-glz %s", display_channel->enable_zlib_glz_wrap ? "enabled" : "disabled");
 
+    if (worker->qxl->st->qif->set_client_capabilities) {
+        RedChannelClient *rcc = (RedChannelClient *)dcc;
+        uint8_t caps[58] = { 0 };
+
+#define SET_CAP(a,c)                                                    \
+        ((a)[(c) / 8] |= (1 << ((c) % 8)))
+
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_SIZED_STREAM);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_MONITORS_CONFIG))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+
+        worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
+    }
+    
     // todo: tune level according to bandwidth
     display_channel->zlib_level = ZLIB_DEFAULT_COMPRESSION_LEVEL;
     red_display_client_init_streams(dcc);
@@ -11137,9 +11154,16 @@ void handle_dev_display_disconnect(void *opaque, void *payload)
 {
     RedWorkerMessageDisplayDisconnect *msg = payload;
     RedChannelClient *rcc = msg->rcc;
+    RedWorker *worker = opaque;
 
     spice_info("disconnect display client");
     spice_assert(rcc);
+
+    if (worker->qxl->st->qif->set_client_capabilities) {
+        uint8_t caps[58] = { 0 };
+        worker->qxl->st->qif->set_client_capabilities(worker->qxl, FALSE, caps);
+    }
+
     red_channel_client_disconnect(rcc);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 3d70ec7..c53c044 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -237,6 +237,9 @@ struct QXLInterface {
     void (*update_area_complete)(QXLInstance *qin, uint32_t surface_id,
                                  struct QXLRect *updated_rects,
                                  uint32_t num_updated_rects);
+    void (*set_client_capabilities)(QXLInstance *qin,
+				    uint8_t client_present,
+				    uint8_t caps[58]);
 };
 
 struct QXLInstance {
-- 
1.7.4

[Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann Pedersen]

From: Søren Sandmann Pedersen <ssp@redhat.com>

This new interface lets spice server inform the guest whether

(a) a client is connected
(b) what capabilities the client has

There is a fixed number (464) of bits reserved for capabilities, and
when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
is generated.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.c |   21 +++++++++++++++++++++
 hw/qxl.h |    2 +-
 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..690743a 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -901,6 +901,22 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     }
 }
 
+/* called from spice server thread context only */
+static void interface_set_client_capabilities(QXLInstance *sin,
+					      uint8_t client_present,
+					      uint8_t caps[58])
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    qxl->shadow_rom.client_present = client_present;
+    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    qxl->rom->client_present = client_present;
+    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    qxl_rom_set_dirty(qxl);
+
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
+}
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -922,6 +938,7 @@ static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
     .update_area_complete    = interface_update_area_complete,
+    .set_client_capabilities = interface_set_client_capabilities,
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1785,6 +1802,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         io_size = 16;
         break;
     case 3: /* qxl-3 */
+	pci_device_rev = QXL_REVISION_STABLE_V10;
+	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+	break;
+    case 5:
     default:
         pci_device_rev = QXL_DEFAULT_REVISION;
         io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..a130f19 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V13
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-- 
1.7.4

[Qemu-devel] Add ability to advertise client capabilities to QXL device

[Søren Sandmann Pedersen]

Hi, 

The following patches add the ability for spice-server to advertise
the capabilities of connected clients to guests. They do this through
adding some new fields to QXLRom:

 - whether a client is present

 - a bit field to indicate which SPICE_DISPLAY_CAP_* capabilities the
   client has.

The patches affect both spice and qemu, and I'm not totally sure how
the dependencies between those are usually managed. If I did it wrong,
please let me know.


Soren

Re: [Qemu-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Gerd Hoffmann]

On 08/27/12 19:20, Søren Sandmann Pedersen wrote:
> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> This new interface lets spice server inform the guest whether
> 
> (a) a client is connected
> (b) what capabilities the client has
> 
> There is a fixed number (464) of bits reserved for capabilities, and
> when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
> is generated.

Needs some #ifdefs to make sure qemu continues to build with old
spice-server versions.

>      case 3: /* qxl-3 */
> +	pci_device_rev = QXL_REVISION_STABLE_V10;
> +	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> +	break;
> +    case 5:

I think there is no need to jump to 5, we can use revision 4 for both
client capabilities and monitors config.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann]

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

Re: [Qemu-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Gerd Hoffmann]

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

Re: [Qemu-devel] [Spice-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Alon Levy]

On Wed, Aug 29, 2012 at 02:58:14AM +0200, 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?
> 
> 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.

Sending a surface with a format the client doesn't recognize as an image
- how does that help? you'd want to render anything dependent on that
  surface. And then the guest will be notified of the reduced
  capabilities and needs to never use those surfaces again (better yet,
  destroy them since they are just taking space).

  The rendering is already accomplished in on_new_display_channel_client
  with the red_flush_current(worker, 0) call, which recursively goes to
  all the dependent surfaces of each drawable and renders it 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.

This should work. Something like this I guess: (probably only if
capabilities have changed - otherwise no need. And without
MAX_PIPE_SIZE, although I'm not sure what you would put instead.):

--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9493,6 +9493,10 @@ static void on_new_display_channel_client(DisplayChannelClient *dcc)
     }
     red_channel_client_ack_zero_messages_window(&dcc->common.base);
     if (worker->surfaces[0].context.canvas) {
+        int ring_is_empty;
+
+        while (red_process_commands(worker, MAX_PIPE_SIZE, &ring_is_empty)) {
+        }
         red_current_flush(worker, 0);
         push_new_primary_surface(dcc);
         red_push_surface_image(dcc, 0);

> 
> 
> Søren
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Qemu-devel] [Spice-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann]

Alon Levy <alevy@redhat.com> writes:

>> 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.
>
> Sending a surface with a format the client doesn't recognize as an image
> - how does that help? you'd want to render anything dependent on that
>   surface. And then the guest will be notified of the reduced
>   capabilities and needs to never use those surfaces again (better yet,
>   destroy them since they are just taking space).
>
>   The rendering is already accomplished in on_new_display_channel_client
>   with the red_flush_current(worker, 0) call, which recursively goes to
>   all the dependent surfaces of each drawable and renders it too.

The scheme I had in mind was this:

    - When a new non-a8-capable client appears, don't send it any of the
      a8 surfaces

    - If the client doesn't understand a8 surfaces,

        - keep all a8 surfaces rendered on the server side

        - if the guest sends a command using an a8 surface as a
          destination, simply render the command on the server side

        - if the client sends a command using an a8 surface as a source,
          rewrite the image object to be a real image referring to the
          server side bits (which are also sent or possibly cached)
          rather than a surface

But it's much simpler to just say that the guest should stop referring
to a8 surfaces if the client can't handle them.

Ie., the same scheme as for commands: When a client disconnects,
spice-server changes the capability bits, then processes everything in
the ring. After this, the guest is expected to stop referring to a8
surfaces (and may indeed want to destroy them as you say).


Soren

Re: [Qemu-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann]

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

Re: [Qemu-devel] [Spice-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Gerd Hoffmann]

  Hi,

> The scheme I had in mind was this:
> 
>     - When a new non-a8-capable client appears, don't send it any of the
>       a8 surfaces
> 
>     - If the client doesn't understand a8 surfaces,
> 
>         - keep all a8 surfaces rendered on the server side
> 
>         - if the guest sends a command using an a8 surface as a
>           destination, simply render the command on the server side
> 
>         - if the client sends a command using an a8 surface as a source,
>           rewrite the image object to be a real image referring to the
>           server side bits (which are also sent or possibly cached)
>           rather than a surface

Hmm, when the server is able to translate a8 ops into non-a8 ops using
server-side rendering, then there is no need to notify the guest about
the client capabilities.

> But it's much simpler to just say that the guest should stop referring
> to a8 surfaces if the client can't handle them.

Not sure about that, this move might just shift the complexity from
spice-server to the guest qxl driver.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann]

Gerd Hoffmann <kraxel@redhat.com> writes:

>> The scheme I had in mind was this:
>> 
>>     - When a new non-a8-capable client appears, don't send it any of the
>>       a8 surfaces
>> 
>>     - If the client doesn't understand a8 surfaces,
>> 
>>         - keep all a8 surfaces rendered on the server side
>> 
>>         - if the guest sends a command using an a8 surface as a
>>           destination, simply render the command on the server side
>> 
>>         - if the client sends a command using an a8 surface as a source,
>>           rewrite the image object to be a real image referring to the
>>           server side bits (which are also sent or possibly cached)
>>           rather than a surface
>
> Hmm, when the server is able to translate a8 ops into non-a8 ops using
> server-side rendering, then there is no need to notify the guest about
> the client capabilities.

To be clear, this ability doesn't exist at the moment, and it would be a
significant chunk of work to add it.

>> But it's much simpler to just say that the guest should stop referring
>> to a8 surfaces if the client can't handle them.
>
> Not sure about that, this move might just shift the complexity from
> spice-server to the guest qxl driver.

The ability to handle this is already pretty much present in at least
the X driver (and I'm pretty sure the Windows driver has it as well)
because any time something can't be expressed in the SPICE protocol, it
has to fall back to software rendering. Ie., it has to read all the
involved surfaces back from video memory, do software rendering, then
upload the result as an image.

Dealing with a disappearing ability to handle a8 surfaces would simply
be a matter of reading back the a8 surfaces to guest RAM and then not
attempt to acccelerate any operations involving them any more.

It looks much more involved to do it in spice-server because it would
probably involve adding a new concept of "emulated surface" that needs
to be handled specially in a bunch of cases.


Søren

Re: [Qemu-devel] [Spice-devel] [PATCH] Add new client_present and client capabilities fields to QXLRom

[Gerd Hoffmann]

  Hi,

>> Hmm, when the server is able to translate a8 ops into non-a8 ops using
>> server-side rendering, then there is no need to notify the guest about
>> the client capabilities.
> 
> To be clear, this ability doesn't exist at the moment, and it would be a
> significant chunk of work to add it.
> 
>>> But it's much simpler to just say that the guest should stop referring
>>> to a8 surfaces if the client can't handle them.
>>
>> Not sure about that, this move might just shift the complexity from
>> spice-server to the guest qxl driver.
> 
> The ability to handle this is already pretty much present in at least
> the X driver (and I'm pretty sure the Windows driver has it as well)
> because any time something can't be expressed in the SPICE protocol, it
> has to fall back to software rendering. Ie., it has to read all the
> involved surfaces back from video memory, do software rendering, then
> upload the result as an image.
> 
> Dealing with a disappearing ability to handle a8 surfaces would simply
> be a matter of reading back the a8 surfaces to guest RAM and then not
> attempt to acccelerate any operations involving them any more.
> 
> It looks much more involved to do it in spice-server because it would
> probably involve adding a new concept of "emulated surface" that needs
> to be handled specially in a bunch of cases.

Ok, so the tradeoff seems clear.  I trust you on that one, you know the
guest drivers alot better than I do.  Lets go forward with the client
capability notification for the guest, /me awaits a new revision of the
patches.

cheers,
  Gerd

[Qemu-devel] New patches to add capabilities to spice and qxl

[Søren Sandmann Pedersen]

Hi,

Here are new revisions of the capabilities patches. Also included here
is a new SPICE_DISPLAY_CAP_A8_SURFACE capability since this is
logically distinct from the composite command.

Thanks,
Soren

[Qemu-devel] [PATCH-v2 spice-protocol 1/2] Add A8 surface capability

[Søren Sandmann Pedersen]

Even though the ability to handle a8 surfaces was added at the same
time as the composite command, they are logically separate, so add a
capability bit to indicate the presence of a8 surfaces.
---
 spice/protocol.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spice/protocol.h b/spice/protocol.h
index 7008399..0671292 100644
--- a/spice/protocol.h
+++ b/spice/protocol.h
@@ -128,6 +128,7 @@ enum {
     SPICE_DISPLAY_CAP_SIZED_STREAM,
     SPICE_DISPLAY_CAP_MONITORS_CONFIG,
     SPICE_DISPLAY_CAP_COMPOSITE,
+    SPICE_DISPLAY_CAP_A8_SURFACE,
 };
 
 enum {
-- 
1.7.11.4

[Qemu-devel] [PATCH-v2 spice-protocol 2/2] Add new client_present and client capabilities fields to QXLRom

[Søren Sandmann Pedersen]

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.
---
 spice/qxl_dev.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/spice/qxl_dev.h b/spice/qxl_dev.h
index 1292767..50784dc 100644
--- a/spice/qxl_dev.h
+++ b/spice/qxl_dev.h
@@ -148,7 +148,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-4 */
+    uint8_t client_present;
+    uint8_t client_capabilities[58];
 } QXLRom;
 
 /* qxl-1 compat: fixed */
@@ -231,6 +233,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 {
-- 
1.7.11.4

[Qemu-devel] [PATCH-v2 spice-gtk] Advertise SPICE_DISPLAY_CAP_A8_SURFACE

[Søren Sandmann Pedersen]

---
 gtk/channel-display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index 99fe9c9..326ad22 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -682,6 +682,7 @@ static void spice_display_channel_reset_capabilities(SpiceChannel *channel)
     spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_SIZED_STREAM);
     spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_MONITORS_CONFIG);
     spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_COMPOSITE);
+    spice_channel_set_capability(SPICE_CHANNEL(channel), SPICE_DISPLAY_CAP_A8_SURFACE);
 }
 
 static void spice_display_channel_init(SpiceDisplayChannel *channel)
-- 
1.7.11.4

[Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann Pedersen]

---
 server/red_worker.c | 2 ++
 spice-common        | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 843f559..23f3464 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10377,6 +10377,8 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
             SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
         if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
             SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
 
         worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
     }
diff --git a/spice-common b/spice-common
index 86e286b..04dc2be 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 86e286ba2003c206e700fd70ec67c1cf4ac8d8a6
+Subproject commit 04dc2bee9ecdda7d7966f9267df37ab23bb5a802
-- 
1.7.11.4

[Qemu-devel] [PATCH-v2 spice 2/2] Bump spice.h version number to 0.11.4

[Søren Sandmann Pedersen]

No new symbols are added, but there is an addition to QXLInterface:

    void (*set_client_capabilities)(QXLInstance *qin,
                                    uint8_t client_present,
                                    uint8_t caps[58]);
---
 server/spice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/spice.h b/server/spice.h
index 9d65efa..71adfb6 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -22,7 +22,7 @@
 #include <sys/socket.h>
 #include <spice/qxl_dev.h>
 
-#define SPICE_SERVER_VERSION 0x000b02 /* release 0.11.2 */
+#define SPICE_SERVER_VERSION 0x000b04 /* release 0.11.4 */
 
 /* interface base type */
 
-- 
1.7.11.4

[Qemu-devel] [PATCH-v2 qemu] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann Pedersen]

This new interface lets spice server inform the guest whether

(a) a client is connected
(b) what capabilities the client has

There is a fixed number (464) of bits reserved for capabilities, and
when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
is generated.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.c | 29 ++++++++++++++++++++++++++++-
 hw/qxl.h |  2 +-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..1f62529 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     }
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b04
+
+/* called from spice server thread context only */
+static void interface_set_client_capabilities(QXLInstance *sin,
+					      uint8_t client_present,
+					      uint8_t caps[58])
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    qxl->shadow_rom.client_present = client_present;
+    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    qxl->rom->client_present = client_present;
+    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    qxl_rom_set_dirty(qxl);
+
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
+}
+
+#endif
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
     .update_area_complete    = interface_update_area_complete,
+#if SPICE_SERVER_VERSION >= 0x000b04
+    .set_client_capabilities = interface_set_client_capabilities,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1292,7 +1315,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
 
     d->mode = QXL_MODE_COMPAT;
     d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
-#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
+#if QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
     if (mode->bits == 16) {
         d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
     }
@@ -1785,6 +1808,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         io_size = 16;
         break;
     case 3: /* qxl-3 */
+	pci_device_rev = QXL_REVISION_STABLE_V10;
+	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+	break;
+    case 4:
     default:
         pci_device_rev = QXL_DEFAULT_REVISION;
         io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..98d5a64 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Alon Levy]

> ---
>  server/red_worker.c | 2 ++
>  spice-common        | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 843f559..23f3464 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -10377,6 +10377,8 @@ static void
> handle_new_display_channel(RedWorker *worker, RedClient *client, Red
>              SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>          if (red_channel_client_test_remote_cap(rcc,
>          SPICE_DISPLAY_CAP_COMPOSITE))
>              SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
> +        if (red_channel_client_test_remote_cap(rcc,
> SPICE_DISPLAY_CAP_COMPOSITE))
> +            SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);

Didn't you mean to test remote SPICE_DISPLAY_CAP_A8_SURFACE?

>  
>          worker->qxl->st->qif->set_client_capabilities(worker->qxl,
>          TRUE, caps);
>      }
> diff --git a/spice-common b/spice-common
> index 86e286b..04dc2be 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 86e286ba2003c206e700fd70ec67c1cf4ac8d8a6
> +Subproject commit 04dc2bee9ecdda7d7966f9267df37ab23bb5a802
> --
> 1.7.11.4
> 
> 
> 

Re: [Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

Alon Levy <alevy@redhat.com> writes:

>> ---
>>  server/red_worker.c | 2 ++
>>  spice-common        | 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 843f559..23f3464 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -10377,6 +10377,8 @@ static void
>> handle_new_display_channel(RedWorker *worker, RedClient *client, Red
>>              SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>          if (red_channel_client_test_remote_cap(rcc,
>>          SPICE_DISPLAY_CAP_COMPOSITE))
>>              SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
>> +        if (red_channel_client_test_remote_cap(rcc,
>> SPICE_DISPLAY_CAP_COMPOSITE))
>> +            SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
>
> Didn't you mean to test remote SPICE_DISPLAY_CAP_A8_SURFACE?

Yes, good catch. I'll fix before pushing.

Thanks,
Soren

Re: [Qemu-devel] [Spice-devel] [PATCH-v2 qemu] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann]

Søren Sandmann Pedersen <ssp@redhat.com> writes:

> @@ -1292,7 +1315,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
>  
>      d->mode = QXL_MODE_COMPAT;
>      d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
> -#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
> +#if QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
>      if (mode->bits == 16) {
>          d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
>      }

This one is obviously a mistake. New patch to follow.


Soren

[Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

This new interface lets spice server inform the guest whether

(a) a client is connected
(b) what capabilities the client has

There is a fixed number (464) of bits reserved for capabilities, and
when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
is generated.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.c |   27 +++++++++++++++++++++++++++
 hw/qxl.h |    2 +-
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..ffe1a76 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     }
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b04
+
+/* called from spice server thread context only */
+static void interface_set_client_capabilities(QXLInstance *sin,
+					      uint8_t client_present,
+					      uint8_t caps[58])
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    qxl->shadow_rom.client_present = client_present;
+    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    qxl->rom->client_present = client_present;
+    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    qxl_rom_set_dirty(qxl);
+
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
+}
+
+#endif
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
     .update_area_complete    = interface_update_area_complete,
+#if SPICE_SERVER_VERSION >= 0x000b04
+    .set_client_capabilities = interface_set_client_capabilities,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
@@ -1785,6 +1808,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
         io_size = 16;
         break;
     case 3: /* qxl-3 */
+	pci_device_rev = QXL_REVISION_STABLE_V10;
+	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
+	break;
+    case 4:
     default:
         pci_device_rev = QXL_DEFAULT_REVISION;
         io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
diff --git a/hw/qxl.h b/hw/qxl.h
index 172baf6..98d5a64 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
         }                                                               \
     } while (0)
 
-#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
+#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12
 
 /* qxl.c */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
-- 
1.7.4

Re: [Qemu-devel] [PATCH-v2 spice 1/2] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

Søren Sandmann <sandmann@cs.au.dk> writes:

> Alon Levy <alevy@redhat.com> writes:
>
>>> ---
>>>  server/red_worker.c | 2 ++
>>>  spice-common        | 2 +-
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index 843f559..23f3464 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -10377,6 +10377,8 @@ static void
>>> handle_new_display_channel(RedWorker *worker, RedClient *client, Red
>>>              SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>>          if (red_channel_client_test_remote_cap(rcc,
>>>          SPICE_DISPLAY_CAP_COMPOSITE))
>>>              SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
>>> +        if (red_channel_client_test_remote_cap(rcc,
>>> SPICE_DISPLAY_CAP_COMPOSITE))
>>> +            SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
>>
>> Didn't you mean to test remote SPICE_DISPLAY_CAP_A8_SURFACE?
>
> Yes, good catch. I'll fix before pushing.

I realized that I didn't actually send the whole patch series for
spice-server. There are five patches rather than two.


Soren

[Qemu-devel] [PATCH 1/5] client: Advertise A8_SURFACE capability

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

---
 client/display_channel.cpp |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/client/display_channel.cpp b/client/display_channel.cpp
index d08072d..49a4c6a 100644
--- a/client/display_channel.cpp
+++ b/client/display_channel.cpp
@@ -652,6 +652,7 @@ DisplayChannel::DisplayChannel(RedClient& client, uint32_t id,
     set_draw_handlers();
 
     set_capability(SPICE_DISPLAY_CAP_COMPOSITE);
+    set_capability(SPICE_DISPLAY_CAP_A8_SURFACE);
 }
 
 DisplayChannel::~DisplayChannel()
-- 
1.7.4

[Qemu-devel] [PATCH 2/5] Add new set_client_capabilities() interface to QXLInstance

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

A new interface

  set_client_capabilities (QXLInstance *qin,
  			   uint8_t client_present,
  			   uint8_t caps[58]);

is added to QXLInstance, and spice server is changed to call it
whenever a client connects or disconnects. The QXL device in response
is expected to update the client capability bits in the ROM of the
device and raise the QXL_INTERRUPT_CLIENT interrupt.

There is a potential race condition in the case where a client
disconnects and a new client with fewer capabilities connects. There
may be commands in the ring that the new client can't handle. This
case is handled by first changing the capability bits, then processing
all commands in the ring, and then start forwarding commands to the
new client. As long as the guest obeys the capability bits, the new
client will never see anything it doesn't understand.
---
 server/red_worker.c |   24 ++++++++++++++++++++++++
 server/spice.h      |    3 +++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 133ba94..60b5471 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10359,6 +10359,23 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
     spice_info("jpeg %s", display_channel->enable_jpeg ? "enabled" : "disabled");
     spice_info("zlib-over-glz %s", display_channel->enable_zlib_glz_wrap ? "enabled" : "disabled");
 
+    if (worker->qxl->st->qif->set_client_capabilities) {
+        RedChannelClient *rcc = (RedChannelClient *)dcc;
+        uint8_t caps[58] = { 0 };
+
+#define SET_CAP(a,c)                                                    \
+        ((a)[(c) / 8] |= (1 << ((c) % 8)))
+
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_SIZED_STREAM);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_MONITORS_CONFIG))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+
+        worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
+    }
+    
     // todo: tune level according to bandwidth
     display_channel->zlib_level = ZLIB_DEFAULT_COMPRESSION_LEVEL;
     red_display_client_init_streams(dcc);
@@ -11213,9 +11230,16 @@ void handle_dev_display_disconnect(void *opaque, void *payload)
 {
     RedWorkerMessageDisplayDisconnect *msg = payload;
     RedChannelClient *rcc = msg->rcc;
+    RedWorker *worker = opaque;
 
     spice_info("disconnect display client");
     spice_assert(rcc);
+
+    if (worker->qxl->st->qif->set_client_capabilities) {
+        uint8_t caps[58] = { 0 };
+        worker->qxl->st->qif->set_client_capabilities(worker->qxl, FALSE, caps);
+    }
+
     red_channel_client_disconnect(rcc);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 0dc9d05..9d65efa 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -239,6 +239,9 @@ struct QXLInterface {
     void (*update_area_complete)(QXLInstance *qin, uint32_t surface_id,
                                  struct QXLRect *updated_rects,
                                  uint32_t num_updated_rects);
+    void (*set_client_capabilities)(QXLInstance *qin,
+				    uint8_t client_present,
+				    uint8_t caps[58]);
 };
 
 struct QXLInstance {
-- 
1.7.4

[Qemu-devel] [PATCH 3/5] Process outstanding commands in the ring after changing capability bits

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

When a new client connects, there may be commands in the ring that it
can't understand, so we need to process these before forwarding new
commands to the client. By doing this after changing the capability
bits we ensure that the new client will never see a command that it
doesn't understand (under the assumption that the guest will read and
obey the capability bits).
---
 server/red_worker.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 60b5471..f87967c 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9493,6 +9493,11 @@ static void on_new_display_channel_client(DisplayChannelClient *dcc)
     }
     red_channel_client_ack_zero_messages_window(&dcc->common.base);
     if (worker->surfaces[0].context.canvas) {
+        int ring_is_empty;
+
+        while (red_process_commands(worker, MAX_PIPE_SIZE, &ring_is_empty)) {
+        }
+        
         red_current_flush(worker, 0);
         push_new_primary_surface(dcc);
         red_push_surface_image(dcc, 0);
-- 
1.7.4

[Qemu-devel] [PATCH 4/5] Set a8 capability in the QXL device if supported by the client

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

---
 server/red_worker.c |    2 ++
 spice-common        |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index f87967c..17d9ef8 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10377,6 +10377,8 @@ static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
             SET_CAP(caps, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
         if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
             SET_CAP(caps, SPICE_DISPLAY_CAP_COMPOSITE);
+        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_COMPOSITE))
+            SET_CAP(caps, SPICE_DISPLAY_CAP_A8_SURFACE);
 
         worker->qxl->st->qif->set_client_capabilities(worker->qxl, TRUE, caps);
     }
diff --git a/spice-common b/spice-common
index 86e286b..04dc2be 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 86e286ba2003c206e700fd70ec67c1cf4ac8d8a6
+Subproject commit 04dc2bee9ecdda7d7966f9267df37ab23bb5a802
-- 
1.7.4

[Qemu-devel] [PATCH 5/5] Bump spice.h version number to 0.11.4

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

No new symbols are added, but there is an addition to QXLInterface:

    void (*set_client_capabilities)(QXLInstance *qin,
                                    uint8_t client_present,
                                    uint8_t caps[58]);
---
 server/spice.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/server/spice.h b/server/spice.h
index 9d65efa..71adfb6 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -22,7 +22,7 @@
 #include <sys/socket.h>
 #include <spice/qxl_dev.h>
 
-#define SPICE_SERVER_VERSION 0x000b02 /* release 0.11.2 */
+#define SPICE_SERVER_VERSION 0x000b04 /* release 0.11.4 */
 
 /* interface base type */
 
-- 
1.7.4

Re: [Qemu-devel] [PATCH] qxl: Add set_client_capabilities() interface to QXLInterface

[Alon Levy]

> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> This new interface lets spice server inform the guest whether
> 
> (a) a client is connected
> (b) what capabilities the client has
> 
> There is a fixed number (464) of bits reserved for capabilities, and
> when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
> is generated.
> 
> Signed-off-by: Soren Sandmann <ssp@redhat.com>
> ---
>  hw/qxl.c |   27 +++++++++++++++++++++++++++
>  hw/qxl.h |    2 +-
>  2 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index c2dd3b4..ffe1a76 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance
> *sin, uint64_t cookie_token)
>      }
>  }
>  
> +#if SPICE_SERVER_VERSION >= 0x000b04
> +
> +/* called from spice server thread context only */
> +static void interface_set_client_capabilities(QXLInstance *sin,
> +					      uint8_t client_present,
> +					      uint8_t caps[58])
> +{
> +    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> +
> +    qxl->shadow_rom.client_present = client_present;
> +    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
> +    qxl->rom->client_present = client_present;
> +    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
> +    qxl_rom_set_dirty(qxl);
> +
> +    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
> +}
> +
> +#endif
> +
>  static const QXLInterface qxl_interface = {
>      .base.type               = SPICE_INTERFACE_QXL,
>      .base.description        = "qxl gpu",
> @@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
>      .flush_resources         = interface_flush_resources,
>      .async_complete          = interface_async_complete,
>      .update_area_complete    = interface_update_area_complete,
> +#if SPICE_SERVER_VERSION >= 0x000b04
> +    .set_client_capabilities = interface_set_client_capabilities,
> +#endif
>  };
>  
>  static void qxl_enter_vga_mode(PCIQXLDevice *d)
> @@ -1785,6 +1808,10 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>          io_size = 16;
>          break;
>      case 3: /* qxl-3 */
> +	pci_device_rev = QXL_REVISION_STABLE_V10;
> +	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> +	break;
> +    case 4:
>      default:
>          pci_device_rev = QXL_DEFAULT_REVISION;
>          io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> diff --git a/hw/qxl.h b/hw/qxl.h
> index 172baf6..98d5a64 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -128,7 +128,7 @@ typedef struct PCIQXLDevice {
>          }
>                                                                        \
>      } while (0)
>  
> -#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12

QXL_REVISION_STABLE_V12 is only defined in latest spice-protocol, too new for the qemu required version.

Gerd, maybe it's a good idea to require spice-protocol 0.12.1, now that it's released? this will remove a lot of cruft.

>  
>  /* qxl.c */
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int
>  group_id);
> --
> 1.7.4
> 
> 
> 

Re: [Qemu-devel] [Spice-devel] [PATCH 3/5] Process outstanding commands in the ring after changing capability bits

[Alon Levy]

> From: Søren Sandmann Pedersen <ssp@redhat.com>
> 
> When a new client connects, there may be commands in the ring that it
> can't understand, so we need to process these before forwarding new
> commands to the client. By doing this after changing the capability
> bits we ensure that the new client will never see a command that it
> doesn't understand (under the assumption that the guest will read and
> obey the capability bits).


ACK.

We really should have some sort of fence mechanism for this. This patch will still work, since the command ring is 32 items long, so it should be relatively cheap to flush it (each item is a single comamnd. worse case can be 32*video_mem). There is still a race - the guest has to handle the interrupt before sending any new commands.

In the future we could introduce a new command called QXLFence and have the interrupt handler call a new io to return it just before pushing it to the ring. The fence would be used only in the server right now, but when the driver releases it it can use it to know all commands before it have been processed (note that it doesn't mean all those commands have been released).

> ---
>  server/red_worker.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 60b5471..f87967c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9493,6 +9493,11 @@ static void
> on_new_display_channel_client(DisplayChannelClient *dcc)
>      }
>      red_channel_client_ack_zero_messages_window(&dcc->common.base);
>      if (worker->surfaces[0].context.canvas) {
> +        int ring_is_empty;
> +
> +        while (red_process_commands(worker, MAX_PIPE_SIZE,
> &ring_is_empty)) {
> +        }
> +
>          red_current_flush(worker, 0);
>          push_new_primary_surface(dcc);
>          red_push_surface_image(dcc, 0);
> --
> 1.7.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>

Re: [Qemu-devel] [PATCH-v2 qemu] qxl: Add set_client_capabilities() interface to QXLInterface

[Gerd Hoffmann]

  Hi,

>  static void qxl_enter_vga_mode(PCIQXLDevice *d)
> @@ -1292,7 +1315,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
>  
>      d->mode = QXL_MODE_COMPAT;
>      d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
> -#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
> +#if QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */

Unrelated change in there.

btw: we require spice-server > 0.8.x anyway, so this #ifdef can go
anyway, but if you do it make it a separate patch please.

>      case 3: /* qxl-3 */
> +	pci_device_rev = QXL_REVISION_STABLE_V10;
> +	io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
> +	break;
> +    case 4:

Conflicts with queued patches.  Please just drop it, I'll go switch to
rev4 by default once we have a spice-server 0.12.0 release & all qemu
patches needed to use the new stuff merged.

> -#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
> +#define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V12

Likewise.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/2] qxl: Add set_client_capabilities() interface to QXLInterface

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

This new interface lets spice server inform the guest whether

(a) a client is connected
(b) what capabilities the client has

There is a fixed number (464) of bits reserved for capabilities, and
when the capabilities bits change, the QXL_INTERRUPT_CLIENT interrupt
is generated.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c2dd3b4..572daa9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -901,6 +901,26 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token)
     }
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b04
+
+/* called from spice server thread context only */
+static void interface_set_client_capabilities(QXLInstance *sin,
+					      uint8_t client_present,
+					      uint8_t caps[58])
+{
+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
+
+    qxl->shadow_rom.client_present = client_present;
+    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    qxl->rom->client_present = client_present;
+    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    qxl_rom_set_dirty(qxl);
+
+    qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
+}
+
+#endif
+
 static const QXLInterface qxl_interface = {
     .base.type               = SPICE_INTERFACE_QXL,
     .base.description        = "qxl gpu",
@@ -922,6 +942,9 @@ static const QXLInterface qxl_interface = {
     .flush_resources         = interface_flush_resources,
     .async_complete          = interface_async_complete,
     .update_area_complete    = interface_update_area_complete,
+#if SPICE_SERVER_VERSION >= 0x000b04
+    .set_client_capabilities = interface_set_client_capabilities,
+#endif
 };
 
 static void qxl_enter_vga_mode(PCIQXLDevice *d)
-- 
1.7.4

[Qemu-devel] [PATCH 2/2] Remove #ifdef QXL_COMMAND_FLAG_COMPAT_16BPP

[Søren Sandmann]

From: Søren Sandmann Pedersen <ssp@redhat.com>

We require spice >= 0.8 now, so this flag is always present.

Signed-off-by: Soren Sandmann <ssp@redhat.com>
---
 hw/qxl.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 572daa9..57e6536 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1315,11 +1315,9 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
 
     d->mode = QXL_MODE_COMPAT;
     d->cmdflags = QXL_COMMAND_FLAG_COMPAT;
-#ifdef QXL_COMMAND_FLAG_COMPAT_16BPP /* new in spice 0.6.1 */
     if (mode->bits == 16) {
         d->cmdflags |= QXL_COMMAND_FLAG_COMPAT_16BPP;
     }
-#endif
     d->shadow_rom.mode = cpu_to_le32(modenr);
     d->rom->mode = cpu_to_le32(modenr);
     qxl_rom_set_dirty(d);
-- 
1.7.4

Re: [Qemu-devel] [PATCH 1/2] qxl: Add set_client_capabilities() interface to QXLInterface

[Gerd Hoffmann]

  Hi,

> +static void interface_set_client_capabilities(QXLInstance *sin,
> +					      uint8_t client_present,
> +					      uint8_t caps[58])

Please don't use tabs for intention in qemu patches.

Fixed it up this time, added both patches to the spice patch queue.

thanks,
  Gerd