[PATCH 1/3] wined3d: Handle discarded buffers in buffer_resource_sub_resource_map().
This is essentially dead code currently; we never leave buffers in a discarded state. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/buffer.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/dlls/wined3d/buffer.c b/dlls/wined3d/buffer.c index b42c7bd919a..e1c1f7aa5fc 100644 --- a/dlls/wined3d/buffer.c +++ b/dlls/wined3d/buffer.c @@ -915,6 +915,7 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc struct wined3d_buffer *buffer = buffer_from_resource(resource); struct wined3d_device *device = resource->device; struct wined3d_context *context; + struct wined3d_bo_address addr; unsigned int offset, size; uint8_t *base; LONG count; @@ -927,10 +928,13 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc count = ++resource->map_count; - if (buffer->buffer_object) + context = context_acquire(device, NULL, 0); + + wined3d_buffer_get_memory(buffer, context, &addr); + + if (addr.buffer_object) { unsigned int dirty_offset = offset, dirty_size = size; - struct wined3d_bo_address addr; /* DISCARD invalidates the entire buffer, regardless of the specified * offset and size. Some applications also depend on the entire buffer @@ -946,20 +950,13 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc || (!(flags & WINED3D_MAP_WRITE) && (buffer->locations & WINED3D_LOCATION_SYSMEM)) || buffer->flags & WINED3D_BUFFER_PIN_SYSMEM) { - if (!(buffer->locations & WINED3D_LOCATION_SYSMEM)) - { - context = context_acquire(device, NULL, 0); - wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM); - context_release(context); - } + wined3d_buffer_load_location(buffer, context, WINED3D_LOCATION_SYSMEM); if (flags & WINED3D_MAP_WRITE) wined3d_buffer_invalidate_range(buffer, WINED3D_LOCATION_BUFFER, dirty_offset, dirty_size); } else { - context = context_acquire(device, NULL, 0); - if (flags & WINED3D_MAP_DISCARD) wined3d_buffer_validate_location(buffer, WINED3D_LOCATION_BUFFER); else @@ -976,8 +973,6 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc if (count == 1) { - addr.buffer_object = buffer->buffer_object; - addr.addr = 0; buffer->map_ptr = wined3d_context_map_bo_address(context, &addr, resource->size, flags); /* We are accessing buffer->resource.client from the CS thread, * but it's safe because the client thread will wait for the @@ -1009,11 +1004,11 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc TRACE("New pointer is %p.\n", resource->heap_memory); } } - - context_release(context); } } + context_release(context); + base = buffer->map_ptr ? buffer->map_ptr : resource->heap_memory; *map_ptr = base + offset; -- 2.34.1
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- Not tested on real hardware since I don't have easy access to it at the moment. We rely on this in test_dispatch_indirect(), which currently succeeds in Wine only because we initialize the buffer in sysmem with wined3d_resource_alloc_sysmem(). dlls/d3d11/tests/d3d11.c | 9 ++++----- dlls/wined3d/view.c | 7 ++++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c index d1ffbe64f0b..1565d26a060 100644 --- a/dlls/d3d11/tests/d3d11.c +++ b/dlls/d3d11/tests/d3d11.c @@ -16371,10 +16371,8 @@ static void test_clear_buffer_unordered_access_view(void) buffer_desc.Usage = D3D11_USAGE_DEFAULT; buffer_desc.BindFlags = D3D11_BIND_UNORDERED_ACCESS; buffer_desc.CPUAccessFlags = 0; - buffer_desc.MiscFlags = 0; - buffer_desc.StructureByteStride = 0; buffer_desc.MiscFlags = D3D11_RESOURCE_MISC_BUFFER_STRUCTURED; - buffer_desc.StructureByteStride = 4; + buffer_desc.StructureByteStride = 8; hr = ID3D11Device_CreateBuffer(device, &buffer_desc, NULL, &buffer); ok(hr == S_OK, "Failed to create a buffer, hr %#x.\n", hr); @@ -16400,9 +16398,10 @@ static void test_clear_buffer_unordered_access_view(void) ID3D11DeviceContext_ClearUnorderedAccessViewUint(context, uav2, &fe_uvec4.x); get_buffer_readback(buffer, &rb); - SetRect(&rect, 0, 0, U(uav_desc).Buffer.NumElements, 1); + SetRect(&rect, 0, 0, U(uav_desc).Buffer.NumElements * buffer_desc.StructureByteStride / sizeof(uvec4.x), 1); check_readback_data_color(&rb, &rect, fe_uvec4.x, 0); - SetRect(&rect, U(uav_desc).Buffer.NumElements, 0, buffer_desc.ByteWidth / sizeof(uvec4.x), 1); + rect.left = rect.right; + rect.right = buffer_desc.ByteWidth / sizeof(uvec4.x); check_readback_data_color(&rb, &rect, uvec4.x, 0); release_resource_readback(&rb); } diff --git a/dlls/wined3d/view.c b/dlls/wined3d/view.c index efa98ea13eb..3b113a940a3 100644 --- a/dlls/wined3d/view.c +++ b/dlls/wined3d/view.c @@ -1985,11 +1985,16 @@ void wined3d_unordered_access_view_vk_clear(struct wined3d_unordered_access_view if (resource->type == WINED3D_RTYPE_BUFFER) { + struct wined3d_buffer *buffer = buffer_from_resource(resource); + uav_location = WINED3D_LOCATION_BUFFER; layout = state->buffer_layout; vk_writes[0].descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER; - constants.extent.width = view_desc->u.buffer.count; + if (buffer->structure_byte_stride) + constants.extent.width = view_desc->u.buffer.count * buffer->structure_byte_stride / 4; + else + constants.extent.width = view_desc->u.buffer.count; constants.extent.height = 1; } else -- 2.34.1
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=105888 Your paranoid android. === w8 (32 bit report) === d3d11: d3d11.c:6292: Test failed: Got unexpected NumPrimitivesWritten: 4176230912. d3d11.c:6295: Test failed: Got unexpected PrimitivesStorageNeeded: 4294967294. === w1064 (32 bit report) === d3d11: d3d11.c:5795: Test failed: Got unexpected query result 0x0000000000000000. === w10pro64 (32 bit report) === d3d11: d3d11.c:5795: Test failed: Got unexpected query result 0x0000000000000000. === debian11 (32 bit Arabic:Morocco report) === d3d11: d3d11.c:20170: Test failed: Got 0xdeadbeef, expected 0xfefefefe or 0x80808080 at 128, uvec4 0xfefefefe, 0xfefefefe, 0xfefefefe, 0xfefefefe. === debian11 (32 bit French report) === d3d11: d3d11.c:9827: Test failed: d3d11.c:15212: Test marked todo: Got hr 0 for WRITE. d3d11.c:9827: Test failed: d3d11.c:15212: Test marked todo: Got hr 0 for WRITE. === debian11 (32 bit WoW report) === d3d11: d3d11.c:28323: Test succeeded inside todo block: Got 0xffffffff, expected 0xffffffff at (640, 480, 1), sub-resource 0.
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Unless it needs to be pinned. This fixes test_map_synchronisation() on 64-bit machines, broken after 194b47b4fd92dda8ebf24e88ca7a14fc926c84ab. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/wined3d/buffer.c b/dlls/wined3d/buffer.c index e1c1f7aa5fc..e75748a4a0a 100644 --- a/dlls/wined3d/buffer.c +++ b/dlls/wined3d/buffer.c @@ -1233,7 +1233,7 @@ static HRESULT wined3d_buffer_init(struct wined3d_buffer *buffer, struct wined3d } buffer->buffer_ops = buffer_ops; buffer->structure_byte_stride = desc->structure_byte_stride; - buffer->locations = data ? WINED3D_LOCATION_DISCARDED : WINED3D_LOCATION_SYSMEM; + buffer->locations = WINED3D_LOCATION_DISCARDED; TRACE("buffer %p, size %#x, usage %#x, memory @ %p.\n", buffer, buffer->resource.size, buffer->resource.usage, buffer->resource.heap_memory); -- 2.34.1
On Tue, 25 Jan 2022 at 03:35, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
This fixes test_map_synchronisation() on 64-bit machines, broken after 194b47b4fd92dda8ebf24e88ca7a14fc926c84ab.
How is it broken, and why does that happen?
@@ -1233,7 +1233,7 @@ static HRESULT wined3d_buffer_init(struct wined3d_buffer *buffer, struct wined3d } buffer->buffer_ops = buffer_ops; buffer->structure_byte_stride = desc->structure_byte_stride; - buffer->locations = data ? WINED3D_LOCATION_DISCARDED : WINED3D_LOCATION_SYSMEM; + buffer->locations = WINED3D_LOCATION_DISCARDED;
I think avoiding WINED3D_LOCATION_SYSMEM would be great, but I'm not as convinced about using WINED3D_LOCATION_DISCARDED for that purpose. I'm not sure we've ever confirmed that buffer resources should be zeroed after creation, but we have for texture resources; it would not surprise me if this change would break some applications. My preferred way of handling this would be to introduce WINED3D_LOCATION_CLEARED, which would effectively defer clearing the resource until loading it into another location. When replacing the entire (sub-)resource with e.g. a DISCARD map or sub-resource update, we can then just skip that clear. This approach would also allow us to avoid zeroing texture resources on creation, an in case of the Vulkan renderer, we could even extend that a bit further and merge render-target clears with starting render passes by using VK_ATTACHMENT_LOAD_OP_CLEAR and appropriate clear values.
On 1/26/22 07:47, Henri Verbeet wrote:
On Tue, 25 Jan 2022 at 03:35, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
This fixes test_map_synchronisation() on 64-bit machines, broken after 194b47b4fd92dda8ebf24e88ca7a14fc926c84ab.
How is it broken, and why does that happen?
The test creates a buffer, maps it once, then maps it again with NOOVERWRITE while the GPU is still drawing, expecting the new data to be read by the GPU during the draw. On 32-bit machines, and 64-bit machines before 194b47b4fd9, we do the following: First map: uses SYSMEM since the BO is not created yet Draw: upload to VBO Second map: map the existing VBO with GL_MAP_UNSYNCHRONIZED_BIT After 194b47b4fd9, we don't use GL_MAP_UNSYNCHRONIZED_BIT since the buffer has READ access, which means that the second map will be synchronized and wait for the draw to complete. After this patch, we do the following: First map: create and map a VBO (not unsynchronized, but coherent and persistently mapped) Draw: use mapped VBO Second map: write to existing (coherent) VBO, which is unsynchronized I'll make sure to include this explanation in the patch next revision. Of course, although this motivated the patch, it seems like a good idea regardless. In most cases we don't want SYSMEM for buffer objects, especially for newer d3d versions, and we should avoid the overhead of allocating it and keeping it around. It may also be that we want to translate D3DVBCAPS_WRITEONLY into ~WINED3D_RESOURCE_ACCESS_MAP_R. My limited understanding is that this doesn't contradict test_vb_writeonly() per se, but rather would only be a bad idea performance-wise if ddraw applications rely on reading from such mapped buffers in critical paths. I don't know whether that's the case.
@@ -1233,7 +1233,7 @@ static HRESULT wined3d_buffer_init(struct wined3d_buffer *buffer, struct wined3d } buffer->buffer_ops = buffer_ops; buffer->structure_byte_stride = desc->structure_byte_stride; - buffer->locations = data ? WINED3D_LOCATION_DISCARDED : WINED3D_LOCATION_SYSMEM; + buffer->locations = WINED3D_LOCATION_DISCARDED;
I think avoiding WINED3D_LOCATION_SYSMEM would be great, but I'm not as convinced about using WINED3D_LOCATION_DISCARDED for that purpose. I'm not sure we've ever confirmed that buffer resources should be zeroed after creation, but we have for texture resources; it would not surprise me if this change would break some applications.
My preferred way of handling this would be to introduce WINED3D_LOCATION_CLEARED, which would effectively defer clearing the resource until loading it into another location. When replacing the entire (sub-)resource with e.g. a DISCARD map or sub-resource update, we can then just skip that clear. This approach would also allow us to avoid zeroing texture resources on creation, an in case of the Vulkan renderer, we could even extend that a bit further and merge render-target clears with starting render passes by using VK_ATTACHMENT_LOAD_OP_CLEAR and appropriate clear values.
I had in fact had that same idea :-) I left off using it for buffers because it wasn't clear to me that it was necessary. Thus far we had only found evidence that it was necessary for textures (and perhaps just swapchain textures?) Unfortunately I don't currently have access to any Windows machines (especially older machines), so I can't verify that buffers (or textures, for that matter) are implicitly cleared on initialization.
On Wed, 26 Jan 2022 at 17:45, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
Of course, although this motivated the patch, it seems like a good idea regardless. In most cases we don't want SYSMEM for buffer objects, especially for newer d3d versions, and we should avoid the overhead of allocating it and keeping it around.
Yes.
It may also be that we want to translate D3DVBCAPS_WRITEONLY into ~WINED3D_RESOURCE_ACCESS_MAP_R. My limited understanding is that this doesn't contradict test_vb_writeonly() per se, but rather would only be a bad idea performance-wise if ddraw applications rely on reading from such mapped buffers in critical paths. I don't know whether that's the case.
Right, as far as I understand D3DVBCAPS_WRITEONLY, like D3DUSAGE_WRITEONLY in later D3D versions, mostly just affects caching of the mapped memory, somewhat like VK_MEMORY_PROPERTY_HOST_CACHED_BIT in Vulkan.
I left off using it for buffers because it wasn't clear to me that it was necessary. Thus far we had only found evidence that it was necessary for textures (and perhaps just swapchain textures?)
I think these were non-swapchain textures in the application that originally motivated clearing textures on creation.
On Tue, 25 Jan 2022 at 03:21, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
@@ -927,10 +928,13 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc
count = ++resource->map_count;
- if (buffer->buffer_object) + context = context_acquire(device, NULL, 0); + + wined3d_buffer_get_memory(buffer, context, &addr); + + if (addr.buffer_object) { I think that potentially changes the location we map in some cases. (In particular, when buffer->buffer_object is non-NULL, but WINED3D_LOCATION_BUFFER is not current.) I don't know for sure whether that would cause issues for applications, but this code is sensitive enough that I'd prefer avoiding wined3d_buffer_get_memory() here unless we have a good reason not to.
What is the issue exactly? Is it that buffer->buffer_object may not have been created yet for discarded resources?
On 1/26/22 07:47, Henri Verbeet wrote:
On Tue, 25 Jan 2022 at 03:21, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
@@ -927,10 +928,13 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc
count = ++resource->map_count;
- if (buffer->buffer_object) + context = context_acquire(device, NULL, 0); + + wined3d_buffer_get_memory(buffer, context, &addr); + + if (addr.buffer_object) { I think that potentially changes the location we map in some cases. (In particular, when buffer->buffer_object is non-NULL, but WINED3D_LOCATION_BUFFER is not current.) I don't know for sure whether that would cause issues for applications, but this code is sensitive enough that I'd prefer avoiding wined3d_buffer_get_memory() here unless we have a good reason not to.
I guess I see the point here—if we are using a BO in the first place, we probably want to actually use the BO instead of reusing sysmem. Note though that this doesn't seem to preclude wined3d_buffer_get_memory(); it just means we want to keep the check for buffer->buffer_object instead of addr.buffer_object.
What is the issue exactly? Is it that buffer->buffer_object may not have been created yet for discarded resources?
More generally, no location may yet have been prepared. In particular after patch 3/3 no location will be prepared on creation.
On Wed, 26 Jan 2022 at 17:17, Zebediah Figura (she/her) <zfigura(a)codeweavers.com> wrote:
On 1/26/22 07:47, Henri Verbeet wrote:
On Tue, 25 Jan 2022 at 03:21, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
@@ -927,10 +928,13 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc
count = ++resource->map_count;
- if (buffer->buffer_object) + context = context_acquire(device, NULL, 0); + + wined3d_buffer_get_memory(buffer, context, &addr); + + if (addr.buffer_object) { I think that potentially changes the location we map in some cases. (In particular, when buffer->buffer_object is non-NULL, but WINED3D_LOCATION_BUFFER is not current.) I don't know for sure whether that would cause issues for applications, but this code is sensitive enough that I'd prefer avoiding wined3d_buffer_get_memory() here unless we have a good reason not to.
I guess I see the point here—if we are using a BO in the first place, we probably want to actually use the BO instead of reusing sysmem.
Note though that this doesn't seem to preclude wined3d_buffer_get_memory(); it just means we want to keep the check for buffer->buffer_object instead of addr.buffer_object.
I suppose, but I'm not sure there's much of a point in that case. Conceptually, wined3d_buffer_get_memory() is for cases where we don't particularly care which location is current.
What is the issue exactly? Is it that buffer->buffer_object may not have been created yet for discarded resources?
More generally, no location may yet have been prepared. In particular after patch 3/3 no location will be prepared on creation.
It probably makes sense to check for WINED3D_BUFFER_USE_BO instead of buffer->buffer_object then, and add wined3d_buffer_prepare_location() calls to paths that don't already call wined3d_buffer_load_location().
On 1/27/22 07:13, Henri Verbeet wrote:
What is the issue exactly? Is it that buffer->buffer_object may not have been created yet for discarded resources?
More generally, no location may yet have been prepared. In particular after patch 3/3 no location will be prepared on creation.
It probably makes sense to check for WINED3D_BUFFER_USE_BO instead of buffer->buffer_object then, and add wined3d_buffer_prepare_location() calls to paths that don't already call wined3d_buffer_load_location().
Sure, that makes sense.
participants (5)
-
Henri Verbeet -
Henri Verbeet -
Marvin -
Zebediah Figura -
Zebediah Figura (she/her)