[PATCH 1/2] d3d9/tests/visual: Add Vertexbuffer locking tests

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi Patrick,

I ran your tests on one of my Nvidia machines, see below.

Thanks for looking into this. I had problems with a few games (3DMark2001, Call of Duty Modern Warfare) doing stupid buffer / texture mapping calls that forced my command stream code to wait. Your findings may have some answers to why they work better on Windows.

Please port this test to ddraw, d3d8, d3d10 and d3d11 as well. I know it's a lot of work, but the APIs might have different behavior for compatibility reasons, and I know at least one ddraw game that requires that we ignore redundant D3DLOCK_DISCARD locks (see commit d3520ef4, bug 32485).

Am 2016-07-08 um 17:15 schrieb Patrick Rudolph:
> +        {
> +            "D3DLOCK_NOOVERWRITE",
> +            D3DLOCK_NOOVERWRITE,
> +            FALSE,
> +            TRUE,
> +            TRUE
> +        },
> +        {
It seems that this is not true on my Nvidia card:

> visual.c:21966: Driver string: "nvd3dum.dll"
> visual.c:21967: Description string: "NVIDIA GeForce GT 650M"
> visual.c:21970: Device name string: "\\.\DISPLAY1"
> visual.c:21972: Driver version 10.18.13.5900
> ...
> visual.c:5503: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE. Dynamic = 0.
> visual.c:5503: Test failed: Expected buffer mapped unsyncronized. Flags = D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD. Dynamic = 0.

If AMD GPUs accept D3DLOCK_NOOVERWRITE on non-dynamic buffers we can pick any behavior in Wine because games can't rely on it on Windows. I'll run the test on AMD GPUs later, currently all my machines with AMD GPUs are in a rather bad shape :-\ .

> +    hr = IDirect3DDevice9_SetRenderState(device, D3DRS_FOGENABLE, FALSE);
> +    ok(SUCCEEDED(hr), "Failed to disable fog, hr %#x.\n", hr);
This is redundant.

> +    /* sanity check */
> +    hr = IDirect3DDevice9_CreateVertexBuffer(device, sizeof(quad_strip1), 0, 0, D3DPOOL_DEFAULT, &buffer, NULL);
> +    ok(SUCCEEDED(hr), "Failed to create vertex buffer, hr %#x.\n", hr);
Is there any place where this would fail, but the other tests succeed? My sense is that if something as basic as a vertex buffer draw is broken we'd have mass test failures anyway.

> +        /* Increase triangle draw count for faster GPUs.
> +          Limit size to 0x40000 as bigger values causes the Nvidia driver to crash. */
As mentioned I'll test this on other machines as well, it might make sense to use a pixel shader that calculates square roots in a loop to slow down the GPU if the current setup is unreliable.

> +            if (tests[i].dynamic)
> +                hr = IDirect3DDevice9_CreateVertexBuffer(device, size, D3DUSAGE_DYNAMIC, 0, D3DPOOL_DEFAULT, &buffer, NULL);
> +            else
> +                hr = IDirect3DDevice9_CreateVertexBuffer(device, size, 0, 0, D3DPOOL_DEFAULT, &buffer, NULL);
You could store a DWORD of flags in the tests structure and set it either to 0 or D3DUSAGE_DYNAMIC. That way you can avoid the branch and it makes the tests array easier to read IMO.

> +            hr = IDirect3DVertexBuffer9_Lock(buffer, 0, size, (void **)&quad_strip_array, 0);
> +            ok(SUCCEEDED(hr), "Failed to lock vertex buffer, hr %#x.\n", hr);
> +            if (!SUCCEEDED(hr) || !quad_strip_array)
> +            {
> +                IDirect3DVertexBuffer9_Release(buffer);
> +                continue;
> +            }
You don't really need the if branch. If the call fails the ok() call will already mean that the test fails, and we don't really care about error handling after that. I.e., if you have a test failure it's no problem if you segfault afterwards. (It would matter if you ok is marked todo_wine or has a broken() clause because it fails in some cases on Windows.)

> +            ok(hr == D3D_OK, "Failed to set stream source, hr %#x.\n", hr);
Please consistently use SUCCEEDED(hr). Yeah, I know the existing code isn't perfect regarding this :-( .

> +            hr = IDirect3DVertexBuffer9_Lock(buffer, (quads - 1) * sizeof(quad_strip1), sizeof(quad_strip1), (void **)&quad_strip_array, D3DLOCK_READONLY);
What is the purpose of the readonly lock here? I think it would be interesting to test how D3DLOCK_READONLY behaves regarding synchronization too.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXgk0zAAoJEN0/YqbEcdMwDDMP/3lJ/4Uu4hHSy9KWeLz5wDxH
mq9cmVCon/34BlPOTAzIvYJ1tEoFnNGrXur5iuYgOuCq/4m8TZA7W10pCYEJ7yYE
xS90d2udHsxYBEBoPDXzUff5GFdkRv53AX6IdYc2BmuPN+jA9+j51TxN9qWqYMHK
XNV76BDZC8EZ+mQyTov82CsSsn0K3TvhBa/1njDlbUvuNuvuaDm7qzBMqooZycgv
AejEiI/dwHPyu0dmAJxHjpEJl5FozdOELJwoRtXFCp2LANnWqCAugKkjD9O1kQsb
u0/iKowANpHkP+VS1PkG5dZeIEJjM2lHlopUigwjeBY/SQ/Pdlh2wfJT3HWGKnEo
mnACOI4xHGp5XyNX6iit1W5mjRtv0ZiMNaTawjvptTfMM54XopsuMw+hAK+6itLo
7fgccAGUhZo+Uun2p5ceZSr+2+J7q3TUtWlh633m8yW1jz/82jer2soyDoiFw+ap
uo3+w8iQ2RUTrmD2kJ7dh0ZSaHeqSh522yi3fExbyyyFQiGd5S1CBaUgrHdnt56Z
ciQWEb/hhJyAhLhlbjqJuaYaaozBwXrWCkpX2uRluOoMZFN3H7mi76jtNLys8mOO
eQRQrw1j9vuN/vxEz1yg5NQMvV9PImrc1p56HYn2NXMqVptABjhcnyRpNwnB2ar+
PH/xwUDQRY9CTGRkt4dR
=d9bG
-----END PGP SIGNATURE-----