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