Patchwork [Spice-devel] Add new client_present and client capabilities fields to QXLRom

login
register
mail settings
Submitter Alon Levy
Date Aug. 29, 2012, 10:14 a.m.
Message ID <20120829101457.GC32547@garlic.redhat.com>
Download mbox | patch
Permalink /patch/180678/
State New
Headers show

Comments

Alon Levy - Aug. 29, 2012, 10:14 a.m.
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.):


> 
> 
> Søren
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Søren Sandmann - Aug. 29, 2012, 8:51 p.m.
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
Gerd Hoffmann - Aug. 30, 2012, 5:34 a.m.
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
Søren Sandmann - Aug. 30, 2012, 4:03 p.m.
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
Gerd Hoffmann - Aug. 31, 2012, 7:32 a.m.
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
Søren Sandmann Pedersen - Sept. 2, 2012, 9:35 p.m.
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

Patch

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