On Mon, Aug 15, 2016 at 6:10 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
On Mon, Aug 15, 2016 at 10:52 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:Where these definition came from ?>
> 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@xxxxxxxxxx>
>
> Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
> Signed-off-by: Sameeh Jubran <sameeh@xxxxxxxxxx>
> ---
> 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;
> +};
> +We have already discussed this in previous versions of the patches:We have written from scratch the ones that we didn't find here:Just put a comment.And the "written from scratch" does not make sense, it's an API and you cannot get theseinformation from nothing, you need to know the protocol.This is not portable, not even between 32 and 64 bit.
> 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))) {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 areusing the same structure layout. Also you have to define the structure in order (on asystem that can have 32/64 bit mixed) avoid bit size problems.Now you send two structure which arestruct Xxx {int ioctl;shared_defined_structure ioctl_data;};depending on shared_defined_structure alignment the structure layout between 32 and 64can potentially change. You are lucky and the 2 structure have the same alignmenton both sizes (QXLHead and QXLEscapeSetCustomDisplay are packed soaligned 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 notfollow 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 shouldbe 8-byte aligned (for instance a pointer/handle). Intel architecture have just some speedpenalty 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); Same as above. It's not clear who is receiving these escape commands...
> + }
> +
> + 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))) {
the driver?Yes the driver is receiving those escape commands.This comment does not make any sense here.
> + //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)If the library was loaded the program is going to crash as the function
> + 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);
> +
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) Who is the "me" here? Would be better to make it explicit.
> +{
> + D3D_HANDLE hAdapter(0);
> +
> + //For some reason, unknown to me, this call will occasionally fail.
This assume adapter_name is into a buffer of CCHDEVICENAME characters so
> + 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);
> +
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.The update_ prefix is confusing. I would personally use set/get/query/read/write.
> + 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 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
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel