[Spice-devel] [RFC qxl-wddm-dod 0/3] Synchronization of display mode change and pushing drawables

[Frediano Ziglio]

The warning is mainly there to notify the guest is sending commands for a not existing surface. 
This should not create issues unless the surface is used as source for other commands. 
The warning was removed as this allows the guest to fill server logs and some drivers send 
some spurious commands. 
There are no leaks in either 0.12 and 0.13. 
Saying that however I consider this something to be fixed. Considering this sequence: 
1- system draw something 
2- system change resolution 
3- system draw something else 
is possible that command generated from 1 is deferred after the resolution is changed basically 
doing the draw on a missing or wrong surface. This potentially can lead so some wrong rendering. 
Your patches seems to me a bit overwhelming and complex for the task. 
Considering that, as you reported, without deferring the drawing the commands are serialized 
and there are no warning this means that commands queued in step 3 cannot be executed before 
2 finishes (as resolution change is not deferred) so the only problem you can have is that 
commands queued in 1 are executed after/while step 2. I would simply add a "generation" field 
(increased before executing resolution change) attached to every command and checked in 
the working thread (so being able to ignore a command is the command generation is not 
the current one). This solution would be much easier to implement and also could be used 
to ignoring command executed after the stop command (just increase the generation during 
the stop request). 

Frediano 

----- Original Message -----

> It would be very helpful to receive a feedback regarding this problem,
> especially:
> - Do you see the reason why this erroneous flow on 'old' spice server
> (0.12.4, for example) can lead to significant device memory leak.
> - If yes: Is spice server 0.12.4 still actual in the field?
> - The warning was removed from 0.13 server - does it mean this flow does not
> present any problem anymore?
> - Finally - do we need this kind of fix in the driver or not?

> Thanks,
> Yuri

> On Sun, Apr 16, 2017 at 10:43 PM, Yuri Benditovich <
> yuri.benditovich at daynix.com > wrote:

> > This is result of investigation of HCK tests "WHQL FPO optimization check
> > for
> > kernel video drivers" on
> 
> > latest upstream driver.
> 
> > These tests execute in aloop fast change of display mode and full screen
> > rendering between changes.
> 
> > When they run on qxl-wddm-dod driver with separate thread that pushes
> > drawables to the host and with
> 
> > spice server 0.12.4 (rhel-7.2) there are warning printouts of spice server:
> 
> > "rendering incorrect from now on: get_drawable" and sometimes (3/10) the
> > guest system
> 
> > stops responding (qxl-wddm-dod driver is in infinite wait for memory
> > allocation).
> 
> > In case of fast change of display mode it is possible that the driver
> > creates
> > drawable objects
> 
> > in selected display mode but sends them down in the middle of the change or
> > after change of display mode.
> 
> > This can cause failure in validation of drawable in spice server.
> 
> > The same tests do not produce any warnings and system stuck is not
> > reproduced
> > with 0.16 release of the
> 
> > driver (before implementation of separate thread)
> 
> > When these tests run on spice server 0.12.8 there are the same warnings but
> > guest stuck was not
> 
> > reproduced in 10 runs.
> 
> > In spice server 0.13 these warnings removed from spice server code and
> > guest
> > stuck also was not
> 
> > reproduced in 10 runs.
> 
> > It is unclear which commit in spice server 0.12 makes the change, if any.
> 
> > To avoid inconsistency between drawables and current display mode this set
> > of
> > patches implements
> 
> > synchronization between display mode change and sending drawables to the
> > host.
> 
> > Each successful processing of rendering callback creates array of
> > drawables,
> > places it to internal
> 
> > ring, increments counter of outstanding operations and triggers processing
> > in
> > separate thread.
> 
> > The thread processes drawables, pushes them to command ring and decrements
> > the counter of outstanding
> 
> > operations.
> 
> > When the OS changes display mode, the driver waits until the thread pushes
> > all the queued drawables to the
> 
> > host, disables queuing of drawables, executes display mode switch and then
> > enables queuing of drawables.
> 

> > Yuri Benditovich (3):
> 
> > Move code for discarding drawable to separate procedure
> 
> > Implement rendering state machine
> 
> > Synchronize display mode change and pushing drawables
> 

> > qxldod/QxlDod.cpp | 35 +++++++++++++++----
> 
> > qxldod/QxlDod.h | 102
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> > 2 files changed, 130 insertions(+), 7 deletions(-)
> 

> > --
> 
> > 2.7.0.windows.1
> 

> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170426/7c30d33b/attachment.html>