[PATCH 6/6] ddraw: Avoid passing invalid device type to wined3d.
Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- dlls/ddraw/main.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/dlls/ddraw/main.c b/dlls/ddraw/main.c index 0faa98234d55..ac4d3f786c79 100644 --- a/dlls/ddraw/main.c +++ b/dlls/ddraw/main.c @@ -256,62 +256,52 @@ HRESULT WINAPI GetSurfaceFromDC(HDC dc, IDirectDrawSurface4 **surface, HDC *devi * E_OUTOFMEMORY if some allocation failed * ***********************************************************************/ -static HRESULT -DDRAW_Create(const GUID *guid, - void **DD, - IUnknown *UnkOuter, - REFIID iid) +static HRESULT DDRAW_Create(const GUID *guid, void **out, IUnknown *outer_unknown, REFIID iid) { enum wined3d_device_type device_type; struct ddraw *ddraw; - HRESULT hr; DWORD flags = 0; + HRESULT hr; TRACE("driver_guid %s, ddraw %p, outer_unknown %p, interface_iid %s.\n", - debugstr_guid(guid), DD, UnkOuter, debugstr_guid(iid)); + debugstr_guid(guid), out, outer_unknown, debugstr_guid(iid)); - *DD = NULL; + *out = NULL; if (guid == (GUID *) DDCREATE_EMULATIONONLY) { - /* Use the reference device id. This doesn't actually change anything, - * WineD3D always uses OpenGL for D3D rendering. One could make it request - * indirect rendering - */ device_type = WINED3D_DEVICE_TYPE_REF; } - else if(guid == (GUID *) DDCREATE_HARDWAREONLY) + else if (guid == (GUID *) DDCREATE_HARDWAREONLY) { device_type = WINED3D_DEVICE_TYPE_HAL; } else { - device_type = 0; + device_type = WINED3D_DEVICE_TYPE_HAL; } /* DDraw doesn't support aggregation, according to msdn */ - if (UnkOuter != NULL) + if (outer_unknown != NULL) return CLASS_E_NOAGGREGATION; if (!IsEqualGUID(iid, &IID_IDirectDraw7)) flags = WINED3D_LEGACY_FFP_LIGHTING; - /* DirectDraw creation comes here */ if (!(ddraw = heap_alloc_zero(sizeof(*ddraw)))) { - ERR("Out of memory when creating DirectDraw\n"); + ERR("Out of memory when creating DirectDraw.\n"); return E_OUTOFMEMORY; } - hr = ddraw_init(ddraw, flags, device_type); - if (FAILED(hr)) + if (FAILED(hr = ddraw_init(ddraw, flags, device_type))) { WARN("Failed to initialize ddraw object, hr %#x.\n", hr); heap_free(ddraw); return hr; } - hr = IDirectDraw7_QueryInterface(&ddraw->IDirectDraw7_iface, iid, DD); + hr = IDirectDraw7_QueryInterface(&ddraw->IDirectDraw7_iface, iid, out); IDirectDraw7_Release(&ddraw->IDirectDraw7_iface); if (SUCCEEDED(hr)) list_add_head(&global_ddraw_list, &ddraw->ddraw_list_entry); -- 2.19.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=50559 Your paranoid android. === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
On 4/5/19 4:04 AM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=50559
Your paranoid android.
=== debian9 (build log) ===
X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
This has finally annoyed me enough to look into it, and here is what I came up with: From reading the source to the X server [1] and doing some testing with a local setup identical enough to the TestBot, it appears that we receive a BadValue in the "x" parameter passed in to XRRSetCrtcConfig(). This function (sometimes) validates the passed-in offsets against the screen size, and this will of course fail if we try to increase the display resolution, as the d3d tests invariably do. A more recent version of the qxl driver, or possibly the X server, or both (I rather lazily just made a new VM with Arch, which of course has more recent packages than Debian Stable) doesn't suffer from this failure. However, it doesn't quite change the display resolution either: it changes the device mode, but not the screen size. From reading the XRandR protocol spec [2], it seems like the problem is on our end. From RRSetCrtcConfig(): "'x' and 'y' contain the desired location within the screen for this monitor's content. 'x' and 'y' must be within the screen size, else a Value error results." and "The entire area of the CRTC must fit with the screen size, else a Match error results. As an example, rotating the screen so that a single CRTC fills the entire screen before and after may necessitate disabling the CRTC, resizing the screen, then re-enabling the CRTC at the new configuration to avoid an invalid intermediate configuration." It seems the X.Org server implementation doesn't quite respect this: we would expect a BadMatch error here. But the point seems to remain: the spec states clearly that the screen must be large enough to contain the new CRTC size. So this suggests that the problem is not really with qxl, but rather on our end. It seems that qxl is not the only driver affected by this: bug 33290 [3] describes a similar problem with NVIDIA binary drivers (those, at least, that actually report multiple modes, and so don't fall back to XRandR 1.0 in Wine regardless). This was diagnosed there too, in comment 15, and a patch was submitted, which received the comment [4] "Updating the screen size isn't necessarily wrong, although avoiding panning is the wrong reason." I can only imagine, then, that "because the spec says so" is the right reason, but I may be misunderstanding or missing any number of things. If there's any obvious way in which I've misanalyzed the problem, or am misreading the specification, please let me know, otherwise I think I'll try to work on an improved version of Gabriel Corona's patch that takes multi-head into account. ἔρρωσθε, Zeb [1] https://cgit.freedesktop.org/xorg/xserver/tree/randr/rrcrtc.c#n1366 [2] https://www.x.org/releases/X11R7.7/doc/randrproto/randrproto.txt [3] https://bugs.winehq.org/show_bug.cgi?id=33290 [4] https://www.winehq.org/pipermail/wine-devel/2016-July/113932.html
We already talked about this on IRC, but for the benefit of the mailing list: On Mon, 8 Apr 2019 at 07:05, Zebediah Figura <z.figura12(a)gmail.com> wrote:
It seems that qxl is not the only driver affected by this: bug 33290 [3] describes a similar problem with NVIDIA binary drivers (those, at least, that actually report multiple modes, and so don't fall back to XRandR 1.0 in Wine regardless). This was diagnosed there too, in comment 15, and a patch was submitted, which received the comment [4] "Updating the screen size isn't necessarily wrong, although avoiding panning is the wrong reason." I can only imagine, then, that "because the spec says so" is the right reason, but I may be misunderstanding or missing any number of things.
The typical starting configuration is DFPs running at their native resolutions, and applications, typically games, switching to a lower resolution. In that scenario the screen size is always large enough to contain the CRTCs. In fact, changing the screen size may cause desktop environments to rearrange panels and icons. Depending on the specifics they may of course already do that when changing CRTC sizes, and rearranging things may or may not be considered desirable by the user. We would of course need to set the screen size when changing to a configuration that would be larger than the current screen size. Another potential reason for us to update the screen size is making sure SM_CXVIRTUALSCREEN and things derived from that match what they would be on Windows. That's something that would need some tests, of course. As for panning, that's something that can be configured in xorg.conf, or with e.g. xrandr. The initial state for panning may be different between drivers. Henri
participants (5)
-
Henri Verbeet -
Henri Verbeet -
Józef Kucia -
Marvin -
Zebediah Figura