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

Submitted by Yuri Benditovich on Feb. 12, 2017, 1:09 p.m.

Details

Message ID 1486904994-169784-8-git-send-email-yuri.benditovich@daynix.com
State New
Headers show
Series "Changes for Win10 HLK test" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich Feb. 12, 2017, 1:09 p.m.
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.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 qxldod/QxlDod.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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

Comments

> 
> 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.

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.
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?
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?
As a minor m_VsyncFiredCounter is updated just for debugging
purposes.
I suspect this problem with restart is due to int_mask
settings (m_RamHdr->int_mask = 0).

> Signed-off-by: Yuri Benditovich <yuri.benditovich@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
On Mon, Feb 13, 2017 at 11:50 AM, Frediano Ziglio <fziglio@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@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
>