[Spice-devel] [win32/vd_agent v2 2/3] introduce turn_monitor_off method of WDDM interface

[Yuri Benditovich]

On Fri, Mar 8, 2019 at 1:34 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > Adding method 'turn_monitor_off' to WDDM interface.
> > It sends QXL_ESCAPE_MONITOR_CONFIG escape with zeroed
> > display size to qxl-wddm-dod driver.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > ---
> >  vdagent/display_configuration.cpp | 18 ++++++++++++++++++
> >  vdagent/display_configuration.h   |  2 +-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/vdagent/display_configuration.cpp
> > b/vdagent/display_configuration.cpp
> > index bb2fb80..420836b 100644
> > --- a/vdagent/display_configuration.cpp
> > +++ b/vdagent/display_configuration.cpp
> > @@ -488,6 +488,24 @@ bool WDDMInterface::update_monitor_config(LPCTSTR
> > device_name, DisplayMode* disp
> >
> >  }
> >
> > +bool WDDMInterface::turn_monitor_off(LPCTSTR device_name)
> > +{
> > +    vd_printf("for %S", device_name);
>
> <ot>
> Off topic: It seems that the code is assuming that the strings
> are always unicode. I agree they should, maybe we should start
> using LPCWSTR instead of LPCTSTR to make sure nobody will try
> to compile without unicode? In this case the code would use
> device_name as multi-byte but vd_printf would assume the string
> is wide causing possible corruptions. Or an easy safety against
> it could be a check like
>
> #if !defined(UNICODE) || !defined(_UNICODE)
> #error This code is designed to use Unicode
> #endif
>
> in some main header file (like common/vdlog.h).
>
> Also if somebody is using some internationalization I think
> using single-byte for the output file would cause some conversions
> leading to replace some no-ANSI character to be replaced by
> question marks ('?'). It would make sense to move the log file
> to Unicode too to avoid that.
> </ot>
>
> > +    if (!_send_monitors_config)
> > +    {
>
> Minor style: in most of occurrences the open bracket is on the
> same line ...
>
> > +        vd_printf("do nothing as _send_monitors_config is off");
> > +        return false;
> > +    }
> > +
> > +    WDDMMonitorConfigEscape wddm_escape(NULL);
> > +    if (escape(device_name, &wddm_escape, sizeof(wddm_escape))) {
>
> ... like this
>
> > +        return true;
> > +    }
> > +
> > +    vd_printf("%S failed", device_name);
> > +    return false;
> > +}
> > +
> >  LONG WDDMInterface::update_display_settings()
> >  {
> >      LONG error(0);
> > diff --git a/vdagent/display_configuration.h
> > b/vdagent/display_configuration.h
> > index 7b5578e..8f3b93a 100644
> > --- a/vdagent/display_configuration.h
> > +++ b/vdagent/display_configuration.h
> > @@ -150,7 +150,6 @@ public:
> >      bool update_monitor_config(LPCTSTR device_name, DisplayMode* mode,
> >      DEVMODE* dev_mode);
> >      bool update_dev_mode_position(LPCTSTR device_name, DEVMODE * dev_mode,
> >      LONG x, LONG y);
> >      void update_config_path();
> > -
>
> Minor: spurious change
>
> >  private:
> >      bool init_d3d_api();
> >      D3D_HANDLE adapter_handle(LPCTSTR device_name);
> > @@ -160,6 +159,7 @@ private:
> >
> >      void close_adapter(D3D_HANDLE handle);
> >      bool escape(LPCTSTR device_name, void* data, UINT sizeData);
> > +    bool turn_monitor_off(LPCTSTR device_name);
> >
> >      //GDI Function pointers
> >      PFND3DKMT_OPENADAPTERFROMHDC _pfnOpen_adapter_hdc;
>
> Otherwise the patch is fine. Do you want me to fix these minor styles
> and merge?

Yes, thanks

>
> Frediano