November 2021 - Wine-Devel - list.winehq.org

[PATCH 2/3] d3d9/tests: Test triangle filling convention (v2).

by Stefan Dösinger

Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- Version 2: Remove MSDN link. I kept the name "nudge" in the test geometry here on purpose. It feels better for a test that deliberately tests a slightly-not-on-the-border geometry. There is a certain overlap between this test and test_[fractional_]viewport. This test explicitly tests which fragment lights up and how diagonals that are hit by two triangles are handled, whereas test_viewport checks for a mismatch between fragment location and position of that fragment inside the triangle. Both tests are worth having IMO. I'll port it to ddraw-d3d11 once the merits of the test and geometry nudge have been agreed upon. --- dlls/d3d9/tests/visual.c | 485 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 485 insertions(+) diff --git a/dlls/d3d9/tests/visual.c b/dlls/d3d9/tests/visual.c index 3820252fdef..eb31f877acd 100644 --- a/dlls/d3d9/tests/visual.c +++ b/dlls/d3d9/tests/visual.c @@ -27019,6 +27019,490 @@ static void test_dynamic_map_synchronization(void) DestroyWindow(window); } +static void test_filling_convention(void) +{ + static const DWORD colour_bottom = 0x00ffff00; + static const DWORD colour_clear = 0x000000ff; + static const DWORD colour_right = 0x00000000; + static const DWORD colour_left = 0x00ff0000; + static const DWORD colour_top = 0x0000ff00; + IDirect3DSurface9 *rt, *backbuffer, *cur; + IDirect3DVertexShader9 *shader = NULL; + struct surface_readback rb; + IDirect3DDevice9 *device; + unsigned int i, j, x, y; + DWORD colour, expected; + IDirect3D9 *d3d; + ULONG refcount; + D3DCAPS9 caps; + HWND window; + HRESULT hr; + BOOL todo; + + static const unsigned int vp_size = 8; + const D3DVIEWPORT9 vp = { 0, 0, vp_size, vp_size, 0.0, 1.0 }; + static const DWORD vs_code[] = + { + 0xfffe0101, /* vs_1_1 */ + 0x0000001f, 0x80000000, 0x900f0000, /* dcl_position v0 */ + 0x0000001f, 0x8000000a, 0x900f0001, /* dcl_color0 v1 */ + 0x00000001, 0xc00f0000, 0x90e40000, /* mov oPos, v0 */ + 0x00000001, 0xd00f0000, 0x90e40001, /* mov oD0, v1 */ + 0x0000ffff /* end */ + }; + + /* This test data follows the examples in MSDN's + * "Rasterization Rules (Direct3D 9)" article. */ + static const float eps = 1.0f / 512.0f; + const struct + { + struct vec3 position; + DWORD diffuse; + } + center_tris[] = + { + /* left */ + {{-2.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_left}, + {{-2.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_left}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_top}, + {{-2.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_top}, + {{-0.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{-0.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_right}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_right}, + {{-0.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_bottom}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_bottom}, + {{-0.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_bottom}, + + }, + edge_tris[] = + { + /* left */ + {{-2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_right_tris[] = + { + /* left */ + {{eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_left_tris[] = + { + {{-eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{-eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{-eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{-eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{-eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{-eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_top_tris[] = + { + /* left */ + {{-2.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_bottom_tris[] = + { + /* left */ + {{-2.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + }; + + const struct + { + struct vec4 position; + DWORD diffuse; + } + center_tris_t[] = + { + /* left */ + {{ 1.5f, 1.5f, 0.0f, 1.0f}, colour_left}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_left}, + {{ 1.5f, 5.5f, 0.0f, 1.0f}, colour_left}, + + /* top */ + {{ 1.5f, 1.5f, 0.0f, 1.0f}, colour_top}, + {{ 3.5f, 1.5f, 0.0f, 1.0f}, colour_top}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_top}, + + /* right */ + {{ 3.5f, 1.5f, 0.0f, 1.0f}, colour_right}, + {{ 3.5f, 5.5f, 0.0f, 1.0f}, colour_right}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_right}, + + /* bottom */ + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_bottom}, + {{ 3.5f, 5.5f, 0.0f, 1.0f}, colour_bottom}, + {{ 1.5f, 5.5f, 0.0f, 1.0f}, colour_bottom}, + }, + edge_tris_t[] = + { + /* left */ + {{ 2.0f, 1.0f, 0.0f, 1.0f}, colour_left}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_left}, + {{ 2.0f, 5.0f, 0.0f, 1.0f}, colour_left}, + + /* top */ + {{ 2.0f, 1.0f, 0.0f, 1.0f}, colour_top}, + {{ 4.0f, 1.0f, 0.0f, 1.0f}, colour_top}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_top}, + + /* right */ + {{ 4.0f, 1.0f, 0.0f, 1.0f}, colour_right}, + {{ 4.0f, 5.0f, 0.0f, 1.0f}, colour_right}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_right}, + + /* bottom */ + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_bottom}, + {{ 4.0f, 5.0f, 0.0f, 1.0f}, colour_bottom}, + {{ 2.0f, 5.0f, 0.0f, 1.0f}, colour_bottom}, + }; + + const struct + { + const void *geometry; + size_t stride; + DWORD fvf; + const char *expected[8]; + } + tests[] = + { + { + center_tris, + sizeof(center_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " ", + " TT ", + " LR ", + " LR ", + " BB ", + " ", + " " + } + }, + { + edge_tris, + sizeof(edge_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_right_tris, + sizeof(nudge_right_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " TR ", + " LR ", + " BR ", + " ", + " ", + " " + } + }, + { + nudge_left_tris, + sizeof(nudge_left_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_top_tris, + sizeof(nudge_top_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " LT ", + " LT ", + " LB ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_bottom_tris, + sizeof(nudge_bottom_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " ", + " LT ", + " Lt ", + " LB ", + " lB ", + " ", + " " + } + }, + { + center_tris_t, + sizeof(center_tris_t[0]), + D3DFVF_XYZRHW | D3DFVF_DIFFUSE, + { + " ", + " ", + " TT ", + " LR ", + " LR ", + " BB ", + " ", + " " + } + }, + { + edge_tris_t, + sizeof(edge_tris_t[0]), + D3DFVF_XYZRHW | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + }; + + window = create_window(); + d3d = Direct3DCreate9(D3D_SDK_VERSION); + ok(!!d3d, "Failed to create a D3D object.\n"); + + if (!(device = create_device(d3d, window, window, TRUE))) + { + skip("Failed to create a 3D device.\n"); + IDirect3D9_Release(d3d); + DestroyWindow(window); + return; + } + + hr = IDirect3DDevice9_CreateRenderTarget(device, vp_size, vp_size, + D3DFMT_A8R8G8B8, D3DMULTISAMPLE_NONE, 0, FALSE, &rt, NULL); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_GetBackBuffer(device, 0, 0, D3DBACKBUFFER_TYPE_MONO, &backbuffer); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_GetDeviceCaps(device, &caps); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + if (caps.VertexShaderVersion >= D3DVS_VERSION(1, 1)) + { + hr = IDirect3DDevice9_CreateVertexShader(device, vs_code, &shader); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + else + skip("Skipping vertex shader codepath in filling convention test.\n"); + + hr = IDirect3DDevice9_SetRenderState(device, D3DRS_LIGHTING, FALSE); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ZENABLE, D3DZB_FALSE); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + hr = IDirect3DDevice9_SetFVF(device, tests[i].fvf); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + /* Run tests with shader and fixed function vertex processing if shaders are + * supported. There's no point in running the XYZRHW tests with a VS though. */ + if (shader && ((tests[i].fvf & D3DFVF_POSITION_MASK) == D3DFVF_XYZ)) + j = 0; + else + j = 2; + + for (; j < 4; ++j) + { + cur = (j & 1) ? rt : backbuffer; + + hr = IDirect3DDevice9_SetVertexShader(device, (j & 2) ? NULL : shader); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_SetRenderTarget(device, 0, cur); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_Clear(device, 0, NULL, D3DCLEAR_TARGET, colour_clear, 0.0f, 0); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_SetViewport(device, &vp); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_BeginScene(device); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_DrawPrimitiveUP(device, D3DPT_TRIANGLELIST, 4, tests[i].geometry, tests[i].stride); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_EndScene(device); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + get_rt_readback(cur, &rb); + for (y = 0; y < 8; y++) + { + for (x = 0; x < 8; x++) + { + todo = FALSE; + switch (tests[i].expected[y][x]) + { + case 'l': todo = TRUE; + case 'L': + expected = colour_left; + break; + case 't': todo = TRUE; + case 'T': + expected = colour_top; + break; + case 'r': todo = TRUE; + case 'R': + expected = colour_right; + break; + case 'b': todo = TRUE; + case 'B': + expected = colour_bottom; + break; + case ' ': + expected = colour_clear; + break; + default: + ok(0, "Unexpected entry in expected test char\n"); + expected = 0xdeadbeef; + } + colour = get_readback_color(&rb, x, y); + /* The nudge-to-bottom test fails on cards that give us a bottom-left + * filling convention. The cause isn't the bottom part of the filling + * convention, but because wined3d will nudge geometry to the left to + * keep diagonals (the 'R' in test case 'edge_tris') intact. */ + todo_wine_if(todo && !color_match(colour, expected, 1)) + ok(color_match(colour, expected, 1), "Got unexpected colour %08x, %ux%u, case %u, j %u.\n", + colour, x, y, i, j); + } + } + release_surface_readback(&rb); + + /* For debugging */ + if (cur != backbuffer) + { + hr = IDirect3DDevice9_StretchRect(device, rt, NULL, backbuffer, NULL, D3DTEXF_POINT); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + hr = IDirect3DDevice9_Present(device, NULL, NULL, NULL, NULL); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + } + + if (shader) + IDirect3DVertexShader9_Release(shader); + IDirect3DSurface9_Release(backbuffer); + IDirect3DSurface9_Release(rt); + refcount = IDirect3DDevice9_Release(device); + ok(!refcount, "Device has %u references left.\n", refcount); + IDirect3D9_Release(d3d); + DestroyWindow(window); +} + START_TEST(visual) { D3DADAPTER_IDENTIFIER9 identifier; @@ -27168,4 +27652,5 @@ START_TEST(visual) test_alpha_to_coverage(); test_sample_mask(); test_dynamic_map_synchronization(); + test_filling_convention(); } -- 2.32.0

Nov. 3, 2021

[PATCH 1/3] wined3d: Test if an offset is needed to get the correct filling convention (v3).

by Stefan Dösinger

This fixes stray lines in GameFace GUIs, e.g. in World of Tanks. Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- Version 3: *) Fix "convetion" for real this time *) nudge -> offset *) good, bad -> upper, lower Version 2: *) Use a binary search for the fixup value. *) Change how an unexpected test result is detected. *) Add a comment about this to the Vulkan backend. *) Small wording changes. Re Vulkan: If you can refer me to the place in the spec that mandates a top-left filling convention I am more than happy to add it in the comment. I really don't want to add this detection mess to the Vulkan backend. I am also more than happy to just drop this nudge thingy unconditionally. I am not aware of a card + game combination that is fixed by this. Spore doesn't need it on my gf9600. I didn't bother to download Everquest, I expect it to have changed a lot in the past 10 years. I don't have access to a dx9 level card right now, so I can't entirely rule out that Spore needs this nudge on those cards. The ddraw tests started succeeding in the todo block for a few of the tested pixels. I looked at the output results and I don't think it happened because the Z values are more precise now. I think a few bits flipped (in the random readback we get) and now a few of those tests were inside the expected diff. --- dlls/d3d11/tests/d3d11.c | 1 - dlls/ddraw/tests/ddraw4.c | 6 +- dlls/ddraw/tests/ddraw7.c | 6 +- dlls/wined3d/adapter_gl.c | 50 ++++++++++++++ dlls/wined3d/adapter_vk.c | 8 +++ dlls/wined3d/state.c | 3 +- dlls/wined3d/utils.c | 118 ++++++++++++++++++++++++++++++--- dlls/wined3d/wined3d_private.h | 13 +++- 8 files changed, 187 insertions(+), 18 deletions(-) diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c index 2ed9aed490e..561a38ec5aa 100644 --- a/dlls/d3d11/tests/d3d11.c +++ b/dlls/d3d11/tests/d3d11.c @@ -28185,7 +28185,6 @@ static void test_fractional_viewports(void) ok(compare_float(v->x, expected.x, 0) && compare_float(v->y, expected.y, 0), "Got fragcoord {%.8e, %.8e}, expected {%.8e, %.8e} at (%u, %u), offset %.8e.\n", v->x, v->y, expected.x, expected.y, x, y, viewport_offsets[i]); - todo_wine ok(compare_float(v->z, expected.z, 2) && compare_float(v->w, expected.w, 2), "Got texcoord {%.8e, %.8e}, expected {%.8e, %.8e} at (%u, %u), offset %.8e.\n", v->z, v->w, expected.z, expected.w, x, y, viewport_offsets[i]); diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 6b514b15e25..4f052256882 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -16118,8 +16118,10 @@ static void test_depth_readback(void) /* The ddraw4 version of this test behaves similarly to the ddraw7 version on Nvidia GPUs, * except that Geforce 7 also returns garbage data in D24S8, whereas the ddraw7 version * returns 0 for that format. Give up on pre-filtering formats, accept Nvidia as generally - * broken here, but still expect at least one format (D16 or D24X8 in practise) to pass. */ - todo_wine_if(tests[i].todo) + * broken here, but still expect at least one format (D16 or D24X8 in practise) to pass. + * + * Some of the tested places pass on some GPUs on Wine by accident. */ + todo_wine_if(tests[i].todo && !compare_uint(expected_depth, depth, max_diff)) ok(compare_uint(expected_depth, depth, max_diff) || ddraw_is_nvidia(ddraw), "Test %u: Got depth 0x%08x (diff %d), expected 0x%08x+/-%u, at %u, %u.\n", i, depth, expected_depth - depth, expected_depth, max_diff, x, y); diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index 4c42d6f4b64..4402f2d93b5 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -15597,8 +15597,10 @@ static void test_depth_readback(void) * Geforce GTX 650 has working D16 and D24, but D24S8 returns 0. * * Arx Fatalis is broken on the Geforce 9 in the same way it was broken in Wine (bug 43654). - * The !tests[i].s_depth is supposed to rule out D16 on GF9 and D24X8 on GF7. */ - todo_wine_if(tests[i].todo) + * The !tests[i].s_depth is supposed to rule out D16 on GF9 and D24X8 on GF7. + * + * Some of the tested places pass on some GPUs on Wine by accident. */ + todo_wine_if(tests[i].todo && !compare_uint(expected_depth, depth, max_diff)) ok(compare_uint(expected_depth, depth, max_diff) || (ddraw_is_nvidia(ddraw) && (all_zero || all_one || !tests[i].s_depth)), "Test %u: Got depth 0x%08x (diff %d), expected 0x%08x+/-%u, at %u, %u.\n", diff --git a/dlls/wined3d/adapter_gl.c b/dlls/wined3d/adapter_gl.c index f1cdbffe718..2a91418b0d0 100644 --- a/dlls/wined3d/adapter_gl.c +++ b/dlls/wined3d/adapter_gl.c @@ -5142,6 +5142,7 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_ d3d_info->scaled_resolve = !!gl_info->supported[EXT_FRAMEBUFFER_MULTISAMPLE_BLIT_SCALED]; d3d_info->pbo = !!gl_info->supported[ARB_PIXEL_BUFFER_OBJECT]; d3d_info->feature_level = feature_level_from_caps(gl_info, &shader_caps, &fragment_caps); + d3d_info->filling_convention_offset = gl_info->filling_convention_offset; if (gl_info->supported[ARB_TEXTURE_MULTISAMPLE]) d3d_info->multisample_draw_location = WINED3D_LOCATION_TEXTURE_RGB; @@ -5149,6 +5150,53 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_ d3d_info->multisample_draw_location = WINED3D_LOCATION_RB_MULTISAMPLE; } +static float wined3d_adapter_find_fill_offset(struct wined3d_caps_gl_ctx *ctx) +{ + static const float test_array[] = + { + 0.0f, + -1.0f / 1024.0f, + -1.0f / 512.0f, + -1.0f / 256.0f, + -1.0f / 128.0f, + -1.0f / 64.0f + }; + unsigned int upper = ARRAY_SIZE(test_array), lower = 0, test; + float value; + + if (wined3d_settings.offscreen_rendering_mode != ORM_FBO) + goto end; + + while (upper != lower) + { + test = (upper + lower) / 2; + value = test_array[test]; + TRACE("Good %u lower %u, test %u.\n", upper, lower, test); + if (wined3d_caps_gl_ctx_test_filling_convention(ctx, value)) + upper = test; + else + lower = test + 1; + } + + if (upper < ARRAY_SIZE(test_array)) + { + value = test_array[upper]; + if (value) + WARN("Using a filling convention fixup offset of -1/%f.\n", -1.0f / value); + else + TRACE("No need for a filling convention offset.\n"); + + return value; + } + + FIXME("Did not find a way to get the filling convention we want.\n"); + +end: + /* This value was used unconditionally before the dynamic test function was + * introduced. */ + return -1.0f / 64.0f; +} + static BOOL wined3d_adapter_gl_init(struct wined3d_adapter_gl *adapter_gl, unsigned int ordinal, unsigned int wined3d_creation_flags) { @@ -5234,6 +5282,8 @@ static BOOL wined3d_adapter_gl_init(struct wined3d_adapter_gl *adapter_gl, return FALSE; } + gl_info->filling_convention_offset = wined3d_adapter_find_fill_offset(&caps_gl_ctx); + wined3d_adapter_gl_init_d3d_info(adapter_gl, wined3d_creation_flags); if (!adapter_gl->a.d3d_info.shader_color_key) diff --git a/dlls/wined3d/adapter_vk.c b/dlls/wined3d/adapter_vk.c index 02a359c4f07..d82b5221d3c 100644 --- a/dlls/wined3d/adapter_vk.c +++ b/dlls/wined3d/adapter_vk.c @@ -2224,6 +2224,14 @@ static void wined3d_adapter_vk_init_d3d_info(struct wined3d_adapter_vk *adapter_ d3d_info->pbo = true; d3d_info->feature_level = feature_level_from_caps(&shader_caps); + /* Like GL, Vulkan doesn't explicitly specify a filling convention and only mandates that a + * shared edge of two adjacent triangles generate a fragment for exactly one of the triangles. + * + * However, every Vulkan implementation we have seen so far uses a top-left rule. Hardware + * that differs either predates Vulkan (d3d9 class HW, GeForce 9xxx) or behaves the way we + * want in Vulkan (MacOS Radeon driver through MoltenVK). */ + d3d_info->filling_convention_offset = 0.0f; + d3d_info->multisample_draw_location = WINED3D_LOCATION_TEXTURE_RGB; } diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c index fd2ade572c8..901771fdcc1 100644 --- a/dlls/wined3d/state.c +++ b/dlls/wined3d/state.c @@ -4239,13 +4239,14 @@ static void viewport_miscpart_cc(struct wined3d_context *context, const struct wined3d_gl_info *gl_info = wined3d_context_gl(context)->gl_info; /* See get_projection_matrix() in utils.c for a discussion about those values. */ float pixel_center_offset = context->d3d_info->wined3d_creation_flags - & WINED3D_PIXEL_CENTER_INTEGER ? 63.0f / 128.0f : -1.0f / 128.0f; + & WINED3D_PIXEL_CENTER_INTEGER ? 0.5f : 0.0f; struct wined3d_viewport vp[WINED3D_MAX_VIEWPORTS]; GLdouble depth_ranges[2 * WINED3D_MAX_VIEWPORTS]; GLfloat viewports[4 * WINED3D_MAX_VIEWPORTS]; unsigned int i, reset_count = 0; float min_z, max_z; + pixel_center_offset += context->d3d_info->filling_convention_offset / 2.0f; get_viewports(context, state, state->viewport_count, vp); GL_EXTCALL(glClipControl(context->render_offscreen ? GL_UPPER_LEFT : GL_LOWER_LEFT, GL_ZERO_TO_ONE)); diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c index 29326668e07..26001775300 100644 --- a/dlls/wined3d/utils.c +++ b/dlls/wined3d/utils.c @@ -3914,6 +3914,100 @@ BOOL wined3d_caps_gl_ctx_test_viewport_subpixel_bits(struct wined3d_caps_gl_ctx return TRUE; } +bool wined3d_caps_gl_ctx_test_filling_convention(struct wined3d_caps_gl_ctx *ctx, float offset) +{ + static const struct wined3d_color red = {1.0f, 0.0f, 0.0f, 1.0f}; + const struct wined3d_gl_info *gl_info = ctx->gl_info; + unsigned int x, y, clear = 0, draw = 0; + GLuint texture, fbo; + DWORD readback[8][8]; + + /* This is a very simple test to find out how GL handles polygon edges: + * Draw a 1x1 quad exactly through 4 adjacent pixel centers in an 8x8 + * viewport and see which pixel it ends up in. So far we've seen top left + * and bottom left conventions. This test may produce unexpected results + * if the driver forces multisampling on us. + * + * If we find a bottom-left filling behavior we also move the x-axis + * by the same amount. This is necessary to keep diagonals that go + * through the pixel center intact. + * + * Note that we are ignoring some settings that might influence the + * driver: How we switch GL to an upper-left coordinate system, + * shaders vs fixed function GL. Testing these isn't possible with + * the current draw_test_quad() infrastructure. Also the test is + * skipped if we are not using FBOs. Drawing into the onscreen + * frame buffer may also yield different driver behavior. + * + * The minimum offset also depends on the viewport size, although + * the relation between those two is GPU dependent and not exactly + * sensible. E.g. a 8192x8192 viewport on a GeForce 9 needs at + * least an offset of 1/240.9, whereas a 8x8 one needs 1/255.982; + * 32x32 needs 1/255.935. 4x4 and lower are happy with something + * below 1/256. The 8x8 size below has been arbitrarily chosen to + * get a useful result out of that card and avoid allocating a + * gigantic texture during library init. + * + * Newer cards usually do the right thing anyway. In cases where + * they do not (e.g. Radeon GPUs in a macbookpro14,3 running MacOS) + * an offset of 1/2^20 is enough. */ + const struct wined3d_vec3 edge_geometry[] = + { + {(-1.0f + offset) / 8.0f, (-1.0f + offset) / 8.0f, 0.0f}, + {( 1.0f + offset) / 8.0f, (-1.0f + offset) / 8.0f, 0.0f}, + {(-1.0f + offset) / 8.0f, ( 1.0f + offset) / 8.0f, 0.0f}, + {( 1.0f + offset) / 8.0f, ( 1.0f + offset) / 8.0f, 0.0f}, + }; + + gl_info->gl_ops.gl.p_glGenTextures(1, &texture); + gl_info->gl_ops.gl.p_glBindTexture(GL_TEXTURE_2D, texture); + gl_info->gl_ops.gl.p_glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0); + gl_info->gl_ops.gl.p_glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 8, 8, 0, + GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, NULL); + gl_info->fbo_ops.glGenFramebuffers(1, &fbo); + gl_info->fbo_ops.glBindFramebuffer(GL_FRAMEBUFFER, fbo); + gl_info->fbo_ops.glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, + GL_TEXTURE_2D, texture, 0); + checkGLcall("create resources"); + + gl_info->gl_ops.gl.p_glViewport(0, 0, 8, 8); + gl_info->gl_ops.gl.p_glClearColor(0.0f, 0.0f, 1.0f, 1.0f); + gl_info->gl_ops.gl.p_glClear(GL_COLOR_BUFFER_BIT); + + draw_test_quad(ctx, edge_geometry, &red); + checkGLcall("draw"); + + gl_info->gl_ops.gl.p_glBindTexture(GL_TEXTURE_2D, texture); + gl_info->gl_ops.gl.p_glGetTexImage(GL_TEXTURE_2D, 0, + GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, readback); + checkGLcall("readback"); + + gl_info->gl_ops.gl.p_glDeleteTextures(1, &texture); + gl_info->fbo_ops.glDeleteFramebuffers(1, &fbo); + gl_info->fbo_ops.glBindFramebuffer(GL_FRAMEBUFFER, 0); + checkGLcall("delete resources"); + + /* We expect that exactly one fragment is generated. */ + for (y = 0; y < ARRAY_SIZE(readback); ++y) + { + for (x = 0; x < ARRAY_SIZE(readback[0]); ++x) + { + if (readback[y][x] == 0xff0000ff) + clear++; + else if (readback[y][x] == 0xffff0000) + draw++; + } + } + + if (clear != 63 || draw != 1) + { + FIXME("Unexpected filling convention test result.\n"); + return FALSE; + } + + /* One pixel was drawn, check if it is the expected one */ + return readback[3][3] == 0xffff0000; +} static float wined3d_adapter_find_polyoffset_scale(struct wined3d_caps_gl_ctx *ctx, GLenum format) { const struct wined3d_gl_info *gl_info = ctx->gl_info; @@ -5542,15 +5636,19 @@ void get_projection_matrix(const struct wined3d_context *context, const struct w * - We need to flip along the y-axis in case of offscreen rendering. * - OpenGL Z range is {-Wc,...,Wc} while D3D Z range is {0,...,Wc}. * - <= D3D9 coordinates refer to pixel centers while GL coordinates - * refer to pixel corners. - * - D3D has a top-left filling convention. We need to maintain this - * even after the y-flip mentioned above. - * In order to handle the last two points, we translate by - * (63.0 / 128.0) / VPw and (63.0 / 128.0) / VPh. This is equivalent to - * translating slightly less than half a pixel. We want the difference to - * be large enough that it doesn't get lost due to rounding inside the - * driver, but small enough to prevent it from interfering with any - * anti-aliasing. */ + * refer to pixel corners. D3D10 fixed this particular oddity. + * - D3D has a top-left filling convention while GL does not specify + * a particular behavior, other than that that the GL implementation + * needs to be consistent. + * + * In order to handle the pixel center, we translate by 0.5 / VPw and + * 0.5 / VPh. We test the filling convention during adapter init and + * add a small offset to correct it if necessary. See + * wined3d_caps_gl_ctx_test_filling_convention() for more details on how + * we test GL and considerations regarding the added offset value. + * + * If we have GL_ARB_clip_control we take care of all this through + * viewport properties and don't have to translate geometry. */ /* Projection matrices are <= d3d9, which all have integer pixel centers. */ if (!(d3d_info->wined3d_creation_flags & WINED3D_PIXEL_CENTER_INTEGER)) @@ -5559,7 +5657,7 @@ void get_projection_matrix(const struct wined3d_context *context, const struct w clip_control = d3d_info->clip_control; flip = !clip_control && context->render_offscreen; if (!clip_control) - center_offset = 63.0f / 64.0f; + center_offset = 1.0f + d3d_info->filling_convention_offset; else center_offset = 0.0f; diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index aa8974366a6..6e122ce31dc 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -243,6 +243,8 @@ struct wined3d_d3d_info enum wined3d_feature_level feature_level; DWORD multisample_draw_location; + + float filling_convention_offset; }; static const struct color_fixup_desc COLOR_FIXUP_IDENTITY = @@ -3265,6 +3267,7 @@ struct wined3d_gl_info DWORD quirks; BOOL supported[WINED3D_GL_EXT_COUNT]; GLint wrap_lookup[WINED3D_TADDRESS_MIRROR_ONCE - WINED3D_TADDRESS_WRAP + 1]; + float filling_convention_offset; HGLRC (WINAPI *p_wglCreateContextAttribsARB)(HDC dc, HGLRC share, const GLint *attribs); struct opengl_funcs gl_ops; @@ -3525,6 +3528,7 @@ BOOL wined3d_adapter_vk_init_format_info(struct wined3d_adapter_vk *adapter_vk, UINT64 adapter_adjust_memory(struct wined3d_adapter *adapter, INT64 amount) DECLSPEC_HIDDEN; BOOL wined3d_caps_gl_ctx_test_viewport_subpixel_bits(struct wined3d_caps_gl_ctx *ctx) DECLSPEC_HIDDEN; +bool wined3d_caps_gl_ctx_test_filling_convention(struct wined3d_caps_gl_ctx *ctx, float offset) DECLSPEC_HIDDEN; void install_gl_compat_wrapper(struct wined3d_gl_info *gl_info, enum wined3d_gl_extension ext) DECLSPEC_HIDDEN; @@ -5759,10 +5763,15 @@ static inline void shader_get_position_fixup(const struct wined3d_context *conte float center_offset; unsigned int i; + /* See get_projection_matrix() in utils.c for a discussion of the position fixup. + * This function here also applies to d3d10+ which does not need adjustment for + * integer pixel centers, but it may need the filling convention offset. */ if (context->d3d_info->wined3d_creation_flags & WINED3D_PIXEL_CENTER_INTEGER) - center_offset = 63.0f / 64.0f; + center_offset = 1.0f; else - center_offset = -1.0f / 64.0f; + center_offset = 0.0f; + + center_offset += context->d3d_info->filling_convention_offset; for (i = 0; i < fixup_count; ++i) { -- 2.32.0

Nov. 3, 2021

Re: [PATCH 8/8] wined3d: Try to allocate new Vulkan BOs from the client thread for DISCARD maps.

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > +static bool adapter_vk_alloc_bo(struct wined3d_device *device, struct wined3d_resource *resource, > + unsigned int sub_resource_idx, struct wined3d_bo_address *addr) > +{ > + wined3d_not_from_cs(device->cs); > + > + if (resource->type == WINED3D_RTYPE_BUFFER) > + { > + struct wined3d_buffer_vk *buffer_vk = wined3d_buffer_vk(buffer_from_resource(resource)); > + struct wined3d_client_bo_vk *client_bo; > + > + if (!(client_bo = heap_alloc(sizeof(*client_bo)))) > + return false; > + > + if (!wined3d_buffer_vk_create_buffer_object(buffer_vk, NULL, &client_bo->bo)) > + { > + heap_free(client_bo); > + return false; > + } > + > + if (!client_bo->bo.b.map_ptr) > + { > + struct wined3d_client_bo_vk_map_ctx ctx = {.device = device, .client_bo = client_bo}; > + > + WARN_(d3d_perf)("BO %p (chunk %p, slab %p) is not persistently mapped.\n", &client_bo->bo, > + client_bo->bo.memory ? client_bo->bo.memory->chunk : NULL, client_bo->bo.slab); > + > + wined3d_cs_map_object(device->cs, wined3d_client_bo_vk_map_cs, &ctx); > + wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_MAP); > + } wined3d_cs_map_object() almost sounds like it would emit a WINED3D_CS_OP_MAP, somewhat like wined3d_device_context_map() back when that was still called wined3d_cs_map()... Here too, Vulkan could map from application threads, it just needs to be synchronised. In the general case we'd need to make sure the GPU isn't still using the bo, but that's not an issue for newly allocated bo's. > +void wined3d_buffer_rename(struct wined3d_buffer *buffer, struct wined3d_context *context, struct wined3d_bo *bo) > +{ > + TRACE("buffer %p, context %p, bo %p.\n", buffer, context, bo); > + > + buffer->buffer_ops->buffer_rename_bo(buffer, context, bo); > + buffer->buffer_object = (uintptr_t)bo; > + wined3d_buffer_validate_location(buffer, WINED3D_LOCATION_BUFFER); > + wined3d_buffer_invalidate_location(buffer, ~WINED3D_LOCATION_BUFFER); > +} > + If we're doing this at the wined3d_buffer level, perhaps it would make sense to name this something like wined3d_buffer_set_bo(), wined3d_buffer_assign_bo(), or something along those lines. > +static void wined3d_buffer_vk_rename_bo(struct wined3d_buffer *buffer, > + struct wined3d_context *context, struct wined3d_bo *bo) > +{ > + struct wined3d_bo_vk *prev_bo = (struct wined3d_bo_vk *)buffer->buffer_object; > + struct wined3d_context_vk *context_vk = wined3d_context_vk(context); > + struct wined3d_buffer_vk *buffer_vk = wined3d_buffer_vk(buffer); > + struct wined3d_bo_vk *bo_vk = wined3d_bo_vk(bo); > + > + if (prev_bo) > + { > + struct wined3d_bo_user *bo_user; > + > + LIST_FOR_EACH_ENTRY(bo_user, &prev_bo->b.users, struct wined3d_bo_user, entry) > + bo_user->valid = false; > + assert(list_empty(&bo_vk->b.users)); > + list_move_head(&bo_vk->b.users, &prev_bo->b.users); > + > + if (prev_bo != &buffer_vk->bo) > + { > + struct wined3d_client_bo_vk *client_bo = CONTAINING_RECORD(prev_bo, struct wined3d_client_bo_vk, bo); > + > + wined3d_context_vk_destroy_bo(context_vk, &client_bo->bo); > + heap_free(client_bo); > + } > + } > + else > + { > + list_add_head(&bo_vk->b.users, &buffer_vk->b.bo_user.entry); > + } > +} > + I admit I didn't try very hard, but it's not immediately clear to me why we wouldn't always assign the new bo to "buffer_vk->bo" here, and mark the previous one for destruction.

Nov. 3, 2021

Re: [PATCH 7/8] wined3d: Avoid accessing the "bo" member of struct wined3d_buffer_vk.

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > --- > dlls/wined3d/adapter_vk.c | 4 ++-- > dlls/wined3d/buffer.c | 13 ++++++++----- > dlls/wined3d/context_vk.c | 8 ++++---- > dlls/wined3d/view.c | 14 ++++++++++---- > dlls/wined3d/wined3d_private.h | 2 +- > 5 files changed, 25 insertions(+), 16 deletions(-) > Why is that? I gather it's related to handling the wined3d_client_bo_vk structure from patch 8/8, but I don't think it's used enough in this series to get the full picture. In any case, that kind of information is helpful to have in the commit message.

Nov. 3, 2021

Re: [PATCH 6/8] wined3d: Allow passing a NULL context to wined3d_device_vk_create_bo().

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > --- > dlls/wined3d/context_vk.c | 19 +++++++------------ > dlls/wined3d/utils.c | 3 +++ > 2 files changed, 10 insertions(+), 12 deletions(-) > As mentioned in 1/8 and 3/8, this should essentially never happen in the Vulkan backend. > @@ -314,14 +314,14 @@ static struct wined3d_allocator_block *wined3d_device_vk_allocate_memory(struct > > EnterCriticalSection(&device_vk->allocator_cs); > > - if (size > WINED3D_ALLOCATOR_CHUNK_SIZE / 2) > + if (context_vk && size > WINED3D_ALLOCATOR_CHUNK_SIZE / 2) > { > *vk_memory = wined3d_context_vk_allocate_vram_chunk_memory(context_vk, memory_type, size); > LeaveCriticalSection(&device_vk->allocator_cs); > return NULL; > } > I suppose this works, but I think we should just enter this block and fail if we have a NULL context here. We're not going to successfully allocate a block larger than WINED3D_ALLOCATOR_CHUNK_SIZE / 2 below either, so there doesn't seem much point in trying. > @@ -398,14 +399,8 @@ static bool wined3d_device_vk_create_slab_bo(struct wined3d_device_vk *device_vk > } > slab->map = ~0u; > > - if (wine_rb_put(&device_vk->bo_slab_available, &key, &slab->entry) < 0) > - { > - ERR("Failed to add slab to available tree.\n"); > - wined3d_context_vk_destroy_bo(context_vk, &slab->bo); > - heap_free(slab); > - return false; > - } > - > + ret = wine_rb_put(&device_vk->bo_slab_available, &key, &slab->entry); > + assert(!ret); > TRACE("Created new bo slab %p.\n", slab); > } It doesn't seem quite right that we'd be able to create a bo without a context, but would then be unable to destroy it again... > @@ -7408,6 +7408,9 @@ struct wined3d_allocator_block *wined3d_allocator_allocate(struct wined3d_alloca > return block; > } > > + if (!context) > + return NULL; > + > if (!(chunk = allocator->ops->allocator_create_chunk(allocator, > context, memory_type, WINED3D_ALLOCATOR_CHUNK_SIZE))) > return NULL; Arguably, we could handle the NULL context in allocator_create_chunk(). It's somewhat moot though; in the Vulkan backend we should never have a NULL context, and in the OpenGL backend we can't do anything particularly useful with it.

Nov. 3, 2021

Re: [PATCH 5/8] wined3d: Protect access to the Vulkan wined3d_allocator with a critical section.

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > @@ -312,23 +312,36 @@ static struct wined3d_allocator_block *wined3d_device_vk_allocate_memory(struct > struct wined3d_allocator *allocator = &device_vk->allocator; > struct wined3d_allocator_block *block; > > + EnterCriticalSection(&device_vk->allocator_cs); > + We'd typically wrap the EnterCriticalSection() call in something like wined3d_device_vk_allocator_lock(). Mostly because that's a little easier to search for than an EnterCriticalSection() call with a specific critical section in some cases, but I suppose it would also make it slightly easier to replace the critical section with something else, if needed.

Nov. 3, 2021

Re: [PATCH 4/8] wined3d: Pass a wined3d_bo_vk pointer to wined3d_buffer_vk_create_buffer_object().

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:20, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > -static BOOL wined3d_buffer_vk_create_buffer_object(struct wined3d_buffer_vk *buffer_vk, > - struct wined3d_context_vk *context_vk) > +static BOOL wined3d_buffer_vk_create_buffer_object(const struct wined3d_buffer_vk *buffer_vk, > + struct wined3d_context_vk *context_vk, struct wined3d_bo_vk *bo) > { "bo_vk" by convention, although we're not terribly consistent about that everywhere. "BOOL" -> "bool" if we're touching it. > @@ -1433,19 +1433,8 @@ static BOOL wined3d_buffer_vk_create_buffer_object(struct wined3d_buffer_vk *buf > else if (!(resource->usage & WINED3DUSAGE_DYNAMIC)) > memory_type |= VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; > > - if (!(wined3d_device_vk_create_bo(wined3d_device_vk(resource->device), > - context_vk, resource->size, usage, memory_type, &buffer_vk->bo))) > - { > - WARN("Failed to create Vulkan buffer.\n"); > - return FALSE; > - } > - > - list_init(&buffer_vk->b.bo_user.entry); > - list_add_head(&buffer_vk->bo.b.users, &buffer_vk->b.bo_user.entry); > - buffer_vk->b.buffer_object = (uintptr_t)&buffer_vk->bo; > - buffer_invalidate_bo_range(&buffer_vk->b, 0, 0); > - > - return TRUE; > + return wined3d_device_vk_create_bo(wined3d_device_vk(resource->device), > + context_vk, resource->size, usage, memory_type, bo); > } > Is this the best way to handle this particular issue? I gather we're doing this primarily in order to create a bo with the correct "usage" and "memory_type" in patch 8/8. However, we could also achieve that by introducing helpers along the lines of vk_access_mask_from_bind_flags() to determine the correct "usage" and "memory_type", and then just call wined3d_device_vk_create_bo() directly from adapter_vk_alloc_bo() in patch 8/8. We may not necessarily need to pass a resource to adapter_alloc_bo() in that case either.

Nov. 3, 2021

Re: [PATCH 3/8] wined3d: Move the bo_slab_available field to struct wined3d_device_vk.

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:20, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > > In order to allow slab allocation from the client thread. > As mentioned in 1/8, this is not necessarily required for the Vulkan backend. It makes sense to move this to the device for GL, but in that case perhaps it also makes sense to move it to struct wined3d_device instead of struct wined3d_device_vk. > @@ -512,6 +524,7 @@ static HRESULT adapter_vk_create_device(struct wined3d *wined3d, const struct wi > hr = E_FAIL; > goto fail; > } > + wine_rb_init(&device_vk->bo_slab_available, wined3d_bo_slab_vk_compare); > > if (FAILED(hr = wined3d_device_init(&device_vk->d, wined3d, adapter->ordinal, device_type, focus_window, > flags, surface_alignment, levels, level_count, vk_info->supported, device_parent))) This should call wine_rb_destroy() on wined3d_device_init() failure. It turns out not doing that doesn't actually leak anything because wine_rb_init() only does trivial initialisation, but that's an implementation detail and could change.

Nov. 3, 2021

Re: [PATCH 2/8] wined3d: Pass a wined3d_device_vk to wined3d_context_vk_allocate_memory().

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:20, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > -static struct wined3d_allocator_block *wined3d_context_vk_allocate_memory(struct wined3d_context_vk *context_vk, > - unsigned int memory_type, VkDeviceSize size, VkDeviceMemory *vk_memory) > +static struct wined3d_allocator_block *wined3d_device_vk_allocate_memory(struct wined3d_device_vk *device_vk, > + struct wined3d_context_vk *context_vk, unsigned int memory_type, VkDeviceSize size, VkDeviceMemory *vk_memory) > { Device functions in device.c. > -static bool wined3d_context_vk_create_slab_bo(struct wined3d_device_vk *device_vk, struct wined3d_context_vk *context_vk, > +static bool wined3d_device_vk_create_slab_bo(struct wined3d_device_vk *device_vk, struct wined3d_context_vk *context_vk, > VkDeviceSize size, VkBufferUsageFlags usage, VkMemoryPropertyFlags memory_type, struct wined3d_bo_vk *bo) > { This change belong in patch 1/8. (Or possibly even a separate patch.)

Nov. 3, 2021

Re: [PATCH 1/8] wined3d: Pass a wined3d_device_vk to wined3d_context_vk_create_bo().

by Henri Verbeet

On Wed, 3 Nov 2021 at 00:20, Zebediah Figura <zfigura(a)codeweavers.com> wrote: > With the goal of making the context_vk parameter optional, so that we can > allocate new BOs from the client thread. > This would mainly be for the wined3d_buffer_vk_create_buffer_object() call in patch 8/8, but in the Vulkan backend, we can always get the context through "device_vk->context_vk", provided we're inside an init_3d()/uninit_3d() pair. I.e., the Vulkan backend can make Vulkan calls from applications threads, it just needs synchronisation. I.e., I don't think the Vulkan backend needs this. That said, I still think this change is probably the right thing to do, but with a bit of a different justification. We'll likely need a change like this for wined3d_context_gl_create_bo(), and then it makes sense to make wined3d_context_vk_create_bo() a device function as well for consistency. > -static bool wined3d_context_vk_create_slab_bo(struct wined3d_context_vk *context_vk, > +static bool wined3d_context_vk_create_slab_bo(struct wined3d_device_vk *device_vk, struct wined3d_context_vk *context_vk, > VkDeviceSize size, VkBufferUsageFlags usage, VkMemoryPropertyFlags memory_type, struct wined3d_bo_vk *bo) > { If we're making this a device function, that should probably be called wined3d_device_vk_create_slab_bo(), and be moved to device.c. > -BOOL wined3d_context_vk_create_bo(struct wined3d_context_vk *context_vk, VkDeviceSize size, > - VkBufferUsageFlags usage, VkMemoryPropertyFlags memory_type, struct wined3d_bo_vk *bo) > +BOOL wined3d_device_vk_create_bo(struct wined3d_device_vk *device_vk, struct wined3d_context_vk *context_vk, > + VkDeviceSize size, VkBufferUsageFlags usage, VkMemoryPropertyFlags memory_type, struct wined3d_bo_vk *bo) > { If we're making this a device function, this should be moved to device.c. And if we're touching this, be may as well use the standard "bool" instead of "BOOL".

Nov. 3, 2021