[PATCH 1/2] wined3d: Add sync object after query buffer issue.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45932 Signed-off-by: Andrew Wesie <awesie(a)gmail.com> --- Notes: Fixes regression on NVIDIA cards due to query buffer objects. Change tested on NVIDIA proprietary driver, Intel Mesa driver, and AMD Mesa driver. dlls/wined3d/query.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/wined3d/query.c b/dlls/wined3d/query.c index 26c32b1f13..d968b0a744 100644 --- a/dlls/wined3d/query.c +++ b/dlls/wined3d/query.c @@ -40,7 +40,7 @@ static BOOL wined3d_query_buffer_is_valid(struct wined3d_query *query) static void wined3d_query_create_buffer_object(struct wined3d_context *context, struct wined3d_query *query) { const struct wined3d_gl_info *gl_info = context->gl_info; - const GLuint map_flags = GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT | GL_MAP_COHERENT_BIT; + const GLuint map_flags = GL_MAP_READ_BIT | GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT; GLuint buffer_object; GL_EXTCALL(glGenBuffers(1, &buffer_object)); @@ -76,6 +76,7 @@ static void wined3d_query_destroy_buffer_object(struct wined3d_context *context, static BOOL wined3d_query_buffer_queue_result(struct wined3d_context *context, struct wined3d_query *query, GLuint id) { const struct wined3d_gl_info *gl_info = context->gl_info; + GLsync tmp_sync; if (!gl_info->supported[ARB_QUERY_BUFFER_OBJECT] || !gl_info->supported[ARB_BUFFER_STORAGE]) return FALSE; @@ -101,9 +102,15 @@ static BOOL wined3d_query_buffer_queue_result(struct wined3d_context *context, s /* Read the same value twice. We know we have the result if map_ptr[0] == map_ptr[1]. */ GL_EXTCALL(glGetQueryObjectui64v(id, GL_QUERY_RESULT, (void *)0)); GL_EXTCALL(glGetQueryObjectui64v(id, GL_QUERY_RESULT, (void *)sizeof(query->map_ptr[0]))); + GL_EXTCALL(glMemoryBarrier(GL_CLIENT_MAPPED_BUFFER_BARRIER_BIT)); GL_EXTCALL(glBindBuffer(GL_QUERY_BUFFER, 0)); checkGLcall("queue query result"); + /* NVIDIA requires a sync object. */ + tmp_sync = GL_EXTCALL(glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0)); + GL_EXTCALL(glDeleteSync(tmp_sync)); + checkGLcall("query buffer sync"); + return TRUE; } -- 2.17.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45932 Signed-off-by: Andrew Wesie <awesie(a)gmail.com> --- Notes: Reproduces regression reported in bug 45932. dlls/d3d9/tests/device.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 66bf5b00c5..3e7663622f 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -5948,6 +5948,32 @@ static void test_occlusion_query(void) || broken(data.dword[0] < 0xffffffff && !data.dword[1]), "Got unexpected query result 0x%08x%08x.\n", data.dword[1], data.dword[0]); + hr = IDirect3DDevice9_BeginScene(device); + ok(SUCCEEDED(hr), "Failed to begin scene, hr %#x.\n", hr); + for (i = 0; i < 50000; ++i) + { + hr = IDirect3DQuery9_Issue(query, D3DISSUE_BEGIN); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DQuery9_Issue(query, D3DISSUE_END); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + hr = IDirect3DDevice9_EndScene(device); + ok(SUCCEEDED(hr), "Failed to end scene, hr %#x.\n", hr); + + for (i = 0; i < 500; ++i) + { + if ((hr = IDirect3DQuery9_GetData(query, NULL, 0, D3DGETDATA_FLUSH)) == S_OK) + break; + Sleep(10); + } + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + + memset(&data, 0xff, sizeof(data)); + hr = IDirect3DQuery9_GetData(query, &data, sizeof(data), D3DGETDATA_FLUSH); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + ok(data.dword[0] == 0 && data.dword[1] == 0, + "Got unexpected query result 0x%08x%08x.\n", data.dword[1], data.dword[0]); + IDirect3DSurface9_Release(rt); done: -- 2.17.1
Hi, While running your changed tests on Windows, 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=43105 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)
On Sun, 14 Oct 2018 at 01:09, Andrew Wesie <awesie(a)gmail.com> wrote:
+ GL_EXTCALL(glMemoryBarrier(GL_CLIENT_MAPPED_BUFFER_BARRIER_BIT)); The ARB_buffer_storage spec is written against GL 4.3, but doesn't seem to require it. I think that means you need to check for ARB_SHADER_IMAGE_LOAD_STORE before using glMemoryBarrier().
+ /* NVIDIA requires a sync object. */ I think I'd phrase that as "The ARB_buffer_storage spec requires …", or just quote the relevant lines from the spec literally, and then add a note that NVIDIA appears to enforce that, while Mesa doesn't.
On Sun, Oct 14, 2018 at 9:45 AM Henri Verbeet <hverbeet(a)gmail.com> wrote:
On Sun, 14 Oct 2018 at 01:09, Andrew Wesie <awesie(a)gmail.com> wrote:
+ GL_EXTCALL(glMemoryBarrier(GL_CLIENT_MAPPED_BUFFER_BARRIER_BIT)); The ARB_buffer_storage spec is written against GL 4.3, but doesn't seem to require it. I think that means you need to check for ARB_SHADER_IMAGE_LOAD_STORE before using glMemoryBarrier().
That would imply that it is impossible to use non-coherent, persistent mappings unless we have the ARB_SHADER_IMAGE_LOAD_STORE extension. And based on the piglit tests, you are probably right. I'll revert back to using GL_MAP_COHERENT_BIT unless I can make a good case for the non-coherent mapping.
+ /* NVIDIA requires a sync object. */ I think I'd phrase that as "The ARB_buffer_storage spec requires …", or just quote the relevant lines from the spec literally, and then add a note that NVIDIA appears to enforce that, while Mesa doesn't.
After further investigation, I don't believe that this statement is accurate. If I modify the implementation to avoid ARB_buffer_storage entirely (e.g. use glBufferData, never map the buffer, etc.), the sync object is still required to avoid NVIDIA crashing. I'm more inclined to indicate that this is a driver-specific workaround. You can demonstrate the bad behavior on NVIDIA with a simple patch to the piglit query_buffer_object tests (which also do not use ARB_buffer_storage): diff --git a/tests/spec/arb_query_buffer_object/qbo.c b/tests/spec/arb_query_buffer_object/qbo.c index 3ba11ee26..bfedefce0 100644 --- a/tests/spec/arb_query_buffer_object/qbo.c +++ b/tests/spec/arb_query_buffer_object/qbo.c @@ -138,7 +138,10 @@ run_subtest(void) else if (result_type == GL_UNSIGNED_INT) glGetQueryObjectuivARB(query, GL_QUERY_RESULT, BUFFER_OFFSET(0)); else - glGetQueryObjectui64v(query, GL_QUERY_RESULT, BUFFER_OFFSET(0)); + { + for (unsigned int i = 0; i < 100000; i++) + glGetQueryObjectui64v(query, GL_QUERY_RESULT, BUFFER_OFFSET(0)); + } } else { if (result_type == GL_INT) { glGetQueryObjectivARB(query, GL_QUERY_RESULT_AVAILABLE, BUFFER_OFFSET(8));
On Sun, 14 Oct 2018 at 20:48, Andrew Wesie <awesie(a)gmail.com> wrote:
On Sun, Oct 14, 2018 at 9:45 AM Henri Verbeet <hverbeet(a)gmail.com> wrote:
On Sun, 14 Oct 2018 at 01:09, Andrew Wesie <awesie(a)gmail.com> wrote:
+ /* NVIDIA requires a sync object. */ I think I'd phrase that as "The ARB_buffer_storage spec requires …", or just quote the relevant lines from the spec literally, and then add a note that NVIDIA appears to enforce that, while Mesa doesn't.
After further investigation, I don't believe that this statement is accurate. If I modify the implementation to avoid ARB_buffer_storage entirely (e.g. use glBufferData, never map the buffer, etc.), the sync object is still required to avoid NVIDIA crashing. I'm more inclined to indicate that this is a driver-specific workaround.
It may very well be unintentional that NVIDIA needs the sync object. The buffer storage spec does contain that language though, so it's legal for any driver to require it, even if that requirement seems slightly odd. Note also that as a rule, we avoid workarounds for driver bugs.
participants (3)
-
Andrew Wesie -
Henri Verbeet -
Marvin