[Spice-devel] [PATCH v3 5/6] qxl-wddm-dod: Timer-based VSync interrupt indication

Wed Feb 22 12:18:20 UTC 2017

> On Thu, Feb 16, 2017 at 5:54 PM, Frediano Ziglio < fziglio at redhat.com >
> wrote:

> > >
> 
> > > In case the driver supports VSync control feature, it
> 
> > > maintains timer for VSync interrupt indication.
> 
> > > In further commits this timer will be started upon
> 
> > > class driver request. The interrupt notification and
> 
> > > related DPC do not access device registers.
> 
> > >
> 
> > > Signed-off-by: Yuri Benditovich < yuri.benditovich at daynix.com >
> 
> > > ---
> 
> > > qxldod/QxlDod.cpp | 82
> 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> > > qxldod/QxlDod.h | 14 ++++++++++
> 
> > > 2 files changed, 96 insertions(+)
> 
> > >
> 
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> 
> > > index cd22446..9175300 100755
> 
> > > --- a/qxldod/QxlDod.cpp
> 
> > > +++ b/qxldod/QxlDod.cpp
> 
> > > @@ -82,6 +82,12 @@ QxlDod::QxlDod(_In_ DEVICE_OBJECT*
> > > pPhysicalDeviceObject)
> 
> > > : m_pPhysicalDevice(pP
> 
> > > RtlZeroMemory(m_CurrentModes, sizeof(m_CurrentModes));
> 
> > > RtlZeroMemory(&m_PointerShape, sizeof(m_PointerShape));
> 
> > > m_pHWDevice = NULL;
> 
> > > +
> 
> > > + KeInitializeDpc(&m_VsyncTimerDpc, VsyncTimerProcGate, this);
> 
> > > + KeInitializeTimer(&m_VsyncTimer);
> 
> > > + m_VsyncFiredCounter = 0;
> 
> > > + m_bVsyncEnabled = FALSE;
> 
> > > +
> 
> > > DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
> 
> > > }
> 
> > >
> 
> > > @@ -198,6 +204,7 @@ NTSTATUS QxlDod::StopDevice(VOID)
> 
> > > {
> 
> > > PAGED_CODE();
> 
> > > m_Flags.DriverStarted = FALSE;
> 
> > > + EnableVsync(FALSE);
> 
> > > return STATUS_SUCCESS;
> 
> > > }
> 
> > >
> 
> > > @@ -518,6 +525,7 @@ NTSTATUS QxlDod::QueryAdapterInfo(_In_ CONST
> 
> > > DXGKARG_QUERYADAPTERINFO* pQueryAda
> 
> > > pDriverCaps->PointerCaps.Color = 1;
> 
> > >
> 
> > > pDriverCaps->SupportNonVGA = m_pHWDevice->IsBIOSCompatible();
> 
> > > + pDriverCaps->SchedulingCaps.VSyncPowerSaveAware =
> 
> > > g_bSupportVSync;
> 
> > >
> 

> > I think we could set this to TRUE. If OS supports VSync will deactivate
> 
> > if not should do nothing or try to deactivate.
> 
> > Could be if set to FALSE that OS still expect VSync coming or don't
> 
> > enable some energy saving policy.
> 

> Setting it to TRUE when VSync is not supported may create some side-effect in
> Windows,
> as this combination is not expected. Setting it to FALSE when VSync is
> supported also does
> not make too much sense as then the driver will send indications also when
> the system
> is completely inactive. So, only meaningful choice for us is to keep it equal
> to VSync control.

> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s 1\n", __FUNCTION__));
> 
> > > return STATUS_SUCCESS;
> 
> > > @@ -4813,6 +4821,14 @@ BOOLEAN QxlDevice::InterruptRoutine(_In_
> 
> > > PDXGKRNL_INTERFACE pDxgkInterface, _In_
> 
> > > }
> 
> > >
> 
> > > QXL_NON_PAGED
> 
> > > +VOID QxlDevice::VSyncInterruptPostProcess(_In_ PDXGKRNL_INTERFACE
> 
> > > pDxgkInterface)
> 
> > > +{
> 
> > > + if (!pDxgkInterface->DxgkCbQueueDpc(pDxgkInterface->DeviceHandle)) {
> 
> > > + DbgPrint(TRACE_LEVEL_WARNING, ("---> %s can't enqueue DPC, pending
> 
> > > interrupts %X\n", __FUNCTION__, m_Pending));
> 
> > > + }
> 
> > > +}
> 
> > > +
> 
> > > +QXL_NON_PAGED
> 
> > > VOID QxlDevice::DpcRoutine(PVOID)
> 
> > > {
> 
> > > LONG intStatus = InterlockedExchange(&m_Pending, 0);
> 
> > > @@ -4914,3 +4930,69 @@ NTSTATUS
> 
> > > HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
> 
> > > }
> 
> > > return Status;
> 
> > > }
> 
> > > +
> 
> > > +// Vga device does not generate interrupts
> 
> > > +QXL_NON_PAGED VOID VgaDevice::VSyncInterruptPostProcess(_In_
> 
> > > PDXGKRNL_INTERFACE pxface)
> 
> > > +{
> 
> > > + pxface->DxgkCbQueueDpc(pxface->DeviceHandle);
> 
> > > +}
> 
> > > +
> 
> > > +QXL_NON_PAGED VOID QxlDod::IndicateVSyncInterrupt()
> 
> > > +{
> 

> > Maybe is more flexible if this function returns BOOLEAN.
> 

> > > + DXGKARGCB_NOTIFY_INTERRUPT_DATA data = {};
> 
> > > + data.InterruptType = DXGK_INTERRUPT_DISPLAYONLY_VSYNC;
> 
> > > + m_DxgkInterface.DxgkCbNotifyInterrupt(m_DxgkInterface.DeviceHandle,
> 
> > > &data);
> 
> > > + m_pHWDevice->VSyncInterruptPostProcess(&m_DxgkInterface);
> 
> > > +}
> 
> > > +
> 
> > > +QXL_NON_PAGED BOOLEAN QxlDod::VsyncTimerSynchRoutine(PVOID context)
> 
> > > +{
> 
> > > + QxlDod* pQxl = reinterpret_cast<QxlDod*>(context);
> 
> > > + pQxl->IndicateVSyncInterrupt();
> 
> > > + return FALSE;
> 

> > Here would be
> 

> > return pQxl->IndicateVSyncInterrupt();
> 

> > so we could handle the return value from the member function (if needed
> 
> > in the future).
> 

> This procedure currently returns boolean just because Windows
> synch routine prototype defined such a way without any reason.
> There is no meaningful information that we can pass back on this boolean.
> When synch procedure needs to return something little bigger than boolean,
> local context passed to the routine does the job.

> > > +}
> 
> > > +
> 
> > > +QXL_NON_PAGED VOID QxlDod::VsyncTimerProc()
> 
> > > +{
> 
> > > + BOOLEAN bDummy;
> 
> > > + DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> 
> > > + if (m_bVsyncEnabled && m_AdapterPowerState == PowerDeviceD0)
> 
> > > + {
> 
> > > + m_DxgkInterface.DxgkCbSynchronizeExecution(
> 
> > > + m_DxgkInterface.DeviceHandle,
> 
> > > + VsyncTimerSynchRoutine,
> 
> > > + this,
> 
> > > + 0,
> 
> > > + &bDummy
> 
> > > + );
> 
> > > + INCREMENT_VSYNC_COUNTER(&m_VsyncFiredCounter);
> 
> > > + }
> 
> > > +}
> 
> > > +
> 
> > > +VOID QxlDod::EnableVsync(BOOLEAN bEnable)
> 
> > > +{
> 
> > > + PAGED_CODE();
> 
> > > + if (g_bSupportVSync)
> 
> > > + {
> 
> > > + m_bVsyncEnabled = bEnable;
> 
> > > + if (!m_bVsyncEnabled)
> 
> > > + {
> 
> > > + DbgPrint(TRACE_LEVEL_WARNING, ("Disabled VSync(fired %d)\n",
> 
> > > InterlockedExchange(&m_VsyncFiredCounter, 0)));
> 
> > > + KeCancelTimer(&m_VsyncTimer);
> 
> > > + }
> 
> > > + else
> 
> > > + {
> 
> > > + LARGE_INTEGER li;
> 
> > > + LONG period = 1000 / VSYNC_RATE;
> 
> > > + DbgPrint(TRACE_LEVEL_WARNING, ("Enabled VSync(fired %d)\n",
> 
> > > m_VsyncFiredCounter));
> 
> > > + li.QuadPart = -10000000 / VSYNC_RATE;
> 
> > > + KeSetTimerEx(&m_VsyncTimer, li, period, &m_VsyncTimerDpc);
> 
> > > + }
> 
> > > + }
> 
> > > +}
> 
> > > +
> 
> > > +QXL_NON_PAGED VOID QxlDod::VsyncTimerProcGate(_In_ _KDPC *dpc, _In_
> > > PVOID
> 
> > > context, _In_ PVOID arg1, _In_ PVOID arg2)
> 
> > > +{
> 
> > > + QxlDod* pQxl = reinterpret_cast<QxlDod*>(context);
> 
> > > + pQxl->VsyncTimerProc();
> 
> > > +}
> 
> > > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> 
> > > index 53724e8..9cfb6de 100755
> 
> > > --- a/qxldod/QxlDod.h
> 
> > > +++ b/qxldod/QxlDod.h
> 
> > > @@ -239,6 +239,7 @@ public:
> 
> > > QXL_NON_PAGED virtual BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE
> 
> > > pDxgkInterface, _In_ ULONG MessageNumber) = 0;
> 
> > > QXL_NON_PAGED virtual VOID DpcRoutine(PVOID) = 0;
> 
> > > QXL_NON_PAGED virtual VOID ResetDevice(void) = 0;
> 
> > > + QXL_NON_PAGED virtual VOID VSyncInterruptPostProcess(_In_
> 
> > > PDXGKRNL_INTERFACE) = 0;
> 
> > > virtual NTSTATUS AcquireFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode) {
> 
> > > return STATUS_SUCCESS; }
> 
> > > virtual NTSTATUS ReleaseFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode) {
> 
> > > return STATUS_SUCCESS; }
> 
> > >
> 
> > > @@ -306,6 +307,7 @@ public:
> 
> > > QXL_NON_PAGED BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE
> 
> > > pDxgkInterface, _In_ ULONG MessageNumber);
> 
> > > QXL_NON_PAGED VOID DpcRoutine(PVOID);
> 
> > > QXL_NON_PAGED VOID ResetDevice(VOID);
> 
> > > + QXL_NON_PAGED VOID VSyncInterruptPostProcess(_In_ PDXGKRNL_INTERFACE);
> 
> > > NTSTATUS AcquireFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode);
> 
> > > NTSTATUS ReleaseFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode);
> 
> > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
> 
> > > pSetPointerShape);
> 
> > > @@ -358,8 +360,10 @@ enum {
> 
> > >
> 
> > > #ifdef DBG
> 
> > > #define RESOURCE_TYPE(res, val) do { res->type = val; } while (0)
> 
> > > +#define INCREMENT_VSYNC_COUNTER(counter) InterlockedIncrement(counter)
> 
> > > #else
> 
> > > #define RESOURCE_TYPE(res, val)
> 
> > > +#define INCREMENT_VSYNC_COUNTER(counter)
> 
> > > #endif
> 
> > >
> 
> > > typedef struct Resource Resource;
> 
> > > @@ -480,6 +484,7 @@ public:
> 
> > > QXL_NON_PAGED BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE
> 
> > > pDxgkInterface, _In_ ULONG MessageNumber);
> 
> > > QXL_NON_PAGED VOID DpcRoutine(PVOID);
> 
> > > QXL_NON_PAGED VOID ResetDevice(VOID);
> 
> > > + QXL_NON_PAGED VOID VSyncInterruptPostProcess(_In_ PDXGKRNL_INTERFACE);
> 
> > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
> 
> > > pSetPointerShape);
> 
> > > NTSTATUS SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION*
> 
> > > pSetPointerPosition);
> 
> > > NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap);
> 
> > > @@ -622,6 +627,10 @@ private:
> 
> > > DXGKARG_SETPOINTERSHAPE m_PointerShape;
> 
> > >
> 
> > > HwDeviceInterface* m_pHWDevice;
> 
> > > + KTIMER m_VsyncTimer;
> 
> > > + KDPC m_VsyncTimerDpc;
> 
> > > + BOOLEAN m_bVsyncEnabled;
> 
> > > + LONG m_VsyncFiredCounter;
> 
> > > public:
> 
> > > QxlDod(_In_ DEVICE_OBJECT* pPhysicalDeviceObject);
> 
> > > ~QxlDod(void);
> 
> > > @@ -719,6 +728,7 @@ public:
> 
> > > {
> 
> > > return
> 
> > > m_DxgkInterface.DxgkCbAcquirePostDisplayOwnership(m_DxgkInterface.DeviceHandle,
> 
> > > &DispInfo);
> 
> > > }
> 
> > > + VOID EnableVsync(BOOLEAN bEnable);
> 
> > > private:
> 
> > > VOID CleanUp(VOID);
> 
> > > NTSTATUS CheckHardware();
> 
> > > @@ -744,6 +754,10 @@ private:
> 
> > > NTSTATUS IsVidPnSourceModeFieldsValid(CONST D3DKMDT_VIDPN_SOURCE_MODE*
> 
> > > pSourceMode) const;
> 
> > > NTSTATUS IsVidPnPathFieldsValid(CONST D3DKMDT_VIDPN_PRESENT_PATH* pPath)
> 
> > > const;
> 
> > > NTSTATUS RegisterHWInfo(_In_ ULONG Id);
> 
> > > + QXL_NON_PAGED VOID VsyncTimerProc();
> 
> > > + static QXL_NON_PAGED VOID VsyncTimerProcGate(_In_ _KDPC *dpc, _In_
> > > PVOID
> 
> > > context, _In_ PVOID arg1, _In_ PVOID arg2);
> 
> > > + QXL_NON_PAGED VOID IndicateVSyncInterrupt();
> 
> > > + static QXL_NON_PAGED BOOLEAN VsyncTimerSynchRoutine(PVOID context);
> 
> > > };
> 
> > >
> 
> > > NTSTATUS
> 

> > Otherwise looks good.
> 

Acked-by: Frediano Ziglio <fziglio at redhat.com> 

Frediano 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170222/120f89d3/attachment.html>