[Spice-devel] [PATCH win-vdagent v6 3/3] Implementing WDDM interface to support multiple monitors and arbitrary resolution
Sameeh Jubran
sameeh at daynix.com
Mon Aug 15 16:00:24 UTC 2016
On Mon, Aug 15, 2016 at 6:10 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
>
> On Mon, Aug 15, 2016 at 10:52 AM, Frediano Ziglio <fziglio at redhat.com>
> wrote:
>
>> >
>> > 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 implements the WDDM interface while using the CCD API to do
>> > so. Moreover it introduces multiple monitors support and arbitrary
>> > resolution for Windows 10 while preserving backward compatiblity with
>> > previous
>> > versions of Windows.
>> >
>> > 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/display_configuration.cpp | 355
>> > +++++++++++++++++++++++++++++++++++++-
>> > vdagent/display_configuration.h | 47 +++++
>> > 2 files changed, 401 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/vdagent/display_configuration.cpp
>> > b/vdagent/display_configuration.cpp
>> > index 5e40d05..4db093f 100755
>> > --- a/vdagent/display_configuration.cpp
>> > +++ b/vdagent/display_configuration.cpp
>> > @@ -152,6 +152,54 @@ struct DISPLAYCONFIG_PATH_INFO {
>> > UINT32 flags;
>> > };
>> >
>> > +
>> > +enum D3DKMT_ESCAPETYPE {
>> > + D3DKMT_ESCAPE_DRIVERPRIVATE = 0
>> > +};
>> > +
>> > +struct D3DDDI_ESCAPEFLAGS {
>> > + union {
>> > + struct {
>> > + UINT Reserved : 31;
>> > + };
>> > + UINT Value;
>> > + };
>> > +};
>> > +
>> > +struct D3DKMT_ESCAPE {
>> > + D3D_HANDLE hAdapter;
>> > + D3D_HANDLE hDevice;
>> > + D3DKMT_ESCAPETYPE Type;
>> > + D3DDDI_ESCAPEFLAGS Flags;
>> > + VOID* pPrivateDriverData;
>> > + UINT PrivateDriverDataSize;
>> > + D3D_HANDLE hContext;
>> > +};
>> > +
>> > +struct D3DKMT_OPENADAPTERFROMHDC {
>> > + HDC hDc;
>> > + D3D_HANDLE hAdapter;
>> > + LUID AdapterLuid;
>> > + UINT VidPnSourceId;
>> > +};
>> > +
>> > +struct D3DKMT_CLOSEADAPTER {
>> > + D3D_HANDLE hAdapter;
>> > +};
>> > +
>> > +struct D3DKMT_OPENADAPTERFROMDEVICENAME {
>> > + const WCHAR *pDeviceName;
>> > + D3D_HANDLE hAdapter;
>> > + LUID AdapterLuid;
>> > +};
>> > +
>> > +struct D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME {
>> > + WCHAR DeviceName[32];
>> > + D3D_HANDLE hAdapter;
>> > + LUID AdapterLuid;
>> > + UINT VidPnSourceId;
>> > +};
>> > +
>>
>> Where these definition came from ?
>>
> We have already discussed this in previous versions of the patches:
> https://mail.google.com/mail/u/0/#search/in%3Asent+from+
> scratch/15650fbeba9cfd30
> We have written from scratch the ones that we didn't find here:
> https://github.com/notr1ch/DWMCapture/blob/master/DWMCaptureSource.cpp
>
> Just put a comment.
> And the "written from scratch" does not make sense, it's an API and you
> cannot get these
> information from nothing, you need to know the protocol.
>
>
>> > struct QXLMonitorEscape {
>> > QXLMonitorEscape(DEVMODE* dev_mode)
>> > {
>> > @@ -174,10 +222,43 @@ struct QxlCustomEscapeObj : public
>> > QXLEscapeSetCustomDisplay {
>> > QxlCustomEscapeObj() {};
>> > };
>> >
>> > +struct WDDMCustomDisplayEscape {
>> > + WDDMCustomDisplayEscape(DEVMODE* dev_mode)
>> > + {
>> > + _ioctl = QXL_ESCAPE_SET_CUSTOM_DISPLAY;
>> > + _custom.bpp = dev_mode->dmBitsPerPel;
>> > + _custom.xres = dev_mode->dmPelsWidth;
>> > + _custom.yres = dev_mode->dmPelsHeight;
>> > + }
>> > + int _ioctl;
>> > + QXLEscapeSetCustomDisplay _custom;
>> > +};
>> > +
>> > +struct WDDMMonitorConfigEscape {
>> > + WDDMMonitorConfigEscape(DisplayMode* mode)
>> > + {
>> > + _ioctl = QXL_ESCAPE_MONITOR_CONFIG;
>> > + _head.id = _head.surface_id = 0;
>> > + _head.x = mode->get_pos_x();
>> > + _head.y = mode->get_pos_y();
>> > + _head.width = mode->get_width();
>> > + _head.height = mode->get_height();
>> > + }
>> > + int _ioctl;
>> > + QXLHead _head;
>> > +};
>> > +
>> > DisplayConfig* DisplayConfig::create_config()
>> > {
>> > DisplayConfig* new_interface;
>> > - new_interface = new XPDMInterface();
>> > + /* Try to open a WDDM adapter.
>> > + If that failed, assume we have an XPDM driver */
>> > + try {
>> > + new_interface = new WDDMInterface();
>> > + }
>> > + catch (std::exception& e) {
>> > + new_interface = new XPDMInterface();
>> > + }
>> > return new_interface;
>> > }
>> >
>> > @@ -326,6 +407,278 @@ bool XPDMInterface::find_best_mode(LPCTSTR
>> Device,
>> > DEVMODE* dev_mode)
>> > return NT_SUCCESS(status);
>> > }
>> >
>> > +WDDMInterface::WDDMInterface()
>> > + : _pfnOpen_adapter_hdc(NULL)
>> > + , _pfnClose_adapter(NULL)
>> > + , _pfnEscape(NULL)
>> > + , _pfnOpen_adapter_device_name(NULL)
>> > + , _pfnOpen_adapter_gdi_name(NULL)
>> > +{
>> > + LONG error(0);
>> > + //Can we find the D3D calls we need?
>> > + if (!init_d3d_api()) {
>> > + throw std::exception();
>> > + }
>> > +
>> > + //Initialize CCD path stuff
>> > + if (!_ccd.query_display_config()) {
>> > + throw std::exception();
>> > + }
>> > +
>> > + if (!_ccd.set_display_config(error)) {
>> > + throw std::exception();
>> > + }
>> > +}
>> > +
>> > +bool WDDMInterface::is_attached(DISPLAY_DEVICE* dev_info)
>> > +{
>> > + return _ccd.is_attached(dev_info->DeviceName);
>> > +}
>> > +
>> > +bool WDDMInterface::set_monitor_state(LPCTSTR device_name, DEVMODE*
>> > dev_mode, MONITOR_STATE state)
>> > +{
>> > + return _ccd.set_path_state(device_name, state);
>> > +}
>> > +
>> > +bool WDDMInterface::custom_display_escape(LPCTSTR device_name,
>> DEVMODE*
>> > dev_mode)
>> > +{
>> > + DISPLAYCONFIG_MODE_INFO* mode = _ccd.get_active_mode(device_name,
>> > false);
>> > + if (!mode) {
>> > + return false;
>> > + }
>> > +
>> > + //Don't bother if we are already set to the new resolution
>> > + if (mode->sourceMode.width == dev_mode->dmPelsWidth &&
>> > + mode->sourceMode.height == dev_mode->dmPelsHeight) {
>> > + return true;
>> > + }
>> > +
>> > + vd_printf("%s: updating %S resolution\n", __FUNCTION__,
>> device_name);
>> > +
>> > + WDDMCustomDisplayEscape wddm_escape(dev_mode);
>> > + if (escape(device_name, &wddm_escape, sizeof(wddm_escape))) {
>>
>> This is not portable, not even between 32 and 64 bit.
>>
>
> Can you please further explain why?
>
>
> You are sending to the driver a structure that you defined here.
> Usually there is an header that both include which is used to make sure
> you are
> using the same structure layout. Also you have to define the structure in
> order (on a
> system that can have 32/64 bit mixed) avoid bit size problems.
>
> Now you send two structure which are
>
> struct Xxx {
> int ioctl;
> shared_defined_structure ioctl_data;
> };
>
> depending on shared_defined_structure alignment the structure layout
> between 32 and 64
> can potentially change. You are lucky and the 2 structure have the same
> alignment
> on both sizes (QXLHead and QXLEscapeSetCustomDisplay are packed so
> aligned to 4 bytes) and the alignment is compatible with int.
> So now works but it's not hard to break this adding a new escape ioctl
> that does not
> follow these rules. So what's the rule? A packed structure after an int?
> Potentially this will cause the protocol to have unaligned accesses if a
> field should
> be 8-byte aligned (for instance a pointer/handle). Intel architecture have
> just some speed
> penalty but if you define the protocol differently you can avoid this
> penalty.
> If you look at ioctl structures rarely they are packet for this reason.
>
The size of int is defined as 4 bytes on windows, however I think it would
be better to change it to uint32_t. There seem
to be no alignment issue inside the current structures, all of the members
are "uint32_t". Moving The structure to
"spice-protocol\spice\qxl-windows.h" would be a better approach?
>
>
>> > + return _ccd.update_mode_size(device_name, dev_mode);
>> > + }
>> > +
>> > + vd_printf("%s: (%dx%d)", __FUNCTION__, mode->sourceMode.width,
>> > mode->sourceMode.height);
>> > + return false;
>> > +}
>> > +
>> > +bool WDDMInterface::update_monitor_config(LPCTSTR device_name,
>> DisplayMode*
>> > display_mode,
>> > + DEVMODE* dev_mode)
>> > +{
>> > + if (!display_mode || !display_mode->get_attached()) {
>> > + return false;
>> > + }
>> > + DISPLAYCONFIG_MODE_INFO* mode = _ccd.get_active_mode(device_name,
>> > false);
>> > + if (!mode || !_send_monitors_config)
>> > + return false;
>> > +
>> > + WDDMMonitorConfigEscape wddm_escape(display_mode);
>> > + if (escape(device_name, &wddm_escape, sizeof(wddm_escape))) {
>>
>> Same as above. It's not clear who is receiving these escape commands...
>> the driver?
>>
> Yes the driver is receiving those escape commands.
>
>
>> > + //Update the path position
>> > + return _ccd.update_mode_position(device_name, dev_mode);
>> > + }
>> > +
>> > + vd_printf("%s: %S failed", __FUNCTION__, device_name);
>> > + return false;
>> > +
>> > +}
>> > +
>> > +LONG WDDMInterface::update_display_settings()
>> > +{
>> > + LONG error(0);
>> > + //If we removed the primary monitor since the last call, we need to
>> > + //reorder the other monitors, making the leftmost one the primary
>> > + _ccd.verify_primary_position();
>> > + _ccd.set_display_config(error);
>> > + return error;
>> > +}
>> > +
>> > +void WDDMInterface::update_config_path()
>> > +{
>> > + _ccd.query_display_config();
>> > +}
>> > +
>> > +bool WDDMInterface::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)
>>
>> This comment does not make any sense here.
>>
>> > + dev_mode->dmPosition.x = x;
>> > + dev_mode->dmPosition.y = y;
>> > + return _ccd.update_mode_position(device_name, dev_mode);
>> > +}
>> > +
>> > +bool WDDMInterface::init_d3d_api()
>> > +{
>> > + HMODULE hModule = LoadLibrary(L"gdi32.dll");
>> > +
>> > + //Look for the gdi32 functions we need to perform driver escapes
>> > + if (!hModule) {
>> > + vd_printf("%s something wildly wrong as we can't open
>> gdi32.dll",
>> > __FUNCTION__);
>> > + return false;
>> > + }
>> > +
>> > + do {
>> > + _pfnClose_adapter = (PFND3DKMT_CLOSEADAPTER)
>> > + GetProcAddress(hModule, "D3DKMTCloseAdapter");
>> > + if (!_pfnClose_adapter) {
>> > + break;
>> > + }
>> > +
>> > + _pfnEscape = (PFND3DKMT_ESCAPE) GetProcAddress(hModule,
>> > "D3DKMTEscape");
>> > + if (!_pfnEscape) {
>> > + break;
>> > + }
>> > +
>> > + _pfnOpen_adapter_hdc = (PFND3DKMT_OPENADAPTERFROMHDC)
>> > + GetProcAddress(hModule, "D3DKMTOpenAdapterFromHdc");
>> > + if (!_pfnOpen_adapter_hdc) {
>> > + break;
>> > + }
>> > +
>> > + _pfnOpen_adapter_device_name = (PFND3DKMT_
>> OPENADAPTERFROMDEVICENAME)
>> > + GetProcAddress(hModule, "D3DKMTOpenAdapterFromDeviceNam
>> e");
>> > + if (!_pfnOpen_adapter_device_name) {
>> > + break;
>> > + }
>> > +
>> > + _pfnOpen_adapter_gdi_name =
>> > (PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME)
>> > + GetProcAddress(hModule, "D3DKMTOpenAdapterFromGdiDispla
>> yName");
>> > + if (!_pfnOpen_adapter_gdi_name) {
>> > + break;
>> > + }
>> > +
>> > + }
>> > + while(0);
>> > +
>> > + FreeLibrary(hModule);
>> > +
>>
>> If the library was loaded the program is going to crash as the function
>> will disappear from memory.
>>
>
> I'll use GetModuleHandle instead.
>
>
>> > + //Did we get them ?
>> > + if (!_pfnClose_adapter || !_pfnOpen_adapter_hdc || !_pfnEscape) {
>> > + return false;
>> > + }
>> > + return true;
>> > +}
>> > +
>> > +D3D_HANDLE WDDMInterface::adapter_handle(LPCTSTR device_name)
>> > +{
>> > + D3D_HANDLE hAdapter(0);
>> > +
>> > + //For some reason, unknown to me, this call will occasionally fail.
>>
>> Who is the "me" here? Would be better to make it explicit.
>>
>> > + if ((hAdapter = handle_from_DC(device_name))) {
>> > + return hAdapter;
>> > + }
>> > + //So try other available methods.
>> > + if (_pfnOpen_adapter_device_name && (hAdapter =
>> > handle_from_device_name(device_name))) {
>> > + return hAdapter;
>> > + }
>> > + //One last chance to open this guy
>> > + if (_pfnOpen_adapter_gdi_name) {
>> > + hAdapter = handle_from_GDI_name(device_name);
>> > + }
>> > +
>> > + if (!hAdapter) {
>> > + vd_printf("%s: failed to open adapter %S", __FUNCTION__,
>> > device_name);
>> > + }
>> > +
>> > + return hAdapter;
>> > +}
>> > +
>> > +D3D_HANDLE WDDMInterface::handle_from_DC(LPCTSTR adapter_name)
>> > +{
>> > + NTSTATUS status;
>> > + D3DKMT_OPENADAPTERFROMHDC open_data;
>> > + HDC hDc(CreateDC(adapter_name, NULL, NULL, NULL));
>> > +
>> > + if (!hDc) {
>> > + vd_printf("%s: %S CreateDC failed with %lu", __FUNCTION__,
>> > adapter_name, GetLastError());
>> > + return 0;
>> > + }
>> > +
>> > + ZeroMemory(&open_data, sizeof(D3DKMT_OPENADAPTERFROMHDC));
>> > + open_data.hDc = hDc;
>> > +
>> > + if (!NT_SUCCESS(status = _pfnOpen_adapter_hdc(&open_data))) {
>> > + vd_printf("%s: %S open adapter from hdc failed with %lu",
>> > __FUNCTION__, adapter_name,
>> > + status);
>> > + open_data.hAdapter = 0;
>> > + }
>> > +
>> > + DeleteDC(hDc);
>> > + return open_data.hAdapter;
>> > +}
>> > +
>> > +D3D_HANDLE WDDMInterface::handle_from_device_name(LPCTSTR
>> adapter_name)
>> > +{
>> > + D3DKMT_OPENADAPTERFROMDEVICENAME display_name_data;
>> > + NTSTATUS status;
>> > +
>> > + ZeroMemory(&display_name_data, sizeof(display_name_data));
>> > + display_name_data.pDeviceName = adapter_name;
>> > +
>> > + if (NT_SUCCESS(status =
>> > _pfnOpen_adapter_device_name(&display_name_data))) {
>> > + return display_name_data.hAdapter;
>> > + }
>> > +
>> > + vd_printf("%s %S failed with 0x%lx", __FUNCTION__, adapter_name,
>> > status);
>> > + return 0;
>> > +}
>> > +
>> > +D3D_HANDLE WDDMInterface::handle_from_GDI_name(LPCTSTR adapter_name)
>> > +{
>> > + D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME gdi_display_name;
>> > + NTSTATUS status;
>> > +
>> > + ZeroMemory(&gdi_display_name, sizeof(gdi_display_name));
>> > + memcpy((void *) gdi_display_name.DeviceName, adapter_name,
>> > sizeof(TCHAR)* CCHDEVICENAME);
>> > +
>>
>> This assume adapter_name is into a buffer of CCHDEVICENAME characters so
>> can in theory overflow. As the destination buffer is already 0 filled
>> would be better a strncpy style function. The void * conversion is useless
>> and misleading.
>
>
>> > + if (NT_SUCCESS(status = _pfnOpen_adapter_gdi_name(&gdi_display_name)))
>> {
>> > + return gdi_display_name.hAdapter;
>> > + }
>> > +
>> > + vd_printf("%s: %S aurrrgghh nothing works..error is 0x%lx",
>> > __FUNCTION__, adapter_name,
>> > + status);
>> > + return 0;
>> > +}
>> > +
>> > +void WDDMInterface::close_adapter(D3D_HANDLE handle)
>> > +{
>> > + D3DKMT_CLOSEADAPTER closeData;
>> > + if (handle) {
>> > + closeData.hAdapter = handle;
>> > + _pfnClose_adapter(&closeData);
>> > + }
>> > +}
>> > +
>> > +bool WDDMInterface::escape(LPCTSTR device_name, void* data, UINT
>> size_data)
>> > +{
>> > + D3DKMT_ESCAPE escapeData;
>> > + NTSTATUS status;
>> > + D3D_HANDLE hAdapter(0);
>> > +
>> > + if (!(hAdapter = adapter_handle(device_name)))
>> > + return false;
>> > +
>> > + escapeData.hAdapter = hAdapter;
>> > + escapeData.hDevice = 0;
>> > + escapeData.hContext = 0;
>> > + escapeData.Type = D3DKMT_ESCAPE_DRIVERPRIVATE;
>> > + escapeData.Flags.Value = 0;
>> > + escapeData.pPrivateDriverData = data;
>> > + escapeData.PrivateDriverDataSize = size_data;
>> > +
>> > + status = _pfnEscape(&escapeData);
>> > +
>> > + if (!NT_SUCCESS(status)) {
>> > + vd_printf("%s: this should never happen. Status is 0x%lx",
>> > __FUNCTION__, status);
>> > + }
>> > +
>> > + //Close the handle to this device
>> > + close_adapter(hAdapter);
>> > + return NT_SUCCESS(status);
>> > +}
>> > +
>> > CCD::CCD()
>> > :_numPathElements(0)
>> > ,_numModeElements(0)
>> > diff --git a/vdagent/display_configuration.h
>> > b/vdagent/display_configuration.h
>> > index 05b35f4..e5ee90d 100755
>> > --- a/vdagent/display_configuration.h
>> > +++ b/vdagent/display_configuration.h
>> > @@ -125,4 +125,51 @@ private:
>> > bool find_best_mode(LPCTSTR Device, DEVMODE* dev_mode);
>> > };
>> >
>> > +//DisplayConfig implementation for guest with WDDM graphics drivers
>> > +typedef UINT D3D_HANDLE;
>> > +
>> > +struct D3DKMT_ESCAPE;
>> > +struct D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME;
>> > +struct D3DKMT_OPENADAPTERFROMDEVICENAME;
>> > +struct D3DKMT_CLOSEADAPTER;
>> > +struct D3DKMT_OPENADAPTERFROMHDC;
>> > +
>> > +typedef NTSTATUS(APIENTRY* PFND3DKMT_ESCAPE)(CONST D3DKMT_ESCAPE*);
>> > +typedef NTSTATUS(APIENTRY*
>> > PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME)(D3DKMT_
>> OPENADAPTERFROMGDIDISPLAYNAME*);
>> > +typedef NTSTATUS(APIENTRY*
>> > PFND3DKMT_OPENADAPTERFROMDEVICENAME)(D3DKMT_
>> OPENADAPTERFROMDEVICENAME*);
>> > +typedef NTSTATUS(APIENTRY* PFND3DKMT_CLOSEADAPTER)(
>> D3DKMT_CLOSEADAPTER*);
>> > +typedef NTSTATUS(APIENTRY*
>> > PFND3DKMT_OPENADAPTERFROMHDC)(D3DKMT_OPENADAPTERFROMHDC*);
>> > +
>> > +class WDDMInterface : public DisplayConfig {
>> > +public:
>> > + WDDMInterface();
>> > + bool is_attached(DISPLAY_DEVICE* dev_info);
>> > + bool set_monitor_state(LPCTSTR device_name, DEVMODE* dev_mode,
>> > MONITOR_STATE state);
>> > + LONG update_display_settings();
>> > + bool custom_display_escape(LPCTSTR device_name, DEVMODE* dev_mode);
>> > + 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();
>> > +
>> > +private:
>> > + bool init_d3d_api();
>> > + D3D_HANDLE adapter_handle(LPCTSTR device_name);
>> > + D3D_HANDLE handle_from_DC(LPCTSTR adapter_name);
>> > + D3D_HANDLE handle_from_device_name(LPCTSTR adapter_name);
>> > + D3D_HANDLE handle_from_GDI_name(LPCTSTR adapter_name);
>> > +
>> > + void close_adapter(D3D_HANDLE handle);
>> > + bool escape(LPCTSTR device_name, void* data, UINT sizeData);
>> > +
>> > + //GDI Function pointers
>> > + PFND3DKMT_OPENADAPTERFROMHDC _pfnOpen_adapter_hdc;
>> > + PFND3DKMT_CLOSEADAPTER _pfnClose_adapter;
>> > + PFND3DKMT_ESCAPE _pfnEscape;
>> > + PFND3DKMT_OPENADAPTERFROMDEVICENAME _pfnOpen_adapter_device_name;
>> > + PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME _pfnOpen_adapter_gdi_name;
>> > +
>> > + //object handles the CCD API
>> > + CCD _ccd;
>> > +};
>> > +
>> > #endif
>> > \ No newline at end of file
>>
>> The update_ prefix is confusing. I would personally use
>> set/get/query/read/write.
>> The reason is that is not clear if they are updating a class internal
>> state (so
>> reading from the device) or updating the device state or a mix of the two.
>>
>
> Frediano
>
>
--
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Junior Software Engineer @ Daynix <http://www.daynix.com>.*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160815/b1cd3418/attachment-0001.html>
More information about the Spice-devel
mailing list