[Spice-devel] [PATCH 7/7] qxl-wddm-dod: Set VSync notification period to 200 ms

[Yuri Benditovich]

On Mon, Feb 13, 2017 at 11:50 AM, Frediano Ziglio <fziglio at redhat.com>
wrote:

> >
> > With default period of VSync interrupt notification
> > (1sec/refresh rate) the driver with device rev.4
> > has a problem when the system starts running after
> > restart. Until the issue is solved we set the notification
> > period to 200 ms, with this value both rev.3 and rev.4
> > function correctly. Final decision about notification
> > period postponed until the investigation is done.
> >
>
> Honestly all these VSync patches looks like a workaround
> just to make Windows tests happy.
>
>
That's correct. There is no other way to specify the frequency. If I find
one this makes our life easier. But till now I do not succeed.


> We don't have an h/w monitor so we are providing fake values.
> We emulate an interrupt with a timer. I think these stuff should
> be avoided just to save CPU power avoiding wake ups not
> required.
>

I also think so, but such a way we do not pass certification tests.

Also we tweak interrupts and require additional communication
> with the server to handle them. Are we sure we don't introduce
> delays if we need to process commands like updates that
> require feedback from the server?
>

When we provide interrupt indication at low rate, we minimize the impact.


> One of the "classic" usages of VSync is to handle double
> buffering used mostly on games. Are we sure that with these
> patches we don't end up with games running at 5 FPS to keep in
> sync with this emulated VSync?
>

We are not sure as we do not have setup for gaming over QXL/Spice.
It would be good to create such one.
For diagnostics/troubleshooting I would allow registry workaround to
disable this feature (VSync control) in user environment, this is to be
discussed soon.

As a minor m_VsyncFiredCounter is updated just for debugging
> purposes.
>

Yes, that's correct.


> I suspect this problem with restart is due to int_mask
> settings (m_RamHdr->int_mask = 0).
>

If we do not do that (int mask setting) during VSync notification:
1) this does not solve the problem during restart
2) this creates a problem for functional interrupts - in case the
functional interrupt comes when DPC is queued by VSync notification, the
functional interrupt will not be able to queue its DPC and waiting GDI
thread will not be notified.

It looks like the problem with restart happens when responsiveness of
client is slow. I'm trying to debug it, but this is not so simple as this
happens during system start and logging is currently problematic. WPP can
help with this and it is in to-do list.

If you think this int mask setting is incorrect, please explain why (even
offline), this is important.


>
> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > ---
> >  qxldod/QxlDod.cpp | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index fcca7d1..6a50265 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -4972,10 +4972,12 @@ VOID QxlDod::EnableVsync(BOOLEAN bEnable)
> >          }
> >          else
> >          {
> > +            // set notification period to 200 ms for now, see commit
> comment
> > for details
> > +            LONG val = 200;
> >              LARGE_INTEGER li;
> > -            LONG period = 1000 / VSYNC_RATE;
> > +            LONG period = val;
> >              DbgPrint(TRACE_LEVEL_WARNING, ("Enabled VSync(fired %d)\n",
> >              m_VsyncFiredCounter));
> > -            li.QuadPart = -10000000 / VSYNC_RATE;
> > +            li.QuadPart = -10000 * val;
> >              KeSetTimerEx(&m_VsyncTimer, li, period, &m_VsyncTimerDpc);
> >          }
> >      }
>
> Frediano
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170213/ed982b34/attachment.html>