<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><br><div class="gmail_extra">On Thu, Aug 11, 2016 at 1:23 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>><br>
> The Direct3D 9 API operates on either the Windows XP display driver<br>
> model (XPDM) or the Windows Vista display driver model (WDDM), depending<br>
> on the operating system installed.<br>
><br>
> This patch encapsulates the current XPDM interface implementation into<br>
> XPDMInterface class which inherits DisplayConfig class. This patch<br>
> makes it easier to introduce WDDM interface to Vdagent in future<br>
> patches.<br>
><br>
> Based on a patch by Sandy Stutsman <<a href="mailto:sstutsma@redhat.com" target="_blank">sstutsma@redhat.com</a>><br>
><br>
> Signed-off-by: Dmitry Fleytman <<a href="mailto:dfleytma@redhat.com" target="_blank">dfleytma@redhat.com</a>><br>
> Signed-off-by: Sameeh Jubran <<a href="mailto:sameeh@daynix.com" target="_blank">sameeh@daynix.com</a>><br>
> ---<br>
>  vdagent/desktop_layout.cpp        | 148 +++++++-------------------------<br>
>  vdagent/desktop_layout.h          |   9 +-<br>
>  vdagent/display_configuration.cpp | 174<br>
>  ++++++++++++++++++++++++++++++++++++++<br>
>  vdagent/display_configuration.h   |  36 ++++++++<br>
>  4 files changed, 245 insertions(+), 122 deletions(-)<br>
><br>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp<br>
> index a7666ca..9c3e873 100644<br>
> --- a/vdagent/desktop_layout.cpp<br>
> +++ b/vdagent/desktop_layout.cpp<br>
> @@ -18,6 +18,7 @@<br>
>  #include <spice/qxl_windows.h><br>
>  #include <spice/qxl_dev.h><br>
>  #include "desktop_layout.h"<br>
> +#include "display_configuration.h"<br>
>  #include "vdlog.h"<br>
><br>
>  #ifdef __MINGW32__<br>
> @@ -35,15 +36,17 @@ void DisplayMode::set_res(DWORD width, DWORD height,<br>
> DWORD depth)<br>
>  DesktopLayout::DesktopLayout()<br>
>      : _total_width (0)<br>
>      , _total_height (0)<br>
> -    , _send_monitors_position(false)<br>
> +    , _display_config (NULL)<br>
>  {<br>
>      MUTEX_INIT(_mutex);<br>
> +    _display_config = DisplayConfig::create_config();<br>
>      get_displays();<br>
>  }<br>
><br>
>  DesktopLayout::~DesktopLayout()<br>
>  {<br>
>      clean_displays();<br>
> +    delete _display_config;<br>
>  }<br>
><br>
>  void DesktopLayout::get_displays()<br>
> @@ -59,6 +62,7 @@ void DesktopLayout::get_displays()<br>
>          unlock();<br>
>          return;<br>
>      }<br>
> +    _display_config->update_config_path();<br>
>      clean_displays();<br>
>      ZeroMemory(&dev_info, sizeof(dev_info));<br>
>      dev_info.cb = sizeof(dev_info);<br>
> @@ -82,12 +86,13 @@ void DesktopLayout::get_displays()<br>
>                  _displays[i] = NULL;<br>
>              }<br>
>          }<br>
> -        attached = !!(dev_info.StateFlags &<br>
> DISPLAY_DEVICE_ATTACHED_TO_DESKTOP);<br>
> +        attached = _display_config->is_attached(&dev_info);<br>
> +<br>
>          EnumDisplaySettings(dev_info.DeviceName, ENUM_CURRENT_SETTINGS,<br>
>          &mode);<br>
>          _displays[display_id] = new DisplayMode(mode.dmPosition.x,<br>
>          mode.dmPosition.y,<br>
>                                                  mode.dmPelsWidth,<br>
>                                                  mode.dmPelsHeight,<br>
>                                                  mode.dmBitsPerPel,<br>
>                                                  attached);<br>
> -        update_monitor_config(dev_info.DeviceName, _displays[display_id]);<br>
> +        _display_config->update_monitor_config(dev_info.DeviceName,<br>
> _displays[display_id], &mode);<br>
>      }<br>
>      normalize_displays_pos();<br>
>      unlock();<br>
> @@ -121,6 +126,7 @@ void DesktopLayout::set_displays()<br>
>          unlock();<br>
>          return;<br>
>      }<br>
> +    _display_config->update_config_path();<br>
>      ZeroMemory(&dev_info, sizeof(dev_info));<br>
>      dev_info.cb = sizeof(dev_info);<br>
>      ZeroMemory(&dev_mode, sizeof(dev_mode));<br>
> @@ -146,28 +152,32 @@ void DesktopLayout::set_displays()<br>
>              break;<br>
>          }<br>
>          DisplayMode * mode(_<a href="http://displays.at" rel="noreferrer" target="_blank">displays.at</a>(display_id));<br>
> -        if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode, normal_x,<br>
> normal_y, true)) {<br>
> +        if (!init_dev_mode(dev_info.DeviceName, &dev_mode, mode)) {<br>
>              vd_printf("No suitable mode found for display %S",<br>
>              dev_info.DeviceName);<br>
>              break;<br>
>          }<br>
>          vd_printf("Set display mode %lux%lu", dev_mode.dmPelsWidth,<br>
>          dev_mode.dmPelsHeight);<br>
> -        LONG ret = ChangeDisplaySettingsEx(dev_info.DeviceName, &dev_mode,<br>
> NULL,<br>
> -                                           CDS_UPDATEREGISTRY | CDS_NORESET,<br>
> NULL);<br>
> -        if (ret == DISP_CHANGE_SUCCESSFUL) {<br>
> +        if (_display_config->update_dev_mode_position(dev_info.DeviceName,<br>
> &dev_mode,<br>
> +                                                     mode->_pos_x -<br>
> normal_x,<br>
> +                                                     mode->_pos_y -<br>
> normal_y)) {<br>
>              dev_sets++;<br>
> -            update_monitor_config(dev_info.DeviceName, mode);<br>
> +            _display_config->update_monitor_config(dev_info.DeviceName,<br>
> mode, &dev_mode);<br>
>          }<br>
>          if (!is_qxl) {<br>
>              display_id++;<br>
>          }<br>
>      }<br>
>      if (dev_sets) {<br>
> -        ChangeDisplaySettingsEx(NULL, NULL, NULL, 0, NULL);<br>
> +        _display_config->update_display_settings();<br>
>          normalize_displays_pos();<br>
>      }<br>
>      unlock();<br>
>  }<br>
><br>
> +void DesktopLayout::set_position_configurable(bool flag) {<br>
> +    _display_config->set_monitors_config(flag);<br>
> +}<br>
> +<br>
>  // Normalize all display positions to non-negative coordinates and update<br>
>  total width and height of<br>
>  // the virtual desktop. Caller is responsible to lock() & unlock().<br>
>  void DesktopLayout::normalize_displays_pos()<br>
> @@ -265,125 +275,29 @@ bool DesktopLayout::get_qxl_device_id(WCHAR*<br>
> device_key, DWORD* device_id)<br>
>      return key_found;<br>
>  }<br>
><br>
> -bool DesktopLayout::init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode,<br>
> DisplayMode* mode,<br>
> -                                  LONG normal_x, LONG normal_y, bool<br>
> set_pos)<br>
> +bool DesktopLayout::init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode,<br>
> DisplayMode* mode)<br>
>  {<br>
> -    DWORD closest_diff = -1;<br>
> -    DWORD best = -1;<br>
> -    QXLEscapeSetCustomDisplay custom;<br>
> -    HDC hdc = NULL;<br>
> -    LONG ret;<br>
> -<br>
>      ZeroMemory(dev_mode, sizeof(DEVMODE));<br>
>      dev_mode->dmSize = sizeof(DEVMODE);<br>
> -    if (!mode || !mode->_attached) {<br>
> -        //Detach monitor<br>
> -        dev_mode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_POSITION;<br>
> -        return true;<br>
> -    }<br>
> -<br>
> -    hdc = CreateDC(dev_name, NULL, NULL, NULL);<br>
> -    if (!hdc) {<br>
> -        // for some reason, windows want those 3 flags to enable monitor<br>
> -        dev_mode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_POSITION;<br>
> -        dev_mode->dmPelsWidth = mode->_width;<br>
> -        dev_mode->dmPelsHeight = mode->_height;<br>
> -        ret = ChangeDisplaySettingsEx(dev_name, dev_mode, NULL,<br>
> CDS_UPDATEREGISTRY, NULL);<br>
> -        if (ret == DISP_CHANGE_BADMODE) {<br>
> -            // custom resolution might not be set yet, use known resolution<br>
> -            // FIXME: this causes client temporary resize... a<br>
> -            // solution would involve passing custom resolution before<br>
> -            // driver initialization, perhaps through registry<br>
> -            dev_mode->dmPelsWidth = 640;<br>
> -            dev_mode->dmPelsHeight = 480;<br>
> -            ret = ChangeDisplaySettingsEx(dev_name, dev_mode, NULL,<br>
> CDS_UPDATEREGISTRY, NULL);<br>
> -        }<br>
><br>
> -        vd_printf("attach %ld", ret);<br>
> -        hdc = CreateDC(dev_name, NULL, NULL, NULL);<br>
> +    //Update monitor state<br>
> +    MONITOR_STATE monitor_state = (!mode || !mode->_attached)?<br>
> MONITOR_DETACHED : MONITOR_ATTACHED;<br>
> +    _display_config->set_monitor_state(dev_name, dev_mode, monitor_state);<br><br></div></div>This looks like an extra call to ChangeDisplaySettingsEx if monitor is detached.<br>
This has nothing to do with encapsulation.<br></blockquote><div>It's instead of the one that was deleted which attaches the monitor.</div></div></div></div></blockquote><div>So there was a previous patch that deleted some code causing regression?<br></div><div>In this case would be better to revert the offending patch instead of putting<br></div><div>a fix into an encapsulation patch.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
> +    if (monitor_state == MONITOR_DETACHED) {<br>
> +        return true;<br>
>      }<br>
><br>
> -    if (!hdc) {<br>
> -        vd_printf("failed to create DC");<br>
> +    // Update custom resolution<br>
> +    dev_mode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_POSITION;<br>
> +    dev_mode->dmPelsWidth = mode->_width;<br>
> +    dev_mode->dmPelsHeight = mode->_height;<br>
> +    dev_mode->dmBitsPerPel = mode->_depth;<br>
> +    if (!_display_config->custom_display_escape(dev_name, dev_mode))<br>
>          return false;<br>
> -    } else {<br>
> -        // Update custom resolution<br>
> -        custom.xres = mode->_width;<br>
> -        custom.yres = mode->_height;<br>
> -        custom.bpp = mode->_depth;<br>
> -<br>
> -        int err = ExtEscape(hdc, QXL_ESCAPE_SET_CUSTOM_DISPLAY,<br>
> -                            sizeof(QXLEscapeSetCustomDisplay),<br>
> (LPCSTR)&custom, 0, NULL);<br>
> -        if (err <= 0) {<br>
> -            vd_printf("can't set custom display, perhaps an old driver");<br>
> -        }<br>
> -        DeleteDC(hdc);<br>
> -    }<br>
> -<br>
> -    // force refresh mode table<br>
> -    DEVMODE tempDevMode;<br>
> -    ZeroMemory(&tempDevMode, sizeof (tempDevMode));<br>
> -    tempDevMode.dmSize = sizeof(DEVMODE);<br>
> -    EnumDisplaySettings(dev_name, 0xffffff, &tempDevMode);<br>
> -<br>
> -    //Find the closest size which will fit within the monitor<br>
> -    for (DWORD i = 0; EnumDisplaySettings(dev_name, i, dev_mode); i++) {<br>
> -        if (dev_mode->dmPelsWidth > mode->_width ||<br>
> -            dev_mode->dmPelsHeight > mode->_height ||<br>
> -            dev_mode->dmBitsPerPel != mode->_depth) {<br>
> -            continue;<br>
> -        }<br>
> -        DWORD wdiff = mode->_width - dev_mode->dmPelsWidth;<br>
> -        DWORD hdiff = mode->_height - dev_mode->dmPelsHeight;<br>
> -        DWORD diff = wdiff * wdiff + hdiff * hdiff;<br>
> -        if (diff < closest_diff) {<br>
> -            closest_diff = diff;<br>
> -            best = i;<br>
> -        }<br>
> -    }<br>
> -    if (best == (DWORD)-1 || !EnumDisplaySettings(dev_name, best, dev_mode))<br>
> {<br>
> -        return false;<br>
> -    }<br>
> -    dev_mode->dmFields = DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT;<br>
> -    if (set_pos) {<br>
> -        //Convert the position so that the primary is always at (0,0)<br>
> -        dev_mode->dmPosition.x = mode->_pos_x - normal_x;<br>
> -        dev_mode->dmPosition.y = mode->_pos_y - normal_y;<br>
> -        dev_mode->dmFields |= DM_POSITION;<br>
> -    }<br>
><br>
>      // update current DisplayMode (so mouse scaling works properly)<br>
>      mode->_width = dev_mode->dmPelsWidth;<br>
>      mode->_height = dev_mode->dmPelsHeight;<br>
> -<br>
>      return true;<br>
> -}<br>
> -<br>
> -bool DesktopLayout::update_monitor_config(LPCTSTR dev_name, DisplayMode*<br>
> mode)<br>
> -{<br>
> -    QXLHead monitor_config;<br>
> -<br>
> -    if (!mode || !mode->get_attached())<br>
> -        return false;<br>
> -<br>
> -    //Don't configure monitors unless the client supports it<br>
> -    if(!_send_monitors_position) return FALSE;<br>
> -<br>
> -    HDC hdc = CreateDC(dev_name, NULL, NULL, NULL);<br>
> -<br>
> -    memset(&monitor_config, 0, sizeof(monitor_config));<br>
> -    monitor_config.x = mode->_pos_x;<br>
> -    monitor_config.y = mode->_pos_y;<br>
> -    monitor_config.width = mode->_width;<br>
> -    monitor_config.height = mode->_height;<br>
> -<br>
> -    int err = ExtEscape(hdc, QXL_ESCAPE_MONITOR_CONFIG,<br>
> -        sizeof(QXLHead), (LPCSTR) &monitor_config, 0, NULL);<br>
> -<br>
> -    if (err < 0){<br>
> -        vd_printf("can't update monitor config, may have an older driver");<br>
> -    }<br>
><br>
> -    DeleteDC(hdc);<br>
> -    return (err >= 0);<br>
>  }<br>
> diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h<br>
> index ece3946..fd6af76 100644<br>
> --- a/vdagent/desktop_layout.h<br>
> +++ b/vdagent/desktop_layout.h<br>
> @@ -60,6 +60,7 @@ private:<br>
>  };<br>
><br>
>  typedef std::vector<DisplayMode*> Displays;<br>
> +class DisplayConfig;<br>
><br>
>  class DesktopLayout {<br>
>  public:<br>
> @@ -73,23 +74,21 @@ public:<br>
>      size_t get_display_count() { return _displays.size();}<br>
>      DWORD get_total_width() { return _total_width;}<br>
>      DWORD get_total_height() { return _total_height;}<br>
> -    void set_position_configurable(bool flag) { _send_monitors_position =<br>
> flag; }<br>
> +    void set_position_configurable(bool flag);<br>
>  private:<br>
>      void clean_displays();<br>
>      void normalize_displays_pos();<br>
>      DisplayMode * get_primary_display();<br>
> -    bool update_monitor_config(LPCTSTR dev_name, DisplayMode* mode);<br>
> +    bool init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMode*<br>
> mode);<br>
>      static bool consistent_displays();<br>
>      static bool is_attached(LPCTSTR dev_name);<br>
>      static bool get_qxl_device_id(WCHAR* device_key, DWORD* device_id);<br>
> -    static bool init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode,<br>
> DisplayMode* mode,<br>
> -                              LONG normal_x, LONG normal_y, bool set_pos);<br>
>  private:<br>
>      mutex_t _mutex;<br>
>      Displays _displays;<br>
>      DWORD _total_width;<br>
>      DWORD _total_height;<br>
> -    bool _send_monitors_position;<br>
> +    DisplayConfig* _display_config;<br>
>  };<br>
><br>
>  #endif<br>
> diff --git a/vdagent/display_configuration.cpp<br>
> b/vdagent/display_configuration.cpp<br>
> index a7aa0d7..5e40d05 100755<br>
> --- a/vdagent/display_configuration.cpp<br>
> +++ b/vdagent/display_configuration.cpp<br>
> @@ -152,6 +152,180 @@ struct DISPLAYCONFIG_PATH_INFO {<br>
>      UINT32 flags;<br>
>  };<br>
><br>
> +struct QXLMonitorEscape {<br>
> +    QXLMonitorEscape(DEVMODE* dev_mode)<br>
> +    {<br>
> +        ZeroMemory(&_head, sizeof(_head));<br>
> +        _head.x = dev_mode->dmPosition.x;<br>
> +        _head.y = dev_mode->dmPosition.y;<br>
> +        _head.width = dev_mode->dmPelsWidth;<br>
> +        _head.height = dev_mode->dmPelsHeight;<br>
> +    }<br>
> +    QXLHead _head;<br>
> +};<br>
> +<br>
> +struct QxlCustomEscapeObj : public QXLEscapeSetCustomDisplay {<br>
> +    QxlCustomEscapeObj(uint32_t bitsPerPel, uint32_t width, uint32_t height)<br>
> +    {<br>
> +        xres = width;<br>
> +        yres = height;<br>
> +        bpp = bitsPerPel;<br>
> +    }<br>
> +    QxlCustomEscapeObj() {};<br>
> +};<br>
> +<br>
> +DisplayConfig* DisplayConfig::create_config()<br>
> +{<br>
> +    DisplayConfig* new_interface;<br>
> +    new_interface = new XPDMInterface();<br>
> +    return new_interface;<br>
> +}<br>
> +<br>
> +DisplayConfig::DisplayConfig()<br>
> +    : _send_monitors_config(false)<br>
> +{}<br>
> +<br>
> +bool XPDMInterface::is_attached(DISPLAY_DEVICE* dev_info)<br>
> +{<br>
> +    return !!(dev_info->StateFlags & DISPLAY_DEVICE_ATTACHED_TO_DESKTOP);<br>
> +}<br>
> +<br>
> +bool XPDMInterface::set_monitor_state(LPCTSTR device_name, DEVMODE*<br>
> dev_mode, MONITOR_STATE state)<br>
> +{<br>
> +    dev_mode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_POSITION;<br>
> +    if (state == MONITOR_ATTACHED) {<br>
> +        return true;<br>
> +    }<br>
> +<br>
> +    LONG status = ChangeDisplaySettingsEx(device_name, dev_mode, NULL,<br>
> CDS_UPDATEREGISTRY, NULL);<br>
> +    return (status == DISP_CHANGE_SUCCESSFUL);<br>
> +}<br>
> +<br>
> +LONG XPDMInterface::update_display_settings()<br>
> +{<br>
> +    return ChangeDisplaySettingsEx(NULL, NULL, NULL, 0, NULL);<br>
> +}<br>
> +<br>
> +bool XPDMInterface::update_dev_mode_position(LPCTSTR device_name,<br>
> +                                             DEVMODE* dev_mode, LONG x, LONG<br>
> y)<br>
> +{<br>
> +    //Convert the position so that the primary is always at (0,0)<br>
> +    dev_mode->dmPosition.x = x;<br>
> +    dev_mode->dmPosition.y = y;<br>
> +    dev_mode->dmFields |= DM_POSITION;<br>
> +    vd_printf("%s: setting %S at (%lu, %lu)", __FUNCTION__, device_name,<br>
> dev_mode->dmPosition.x,<br>
> +        dev_mode->dmPosition.y);<br>
> +<br>
> +    LONG status = ChangeDisplaySettingsEx(device_name, dev_mode, NULL,<br>
> +                                          CDS_UPDATEREGISTRY | CDS_NORESET,<br>
> NULL);<br>
> +    return (status == DISP_CHANGE_SUCCESSFUL);<br>
> +}<br>
> +<br>
> +bool XPDMInterface::custom_display_escape(LPCTSTR device_name, DEVMODE*<br>
> dev_mode)<br><br></div></div>I found this name quite misleading.<br>
What about set_custom_display or set_device_mode ?<br><div><div><br>
> +{<br>
> +    LONG        ret;<br>
> +    NTSTATUS    Status (ERROR_SUCCESS);<br>
> +    HDC         hdc = CreateDC(device_name, NULL, NULL, NULL);<br>
> +<br>
> +    if (!hdc) {<br>
> +        ret = ChangeDisplaySettingsEx(device_name, dev_mode, NULL,<br>
> CDS_UPDATEREGISTRY, NULL);<br>
> +        if (ret == DISP_CHANGE_BADMODE) {<br>
> +            // custom resolution might not be set yet, use known resolution<br>
> +            // FIXME: this causes client temporary resize... a<br>
> +            // solution would involve passing custom resolution before<br>
> +            // driver initialization, perhaps through registry<br>
> +            dev_mode->dmPelsWidth = 640;<br>
> +            dev_mode->dmPelsHeight = 480;<br>
> +            ret = ChangeDisplaySettingsEx(device_name, dev_mode, NULL,<br>
> CDS_UPDATEREGISTRY, NULL);<br>
> +        }<br>
> +<br>
> +        vd_printf("attach %ld", ret);<br>
> +        if (!(hdc = CreateDC(device_name, NULL, NULL, NULL))) {<br>
> +            vd_printf("%s: failed to create DC", __FUNCTION__);<br>
> +            return false;<br>
> +        }<br>
> +    }<br>
> +<br>
> +    QxlCustomEscapeObj custom_escape(dev_mode->dmBitsPerPel,<br>
> +                                            dev_mode->dmPelsWidth,<br>
> dev_mode->dmPelsHeight);<br>
> +<br>
> +    int err = ExtEscape(hdc, QXL_ESCAPE_SET_CUSTOM_DISPLAY,<br>
> +              sizeof(QXLEscapeSetCustomDisplay), (LPCSTR) &custom_escape, 0,<br>
> NULL);<br>
> +    if (err <= 0) {<br>
> +        vd_printf("%s: Can't set custom display, perhaps running with an<br>
> older driver?",<br>
> +            __FUNCTION__);<br>
> +    }<br>
> +<br>
> +    if (!find_best_mode(device_name, dev_mode)) {<br>
> +        Status = E_FAIL;<br>
> +    }<br>
> +<br>
> +    DeleteDC(hdc);<br>
> +    return NT_SUCCESS(Status);<br>
> +}<br>
> +<br>
> +bool XPDMInterface::update_monitor_config(LPCTSTR device_name, DisplayMode*<br>
> mode,<br>
> +                                           DEVMODE* dev_mode)<br><br></div></div>Here you are declaring you can change dev_mode and mode as they are<br>
not constant. Are you expecting to change them?<br>
I would use a const DEVMODE &dev_mode instead.<br></blockquote><div>In the next patch which adds WDDMinterface, mode changes but DEVMODE doesn't.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><br>
> +{<br>
> +    if (!mode || !mode->get_attached()) {<br>
> +        return false;<br>
> +    }<br>
> +<br>
> +    QXLMonitorEscape monitor_config(dev_mode);<br>
> +    HDC hdc(CreateDC(device_name, NULL, NULL, NULL));<br>
> +    int err(0);<br>
> +<br>
> +    if (!hdc || !_send_monitors_config) {<br>
> +        return false;<br>
> +    }<br>
> +<br>
> +    err = ExtEscape(hdc, QXL_ESCAPE_MONITOR_CONFIG, sizeof(QXLHead),<br>
> +                    (LPCSTR) &monitor_config, 0, NULL);<br>
<br>
</span>here it's better to use (LPCSTR) &monitor_config._head to make sure<br>
you are sending head. Just adding a virtual function would break this code.<br><div><div><br>
> +    if (err < 0) {<br>
> +        vd_printf("%s: %S can't update monitor config, may have old, old<br>
> driver",<br>
> +                 __FUNCTION__, device_name);<br>
> +    }<br>
> +    DeleteDC(hdc);<br>
> +    return (err >= 0);<br>
> +}<br>
> +<br>
> +bool XPDMInterface::find_best_mode(LPCTSTR Device, DEVMODE* dev_mode)<br>
> +{<br>
> +    DWORD closest_diff = -1;<br>
> +    DWORD best = -1;<br>
> +<br>
> +    // force refresh mode table<br>
> +    DEVMODE test_dev_mode;<br>
> +    ZeroMemory(&test_dev_mode, sizeof(test_dev_mode));<br>
> +    test_dev_mode.dmSize = sizeof(DEVMODE);<br>
> +    EnumDisplaySettings(Device, 0xffffff, &test_dev_mode);<br>
> +<br>
> +    //Find the closest size which will fit within the monitor<br>
> +    for (DWORD i = 0; EnumDisplaySettings(Device, i, &test_dev_mode); i++) {<br>
> +        if (dev_mode->dmPelsWidth > test_dev_mode.dmPelsWidth ||<br>
> +            dev_mode->dmPelsHeight > test_dev_mode.dmPelsHeight ||<br>
> +            dev_mode->dmBitsPerPel != test_dev_mode.dmBitsPerPel) {<br>
> +            continue;<br>
> +        }<br>
> +        DWORD wdiff = dev_mode->dmPelsWidth - test_dev_mode.dmPelsWidth;<br>
> +        DWORD hdiff = dev_mode->dmPelsHeight - test_dev_mode.dmPelsHeight;<br>
> +        DWORD diff = wdiff * wdiff + hdiff * hdiff;<br>
> +        if (diff < closest_diff) {<br>
> +            closest_diff = diff;<br>
> +            best = i;<br>
> +        }<br>
> +    }<br>
> +    vd_printf("%s: closest_diff at %lu best %lu", __FUNCTION__,<br>
> closest_diff, best);<br>
> +    if (best == (DWORD) -1 || !EnumDisplaySettings(Device, best, dev_mode))<br>
> {<br>
> +        return false;<br>
> +    }<br>
> +<br>
> +    //Change to the best fit<br>
> +    LONG status = ChangeDisplaySettingsEx(Device, dev_mode, NULL,<br>
> +                                          CDS_UPDATEREGISTRY | CDS_NORESET,<br>
> NULL);<br><br></div></div>This change the behavior. Previously there was no such calls from init_dev_mode.<br>
Also the name is not suggesting that you set the mode but just that you find it.<br>
Where these lines are now?<br><span><br>
   dev_mode->dmFields = DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT;<br>
</span>   if (set_pos) {<br><span>       //Convert the position so that the primary is always at (0,0)<br>
</span><span>       dev_mode->dmPosition.x = mode->_pos_x - normal_x;<br>
</span><span>       dev_mode->dmPosition.y = mode->_pos_y - normal_y;<br>
</span>       dev_mode->dmFields |= DM_POSITION;<br>
   }</blockquote><div> This part is now in a separate function "update_dev_mode_position".</div><div><br></div><div>Setting the flags (   dev_mode->dmFields = DM...) is still done in "init_dev_mode" However the flags should be</div><div>"DM_PELSWIDTH | DM_PELSHEIGHT | DM_BITSPERPEL"</div><div>and not</div><div>"DM_PELSWIDTH | DM_PELSHEIGHT | DM_POSITION"</div><div>as they are in this patch.</div><div>Thanks for notifying me of that.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br>
> +    return NT_SUCCESS(status);<br>
> +}<br>
> +<br>
>  CCD::CCD()<br>
>      :_numPathElements(0)<br>
>      ,_numModeElements(0)<br>
> diff --git a/vdagent/display_configuration.h<br>
> b/vdagent/display_configuration.h<br>
> index 94e52e4..05b35f4 100755<br>
> --- a/vdagent/display_configuration.h<br>
> +++ b/vdagent/display_configuration.h<br>
> @@ -89,4 +89,40 @@ private:<br>
>      PATH_STATE _path_state;<br>
>  };<br>
><br>
> +class DisplayMode;<br>
> +<br>
> +//Class provides interface to get/set display configurations<br>
> +class DisplayConfig {<br>
> +public:<br>
> +    static DisplayConfig* create_config();<br>
> +    DisplayConfig();<br>
> +    virtual ~DisplayConfig() {};<br>
> +    virtual bool is_attached(DISPLAY_DEVICE* dev_info) = 0;<br>
> +    virtual bool custom_display_escape(LPCTSTR device, DEVMODE* mode) = 0;<br>
> +    virtual bool update_monitor_config(LPCTSTR device, DisplayMode* mode,<br>
> DEVMODE* dev_mode) = 0;<br>
> +    virtual bool set_monitor_state(LPCTSTR device_name, DEVMODE* dev_mode,<br>
> MONITOR_STATE state) = 0;<br>
> +    virtual LONG update_display_settings() = 0;<br>
> +    virtual bool update_dev_mode_position(LPCTSTR dev_name, DEVMODE*<br>
> dev_mode, LONG x, LONG y) = 0;<br>
> +    void set_monitors_config(bool flag) { _send_monitors_config = flag; }<br>
> +    virtual void update_config_path() {};<br>
> +<br>
> +protected:<br>
> +    bool _send_monitors_config;<br>
> +};<br>
> +<br>
> +//DisplayConfig implementation for guest with XPDM graphics drivers<br>
> +class XPDMInterface : public DisplayConfig {<br>
> +public:<br>
> +    XPDMInterface() :DisplayConfig() {};<br>
> +    bool is_attached(DISPLAY_DEVICE* dev_info);<br>
> +    bool custom_display_escape(LPCTSTR device_name, DEVMODE* dev_mode);<br>
> +    bool update_monitor_config(LPCTSTR device_name, DisplayMode* mode,<br>
> DEVMODE* dev_mode);<br>
> +    bool set_monitor_state(LPCTSTR device_name, DEVMODE* dev_mode,<br>
> MONITOR_STATE state);<br>
> +    LONG update_display_settings();<br>
> +    bool update_dev_mode_position(LPCTSTR device_name, DEVMODE * dev_mode,<br>
> LONG x, LONG y);<br>
> +<br>
> +private:<br>
> +    bool find_best_mode(LPCTSTR Device, DEVMODE* dev_mode);<br>
> +};<br>
> +<br>
>  #endif<br>
> \ No newline at end of file<br></div></div></blockquote></div></div></div></blockquote><div><br></div><div>Frediano</div><div><br></div></div></body></html>