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

[Alon Levy <alevy@redhat.com>]

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