[PATCH v5 0/2] MR3431: wined3d: Fix clears of the current render target in Vulkan
Previously, clearing the current render target would delay the clear until the render pass was ended by something else Fixes missing UI in [Moving Out](https://store.steampowered.com/app/996770/Moving_Out/) -- v5: wined3d: Restart vk render pass on RT clear. d3d11: Add tests for clearing RTs in the middle of a render. https://gitlab.winehq.org/wine/wine/-/merge_requests/3431
From: Evan Tang <etang(a)codeweavers.com> --- dlls/d3d11/tests/d3d11.c | 174 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c index b144fb36aca..0fe2ea2c550 100644 --- a/dlls/d3d11/tests/d3d11.c +++ b/dlls/d3d11/tests/d3d11.c @@ -35266,6 +35266,179 @@ static void test_keyed_mutex(void) ok(!ref, "got %ld.\n", ref); } +static void test_clear_during_render(void) +{ + /* Quads only cover left half so we can see both drawn color on the left and clear color on the right. */ + static const struct vec3 depth_quarter_quad_data[] = + { + {-1.0f, -1.0f, 0.25f}, + {-1.0f, 1.0f, 0.25f}, + { 0.0f, -1.0f, 0.25f}, + { 0.0f, 1.0f, 0.25f}, + }; + static const struct vec3 depth_three_quarter_quad_data[] = + { + {-1.0f, -1.0f, 0.75f}, + {-1.0f, 1.0f, 0.75f}, + { 0.0f, -1.0f, 0.75f}, + { 0.0f, 1.0f, 0.75f}, + }; + static const struct vec4 draw_color_data = {1, 0, 0, 1}; + struct d3d11_test_context test_context; + struct resource_readback rb; + ID3D11DeviceContext *context; + ID3D11Device *device; + ID3D11Texture2D *tex, *dstex; + ID3D11DepthStencilView *dsv; + ID3D11RenderTargetView *rtv; + ID3D11DepthStencilState *depth_none, *depth_ge, *depth_write_ge; + ID3D11Buffer *depth_quarter_quad, *depth_three_quarter_quad, *draw_color; + D3D11_DEPTH_STENCIL_DESC depth_stencil_desc; + D3D11_TEXTURE2D_DESC texture_desc; + HRESULT hr; + unsigned int test; + + if (!init_test_context(&test_context, NULL)) + return; + + device = test_context.device; + context = test_context.immediate_context; + depth_quarter_quad = create_buffer(device, D3D11_BIND_VERTEX_BUFFER, sizeof(depth_quarter_quad_data), &depth_quarter_quad_data); + depth_three_quarter_quad = create_buffer(device, D3D11_BIND_VERTEX_BUFFER, sizeof(depth_three_quarter_quad_data), &depth_three_quarter_quad_data); + draw_color = create_buffer(device, D3D11_BIND_CONSTANT_BUFFER, sizeof(draw_color_data), &draw_color_data); + + memset(&depth_stencil_desc, 0, sizeof(depth_stencil_desc)); + hr = ID3D11Device_CreateDepthStencilState(device, &depth_stencil_desc, &depth_none); + ok(hr == S_OK, "Failed to create depth none dss: %#lx.\n", hr); + depth_stencil_desc.DepthEnable = TRUE; + depth_stencil_desc.DepthWriteMask = D3D11_DEPTH_WRITE_MASK_ALL; + depth_stencil_desc.DepthFunc = D3D11_COMPARISON_ALWAYS; + depth_stencil_desc.DepthFunc = D3D11_COMPARISON_GREATER_EQUAL; + hr = ID3D11Device_CreateDepthStencilState(device, &depth_stencil_desc, &depth_write_ge); + ok(hr == S_OK, "Failed to create depth write ge dss: %#lx.\n", hr); + depth_stencil_desc.DepthWriteMask = D3D11_DEPTH_WRITE_MASK_ZERO; + hr = ID3D11Device_CreateDepthStencilState(device, &depth_stencil_desc, &depth_ge); + ok(hr == S_OK, "Failed to create depth ge dss: %#lx.\n", hr); + + memset(&texture_desc, 0, sizeof(texture_desc)); + texture_desc.Width = 2; + texture_desc.Height = 1; + texture_desc.MipLevels = 1; + texture_desc.ArraySize = 1; + texture_desc.SampleDesc.Count = 1; + texture_desc.Usage = D3D11_USAGE_DEFAULT; + texture_desc.BindFlags = D3D11_BIND_RENDER_TARGET; + texture_desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; + hr = ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &tex); + ok(hr == S_OK, "Failed to create color texture: %#lx.\n", hr); + texture_desc.BindFlags = D3D11_BIND_DEPTH_STENCIL; + texture_desc.Format = DXGI_FORMAT_D32_FLOAT; + hr = ID3D11Device_CreateTexture2D(device, &texture_desc, NULL, &dstex); + ok(hr == S_OK, "Failed to create depth texture: %#lx.\n", hr); + hr = ID3D11Device_CreateRenderTargetView(device, (ID3D11Resource*)tex, NULL, &rtv); + ok(hr == S_OK, "Failed to create rtv: %#lx.\n", hr); + hr = ID3D11Device_CreateDepthStencilView(device, (ID3D11Resource*)dstex, NULL, &dsv); + ok(hr == S_OK, "Failed to create dsv: %#lx.\n", hr); + + for (test = 0; test < 5; test++) + { + unsigned int i; + static const UINT vertex_stride = sizeof(depth_quarter_quad_data[0]); + static const UINT vertex_offset = 0; + static const struct vec4 initial_color = {0, 0, 0, 1}; + static const float clear_color[] = {0, 1, 0, 1}; + DWORD expected_color[] = {0xff000000, 0xff000000}; + float expected_depth[] = {0.5f, 0.5f}; + D3D11_VIEWPORT viewport = {0, 0, (float)texture_desc.Width, (float)texture_desc.Height, 0, 1}; + + ID3D11DeviceContext_RSSetViewports(context, 1, &viewport); + ID3D11DeviceContext_OMSetRenderTargets(context, 1, &rtv, dsv); + ID3D11DeviceContext_OMSetDepthStencilState(context, depth_none, 0); + ID3D11DeviceContext_ClearDepthStencilView(context, dsv, D3D11_CLEAR_DEPTH, 0.5f, 0); + draw_color_quad(&test_context, &initial_color); + + /* We can't use draw_color_quad in here because the buffer updates it does can suppress bugs. */ + switch (test) + { + case 0: + /* Nothing, verify initial values. */ + break; + case 1: + /* Draw with succeeding depth compare. */ + ID3D11DeviceContext_ClearDepthStencilView(context, dsv, D3D11_CLEAR_DEPTH, 0.0f, 0); + ID3D11DeviceContext_OMSetDepthStencilState(context, depth_ge, 0); + ID3D11DeviceContext_IASetVertexBuffers(context, 0, 1, &depth_quarter_quad, &vertex_stride, &vertex_offset); + ID3D11DeviceContext_PSSetConstantBuffers(context, 0, 1, &draw_color); + ID3D11DeviceContext_Draw(context, 4, 0); + expected_color[0] = 0xff0000ff; + expected_depth[0] = expected_depth[1] = 0.0f; + break; + case 2: + /* Draw with failing depth compare. */ + ID3D11DeviceContext_ClearDepthStencilView(context, dsv, D3D11_CLEAR_DEPTH, 1.0f, 0); + ID3D11DeviceContext_OMSetDepthStencilState(context, depth_ge, 0); + ID3D11DeviceContext_IASetVertexBuffers(context, 0, 1, &depth_three_quarter_quad, &vertex_stride, &vertex_offset); + ID3D11DeviceContext_PSSetConstantBuffers(context, 0, 1, &draw_color); + ID3D11DeviceContext_Draw(context, 4, 0); + expected_depth[0] = expected_depth[1] = 1.0f; + break; + case 3: + /* Draw with depth write. */ + ID3D11DeviceContext_ClearDepthStencilView(context, dsv, D3D11_CLEAR_DEPTH, 0.0f, 0); + ID3D11DeviceContext_OMSetDepthStencilState(context, depth_write_ge, 0); + ID3D11DeviceContext_IASetVertexBuffers(context, 0, 1, &depth_quarter_quad, &vertex_stride, &vertex_offset); + ID3D11DeviceContext_PSSetShader(context, NULL, NULL, 0); + ID3D11DeviceContext_Draw(context, 4, 0); + expected_depth[0] = 0.25f; + expected_depth[1] = 0.0f; + break; + case 4: + /* Color write. */ + ID3D11DeviceContext_ClearRenderTargetView(context, rtv, clear_color); + ID3D11DeviceContext_IASetVertexBuffers(context, 0, 1, &depth_quarter_quad, &vertex_stride, &vertex_offset); + ID3D11DeviceContext_PSSetConstantBuffers(context, 0, 1, &draw_color); + ID3D11DeviceContext_Draw(context, 4, 0); + expected_color[0] = 0xff0000ff; + expected_color[1] = 0xff00ff00; + break; + default: + break; + } + + get_texture_readback(tex, 0, &rb); + for (i = 0; i < ARRAY_SIZE(expected_color); i++) + { + DWORD color = get_readback_color(&rb, i, 0, 0); + todo_wine_if( + (damavand && (((test == 1 || test == 2) && i == 0) || (test == 4 && i == 1))) || + (!damavand && test == 3 && i == 0) /* Draw with no PS doesn't write color on D3D11 but does on OpenGL. */ + ) + ok(color == expected_color[i], "Test %d: Got unexpected color 0x%08lx instead of 0x%08lx at %u.\n", test, color, expected_color[i], i); + } + release_resource_readback(&rb); + get_texture_readback(dstex, 0, &rb); + for (i = 0; i < ARRAY_SIZE(expected_depth); i++) + { + float depth = get_readback_float(&rb, i, 0); + todo_wine_if(damavand && test == 3) + ok(depth == expected_depth[i], "Test %d: Got unexpected depth %.8e instead of %.8e at %u.\n", test, depth, expected_depth[i], i); + } + release_resource_readback(&rb); + } + + ID3D11Buffer_Release(depth_quarter_quad); + ID3D11Buffer_Release(depth_three_quarter_quad); + ID3D11Buffer_Release(draw_color); + ID3D11RenderTargetView_Release(rtv); + ID3D11DepthStencilView_Release(dsv); + ID3D11Texture2D_Release(tex); + ID3D11Texture2D_Release(dstex); + ID3D11DepthStencilState_Release(depth_none); + ID3D11DepthStencilState_Release(depth_ge); + ID3D11DepthStencilState_Release(depth_write_ge); + release_test_context(&test_context); +} + START_TEST(d3d11) { unsigned int argc, i; @@ -35462,6 +35635,7 @@ START_TEST(d3d11) queue_test(test_dxgi_resources); queue_for_each_feature_level(test_shared_resource); queue_test(test_keyed_mutex); + queue_test(test_clear_during_render); run_queued_tests(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3431
From: Evan Tang <etang(a)codeweavers.com> --- dlls/d3d11/tests/d3d11.c | 6 +----- dlls/wined3d/context_vk.c | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c index 0fe2ea2c550..95d70714de2 100644 --- a/dlls/d3d11/tests/d3d11.c +++ b/dlls/d3d11/tests/d3d11.c @@ -35409,10 +35409,7 @@ static void test_clear_during_render(void) for (i = 0; i < ARRAY_SIZE(expected_color); i++) { DWORD color = get_readback_color(&rb, i, 0, 0); - todo_wine_if( - (damavand && (((test == 1 || test == 2) && i == 0) || (test == 4 && i == 1))) || - (!damavand && test == 3 && i == 0) /* Draw with no PS doesn't write color on D3D11 but does on OpenGL. */ - ) + todo_wine_if(!damavand && test == 3 && i == 0) /* Draw with no PS doesn't write color on D3D11 but does on OpenGL. */ ok(color == expected_color[i], "Test %d: Got unexpected color 0x%08lx instead of 0x%08lx at %u.\n", test, color, expected_color[i], i); } release_resource_readback(&rb); @@ -35420,7 +35417,6 @@ static void test_clear_during_render(void) for (i = 0; i < ARRAY_SIZE(expected_depth); i++) { float depth = get_readback_float(&rb, i, 0); - todo_wine_if(damavand && test == 3) ok(depth == expected_depth[i], "Test %d: Got unexpected depth %.8e instead of %.8e at %u.\n", test, depth, expected_depth[i], i); } release_resource_readback(&rb); diff --git a/dlls/wined3d/context_vk.c b/dlls/wined3d/context_vk.c index 859e8fb5949..abd75066f29 100644 --- a/dlls/wined3d/context_vk.c +++ b/dlls/wined3d/context_vk.c @@ -3565,6 +3565,20 @@ static void wined3d_context_vk_load_shader_resources(struct wined3d_context_vk * } } +static void wined3d_context_prepare_used_rtv(struct wined3d_context *context, struct wined3d_rendertarget_view *rtv) +{ + if (wined3d_rendertarget_view_get_locations(rtv) & WINED3D_LOCATION_CLEARED) + { + /* Need to restart render pass or the clear won't happen. */ + context_invalidate_state(context, STATE_FRAMEBUFFER); + wined3d_rendertarget_view_prepare_location(rtv, context, rtv->resource->draw_binding); + } + else + { + wined3d_rendertarget_view_load_location(rtv, context, rtv->resource->draw_binding); + } +} + VkCommandBuffer wined3d_context_vk_apply_draw_state(struct wined3d_context_vk *context_vk, const struct wined3d_state *state, struct wined3d_buffer_vk *indirect_vk, bool indexed) { @@ -3607,10 +3621,7 @@ VkCommandBuffer wined3d_context_vk_apply_draw_state(struct wined3d_context_vk *c { /* We handle clears at the beginning of the render pass, no need for an explicit clear * first. */ - if (wined3d_rendertarget_view_get_locations(rtv) & WINED3D_LOCATION_CLEARED) - wined3d_rendertarget_view_prepare_location(rtv, &context_vk->c, rtv->resource->draw_binding); - else - wined3d_rendertarget_view_load_location(rtv, &context_vk->c, rtv->resource->draw_binding); + wined3d_context_prepare_used_rtv(&context_vk->c, rtv); invalidate_rt |= (1 << i); } else @@ -3628,16 +3639,9 @@ VkCommandBuffer wined3d_context_vk_apply_draw_state(struct wined3d_context_vk *c if ((dsv = state->fb.depth_stencil)) { if (wined3d_state_uses_depth_buffer(state)) - { - if (wined3d_rendertarget_view_get_locations(dsv) & WINED3D_LOCATION_CLEARED) - wined3d_rendertarget_view_prepare_location(dsv, &context_vk->c, dsv->resource->draw_binding); - else - wined3d_rendertarget_view_load_location(dsv, &context_vk->c, dsv->resource->draw_binding); - } + wined3d_context_prepare_used_rtv(&context_vk->c, dsv); else - { wined3d_rendertarget_view_prepare_location(dsv, &context_vk->c, dsv->resource->draw_binding); - } if (!state->depth_stencil_state || state->depth_stencil_state->writes_ds) invalidate_ds = true; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3431
Personally, I'm not a fan of the switch/case style of tests. My preference would be to either just explicitly write out the tests, or to encode them using the `tests[]` construction we use in a couple of other tests.
Do you have any recommendations on what to put in a `tests[]` table? I feel like I'd end up with this, which doesn't make it super obvious how the values get used in the test. ```c static const struct { BOOL clear_depth; float depth_clear_value; BOOL clear_color; float color_clear_value[4]; enum { DEPTH_NONE, DEPTH_GE, DEPTH_WRITE_GE } depth_state; enum { DEPTH_QUARTER_QUAD, DEPTH_THREE_QUARTER_QUAD } vertex_buffer_depth; BOOL use_ps; DWORD expected_color[2]; float expected_depth[2]; } tests[] = { /* Draw with succeeding depth compare */ {TRUE, 0.0f, FALSE, {0, 0, 0, 0}, DEPTH_GE, DEPTH_QUARTER_QUAD, TRUE, {0xff0000ff, 0xff000000}, {0.00f, 0.00f}}, /* Draw with failing depth compare */ {TRUE, 1.0f, FALSE, {0, 0, 0, 0}, DEPTH_GE, DEPTH_THREE_QUARTER_QUAD, TRUE, {0xff000000, 0xff000000}, {1.00f, 1.00f}}, /* Draw with depth write */ {TRUE, 0.0f, FALSE, {0, 0, 0, 0}, DEPTH_WRITE_GE, DEPTH_QUARTER_QUAD, FALSE, {0xff000000, 0xff000000}, {0.25f, 0.00f}}, /* Color write */ {FALSE, 0.0f, TRUE, {0, 1, 0, 1}, DEPTH_NONE, DEPTH_QUARTER_QUAD, TRUE, {0xff0000ff, 0xff00ff00}, {0.50f, 0.50f}}, }; ```
I don't think the comment is terribly clear, but it sounds like we're getting fixed-function with the GL backend, and things just happen to work with the Vulkan backend because we don't yet have a fixed-function implementation there.
As in we're getting GL's emulation of d3d ffp, or we're hitting GL ffp? [I took an apitrace](/uploads/f29fa0f24cb38f3f2600183eddd28201/d3d11_test.trace) and if you look at call 233640, it is using a program with a VS and no FS, so I don't think we're hitting ffp emulation. As for whether we're hitting GL's ffp path, I don't think so but I don't really know GL that well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3431#note_42542
I'm fairly lenient about it, but since you'll be rebasing, please consider Henri's suggestions.
I feel similarly, especially when it comes to test, but I think I agree with most of that. I don't mind the switch in tests on principle, but in this case it doesn't seem to be doing anything, since the only meaningfully shared code is the clear and probe, so it probably makes more sense just to write it all out linearly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3431#note_42569
Personally, I'm not a fan of the switch/case style of tests. My preference would be to either just explicitly write out the tests, or to encode them using the `tests[]` construction we use in a couple of other tests.
Do you have any recommendations on what to put in a `tests[]` table? I feel like I'd end up with this, which doesn't make it super obvious how the values get used in the test.
Mostly what Zeb said; there doesn't seem all that much commonality between these. Generally, I think we should value clarity over compactness for the tests, particularly if we'd like people other than the original author to work on them.
I don't mind the switch in tests on principle,
I'm sure there are exceptions, but generally I find the "switch on loop index" construct not all that much more readable than just using "goto"...
I don't think the comment is terribly clear, but it sounds like we're getting fixed-function with the GL backend, and things just happen to work with the Vulkan backend because we don't yet have a fixed-function implementation there.
As in we're getting GL's emulation of d3d ffp, or we're hitting GL ffp? [I took an apitrace](https://gitlab.winehq.org/wine/wine/uploads/f29fa0f24cb38f3f2600183eddd28201...) and if you look at call 233640, it is using a program with a VS and no FS, so I don't think we're hitting ffp emulation. As for whether we're hitting GL's ffp path, I don't think so but I don't really know GL that well.
Well, either. Looking at the actual code in set_glsl_shader_program() though, it looks like we'd end up with "ps_id" being 0 when we have a sm4+ vertex shader, and that seems consistent with the apitrace. In a compatibility context, that would get us GL fixed-function, while in a pure core context we'd probably get a GL error. I imagine we instead get validation layer errors from this with the Vulkan backend? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3431#note_42602
participants (4)
-
Evan Tang -
Evan Tang (＠etang-cw)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)