[PATCH 1/3] wined3d: Return the map pitch from wined3d_device_context_emit_map().
Partially undoes 61024aa12f. We want to be able to allocate a new upload BO for a given map, but it's a bit structurally awkward to ask for a specific memory layout when parepare_upload_bo() might return different ones depending on the flags and other factors. Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- This is kind of janky, though. It might not be all that unreasonable to ask prepare_upload_bo() for a specific layout. dlls/wined3d/cs.c | 21 ++++++++++++++++----- dlls/wined3d/device.c | 7 +------ dlls/wined3d/wined3d_private.h | 2 +- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/dlls/wined3d/cs.c b/dlls/wined3d/cs.c index a34aad060f9..cbe333fab95 100644 --- a/dlls/wined3d/cs.c +++ b/dlls/wined3d/cs.c @@ -2483,20 +2483,25 @@ static void wined3d_cs_exec_map(struct wined3d_cs *cs, const void *data) op->sub_resource_idx, op->map_ptr, op->box, op->flags); } -HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, struct wined3d_resource *resource, - unsigned int sub_resource_idx, void **map_ptr, const struct wined3d_box *box, unsigned int flags) +HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, + struct wined3d_resource *resource, unsigned int sub_resource_idx, + struct wined3d_map_desc *map_desc, const struct wined3d_box *box, unsigned int flags) { unsigned int row_pitch, slice_pitch; struct wined3d_cs_map *op; + void *map_ptr; HRESULT hr; wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) - && (*map_ptr = context->ops->map_upload_bo(context, resource, + && (map_ptr = context->ops->map_upload_bo(context, resource, sub_resource_idx, box, row_pitch, slice_pitch, flags))) { - TRACE("Returning map pointer %p, row pitch %u, slice pitch %u.\n", *map_ptr, row_pitch, slice_pitch); + TRACE("Returning map pointer %p, row pitch %u, slice pitch %u.\n", map_ptr, row_pitch, slice_pitch); + map_desc->data = map_ptr; + map_desc->row_pitch = row_pitch; + map_desc->slice_pitch = slice_pitch; return WINED3D_OK; } @@ -2511,7 +2516,7 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, op->opcode = WINED3D_CS_OP_MAP; op->resource = resource; op->sub_resource_idx = sub_resource_idx; - op->map_ptr = map_ptr; + op->map_ptr = &map_ptr; op->box = box; op->flags = flags; op->hr = &hr; @@ -2519,6 +2524,12 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, wined3d_device_context_submit(context, WINED3D_CS_QUEUE_MAP); wined3d_device_context_finish(context, WINED3D_CS_QUEUE_MAP); + if (SUCCEEDED(hr)) + { + map_desc->data = map_ptr; + map_desc->row_pitch = row_pitch; + map_desc->slice_pitch = slice_pitch; + } return hr; } diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index 71b1e3c3f88..8e84544e6af 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -4841,7 +4841,6 @@ HRESULT CDECL wined3d_device_context_map(struct wined3d_device_context *context, { struct wined3d_sub_resource_desc desc; struct wined3d_box b; - HRESULT hr; TRACE("context %p, resource %p, sub_resource_idx %u, map_desc %p, box %s, flags %#x.\n", context, resource, sub_resource_idx, map_desc, debug_box(box), flags); @@ -4885,11 +4884,7 @@ HRESULT CDECL wined3d_device_context_map(struct wined3d_device_context *context, return WINED3DERR_INVALIDCALL; } - if (SUCCEEDED(hr = wined3d_device_context_emit_map(context, resource, - sub_resource_idx, &map_desc->data, box, flags))) - wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, - &map_desc->row_pitch, &map_desc->slice_pitch); - return hr; + return wined3d_device_context_emit_map(context, resource, sub_resource_idx, map_desc, box, flags); } HRESULT CDECL wined3d_device_context_unmap(struct wined3d_device_context *context, diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 0c169ec344b..acd3ded7a24 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -4868,7 +4868,7 @@ void wined3d_device_context_emit_execute_command_list(struct wined3d_device_cont void wined3d_device_context_emit_generate_mipmaps(struct wined3d_device_context *context, struct wined3d_shader_resource_view *view) DECLSPEC_HIDDEN; HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, - struct wined3d_resource *resource, unsigned int sub_resource_idx, void **map_ptr, + struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_map_desc *map_desc, const struct wined3d_box *box, unsigned int flags) DECLSPEC_HIDDEN; void wined3d_device_context_emit_reset_state(struct wined3d_device_context *context, bool invalidate) DECLSPEC_HIDDEN; void wined3d_device_context_emit_set_blend_state(struct wined3d_device_context *context, -- 2.33.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/wined3d/cs.c | 33 ++++++++++++++++++--------------- dlls/wined3d/wined3d_private.h | 4 ++-- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/dlls/wined3d/cs.c b/dlls/wined3d/cs.c index cbe333fab95..c35e85c3cc2 100644 --- a/dlls/wined3d/cs.c +++ b/dlls/wined3d/cs.c @@ -2492,11 +2492,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, void *map_ptr; HRESULT hr; - wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) && (map_ptr = context->ops->map_upload_bo(context, resource, - sub_resource_idx, box, row_pitch, slice_pitch, flags))) + sub_resource_idx, box, &row_pitch, &slice_pitch, flags))) { TRACE("Returning map pointer %p, row pitch %u, slice pitch %u.\n", map_ptr, row_pitch, slice_pitch); map_desc->data = map_ptr; @@ -2505,6 +2503,8 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, return WINED3D_OK; } + wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); + /* Mapping resources from the worker thread isn't an issue by itself, but * increasing the map count would be visible to applications. */ wined3d_not_from_cs(context->device->cs); @@ -2762,20 +2762,21 @@ done: void wined3d_device_context_emit_update_sub_resource(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, const struct wined3d_box *box, - const void *data, unsigned int row_pitch, unsigned int slice_pitch) + const void *data, unsigned int src_row_pitch, unsigned int src_slice_pitch) { + unsigned int dst_row_pitch, dst_slice_pitch; struct wined3d_cs_update_sub_resource *op; struct wined3d_box dummy_box; struct upload_bo bo; void *map_ptr; if ((map_ptr = context->ops->map_upload_bo(context, resource, sub_resource_idx, box, - row_pitch, slice_pitch, WINED3D_MAP_WRITE))) + &dst_row_pitch, &dst_slice_pitch, WINED3D_MAP_WRITE))) { - wined3d_format_copy_data(resource->format, data, row_pitch, slice_pitch, map_ptr, row_pitch, slice_pitch, - box->right - box->left, box->bottom - box->top, box->back - box->front); + wined3d_format_copy_data(resource->format, data, src_row_pitch, src_slice_pitch, map_ptr, dst_row_pitch, + dst_slice_pitch, box->right - box->left, box->bottom - box->top, box->back - box->front); context->ops->unmap_upload_bo(context, resource, sub_resource_idx, &dummy_box, &bo); - wined3d_device_context_upload_bo(context, resource, sub_resource_idx, box, &bo, row_pitch, slice_pitch); + wined3d_device_context_upload_bo(context, resource, sub_resource_idx, box, &bo, dst_row_pitch, dst_slice_pitch); return; } @@ -2789,8 +2790,8 @@ void wined3d_device_context_emit_update_sub_resource(struct wined3d_device_conte op->bo.addr.buffer_object = 0; op->bo.addr.addr = data; op->bo.flags = 0; - op->row_pitch = row_pitch; - op->slice_pitch = slice_pitch; + op->row_pitch = src_row_pitch; + op->slice_pitch = src_slice_pitch; wined3d_device_context_acquire_resource(context, resource); @@ -3090,8 +3091,8 @@ static void wined3d_cs_st_finish(struct wined3d_device_context *context, enum wi } static void *wined3d_cs_map_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, - unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int row_pitch, - unsigned int slice_pitch, uint32_t flags) + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags) { /* FIXME: We would like to return mapped or newly allocated memory here. */ return NULL; @@ -3536,7 +3537,7 @@ static struct wined3d_deferred_upload *deferred_context_get_upload(struct wined3 static void *wined3d_deferred_context_map_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, const struct wined3d_box *box, - unsigned int row_pitch, unsigned int slice_pitch, uint32_t flags) + unsigned int *row_pitch, unsigned int *slice_pitch, uint32_t flags) { struct wined3d_deferred_context *deferred = wined3d_deferred_context_from_context(context); const struct wined3d_format *format = resource->format; @@ -3544,8 +3545,10 @@ static void *wined3d_deferred_context_map_upload_bo(struct wined3d_device_contex uint8_t *sysmem; size_t size; - size = (box->back - box->front - 1) * slice_pitch - + ((box->bottom - box->top - 1) / format->block_height) * row_pitch + wined3d_format_calculate_pitch(format, 1, box->right - box->left, box->bottom - box->top, row_pitch, slice_pitch); + + size = (box->back - box->front - 1) * (*slice_pitch) + + ((box->bottom - box->top - 1) / format->block_height) * (*row_pitch) + ((box->right - box->left + format->block_width - 1) / format->block_width) * format->block_byte_count; if (!(flags & WINED3D_MAP_WRITE)) diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index acd3ded7a24..87d69b7b6fe 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -4778,8 +4778,8 @@ struct wined3d_device_context_ops void (*push_constants)(struct wined3d_device_context *context, enum wined3d_push_constants p, unsigned int start_idx, unsigned int count, const void *constants); void *(*map_upload_bo)(struct wined3d_device_context *context, struct wined3d_resource *resource, - unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int row_pitch, - unsigned int slice_pitch, uint32_t flags); + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags); bool (*unmap_upload_bo)(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_box *box, struct upload_bo *upload_bo); void (*issue_query)(struct wined3d_device_context *context, struct wined3d_query *query, unsigned int flags); -- 2.33.0
On Thu, 7 Oct 2021 at 04:22, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
--- dlls/wined3d/cs.c | 33 ++++++++++++++++++--------------- dlls/wined3d/wined3d_private.h | 4 ++-- 2 files changed, 20 insertions(+), 17 deletions(-)
Like in 1/3, prepare_upload_bo() has been renamed to map_upload_bo().
@@ -2492,11 +2492,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, void *map_ptr; HRESULT hr;
- wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) && (map_ptr = context->ops->map_upload_bo(context, resource, - sub_resource_idx, box, row_pitch, slice_pitch, flags))) + sub_resource_idx, box, &row_pitch, &slice_pitch, flags))) { Would it then make sense to pass a wined3d_map_desc pointer to map_upload_bo()?
@@ -2505,6 +2503,8 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, return WINED3D_OK; }
+ wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); + It's mostly inconsequential, but note that we only need to call this on success. Also, I suppose we might as well pass "&map_desc->row_pitch" and "&map_desc->slice_pitch" here.
On 10/8/21 11:23 AM, Henri Verbeet wrote:
On Thu, 7 Oct 2021 at 04:22, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
--- dlls/wined3d/cs.c | 33 ++++++++++++++++++--------------- dlls/wined3d/wined3d_private.h | 4 ++-- 2 files changed, 20 insertions(+), 17 deletions(-)
Like in 1/3, prepare_upload_bo() has been renamed to map_upload_bo().
@@ -2492,11 +2492,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, void *map_ptr; HRESULT hr;
- wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if ((flags & (WINED3D_MAP_DISCARD | WINED3D_MAP_NOOVERWRITE)) && (map_ptr = context->ops->map_upload_bo(context, resource, - sub_resource_idx, box, row_pitch, slice_pitch, flags))) + sub_resource_idx, box, &row_pitch, &slice_pitch, flags))) { Would it then make sense to pass a wined3d_map_desc pointer to map_upload_bo()?
Sure, that seems reasonable.
@@ -2505,6 +2503,8 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, return WINED3D_OK; }
+ wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); + It's mostly inconsequential, but note that we only need to call this on success. Also, I suppose we might as well pass "&map_desc->row_pitch" and "&map_desc->slice_pitch" here.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- And I'm sure there's a lot to complain about here ;-) dlls/wined3d/adapter_gl.c | 15 ++++++ dlls/wined3d/adapter_vk.c | 68 ++++++++++++++++++++++++++ dlls/wined3d/buffer.c | 10 ++++ dlls/wined3d/cs.c | 88 +++++++++++++++++++++++++++++++--- dlls/wined3d/directx.c | 14 ++++++ dlls/wined3d/texture.c | 9 ++++ dlls/wined3d/wined3d_private.h | 32 +++++++++++++ 7 files changed, 230 insertions(+), 6 deletions(-) diff --git a/dlls/wined3d/adapter_gl.c b/dlls/wined3d/adapter_gl.c index bba728e2fb5..228e64b5a56 100644 --- a/dlls/wined3d/adapter_gl.c +++ b/dlls/wined3d/adapter_gl.c @@ -4597,6 +4597,19 @@ static void adapter_gl_copy_bo_address(struct wined3d_context *context, wined3d_context_gl_copy_bo_address(wined3d_context_gl(context), dst, src, size); } +static void adapter_gl_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ + FIXME("context %p, data %s, size %zu, stub!\n", context, debug_const_bo_address(data), size); +} + +static void *adapter_gl_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo) +{ + return NULL; +} + static HRESULT adapter_gl_create_swapchain(struct wined3d_device *device, struct wined3d_swapchain_desc *desc, struct wined3d_swapchain_state_parent *state_parent, void *parent, const struct wined3d_parent_ops *parent_ops, struct wined3d_swapchain **swapchain) @@ -5049,6 +5062,8 @@ static const struct wined3d_adapter_ops wined3d_adapter_gl_ops = .adapter_map_bo_address = adapter_gl_map_bo_address, .adapter_unmap_bo_address = adapter_gl_unmap_bo_address, .adapter_copy_bo_address = adapter_gl_copy_bo_address, + .adapter_flush_bo_address = adapter_gl_flush_bo_address, + .adapter_alloc_upload_bo = adapter_gl_alloc_upload_bo, .adapter_create_swapchain = adapter_gl_create_swapchain, .adapter_destroy_swapchain = adapter_gl_destroy_swapchain, .adapter_create_buffer = adapter_gl_create_buffer, diff --git a/dlls/wined3d/adapter_vk.c b/dlls/wined3d/adapter_vk.c index 91a6b92b4aa..9596bef3cd5 100644 --- a/dlls/wined3d/adapter_vk.c +++ b/dlls/wined3d/adapter_vk.c @@ -1169,6 +1169,72 @@ void adapter_vk_copy_bo_address(struct wined3d_context *context, adapter_vk_unmap_bo_address(context, src, 0, NULL); } +static void adapter_vk_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ + struct wined3d_context_vk *context_vk = wined3d_context_vk(context); + const struct wined3d_vk_info *vk_info; + struct wined3d_device_vk *device_vk; + VkMappedMemoryRange range; + struct wined3d_bo_vk *bo; + + if (!(bo = (struct wined3d_bo_vk *)data->buffer_object)) + return; + + vk_info = context_vk->vk_info; + device_vk = wined3d_device_vk(context->device); + + range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE; + range.pNext = NULL; + range.memory = bo->vk_memory; + range.offset = bo->memory_offset + (uintptr_t)data->addr; + range.size = size; + VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range)); +} + +static void *adapter_vk_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo) +{ + wined3d_not_from_cs(device->cs); + + /* Limit NOOVERWRITE maps to buffers for now; there are too many ways that + * a texture can be invalidated to even count. */ + if (wined3d_map_persistent() && resource->type == WINED3D_RTYPE_BUFFER && (flags & WINED3D_MAP_NOOVERWRITE)) + { + struct wined3d_client_sub_resource *sub_resource; + const struct wined3d_bo_vk *bo_vk; + uint8_t *map_ptr; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + + bo_vk = (const struct wined3d_bo_vk *)sub_resource->addr.buffer_object; + map_ptr = bo_vk ? bo_vk->map_ptr : NULL; + map_ptr += (uintptr_t)sub_resource->addr.addr; + + if (!map_ptr) + { + TRACE("Sub-resource is not persistently mapped.\n"); + return NULL; + } + + wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, row_pitch, slice_pitch); + + upload_bo->addr = *wined3d_const_bo_address(&sub_resource->addr); + upload_bo->flags = 0; + if (bo_vk) + map_ptr += bo_vk->memory_offset; + map_ptr = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box); + + if (!(bo_vk->memory_type & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) + upload_bo->flags |= UPLOAD_BO_FLUSH_ON_UNMAP; + + return map_ptr; + } + + return NULL; +} + static HRESULT adapter_vk_create_swapchain(struct wined3d_device *device, struct wined3d_swapchain_desc *desc, struct wined3d_swapchain_state_parent *state_parent, void *parent, const struct wined3d_parent_ops *parent_ops, struct wined3d_swapchain **swapchain) @@ -1820,6 +1886,8 @@ static const struct wined3d_adapter_ops wined3d_adapter_vk_ops = .adapter_map_bo_address = adapter_vk_map_bo_address, .adapter_unmap_bo_address = adapter_vk_unmap_bo_address, .adapter_copy_bo_address = adapter_vk_copy_bo_address, + .adapter_flush_bo_address = adapter_vk_flush_bo_address, + .adapter_alloc_upload_bo = adapter_vk_alloc_upload_bo, .adapter_create_swapchain = adapter_vk_create_swapchain, .adapter_destroy_swapchain = adapter_vk_destroy_swapchain, .adapter_create_buffer = adapter_vk_create_buffer, diff --git a/dlls/wined3d/buffer.c b/dlls/wined3d/buffer.c index 3ea3bf4b180..c090a24ceb6 100644 --- a/dlls/wined3d/buffer.c +++ b/dlls/wined3d/buffer.c @@ -840,6 +840,14 @@ struct wined3d_resource * CDECL wined3d_buffer_get_resource(struct wined3d_buffe return &buffer->resource; } +static struct wined3d_client_sub_resource *buffer_resource_get_client_sub_resource(struct wined3d_resource *resource, + unsigned int sub_resource_idx) +{ + struct wined3d_buffer *buffer = buffer_from_resource(resource); + + return &buffer->client; +} + static HRESULT buffer_resource_sub_resource_get_desc(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_sub_resource_desc *desc) { @@ -938,6 +946,7 @@ static HRESULT buffer_resource_sub_resource_map(struct wined3d_resource *resourc addr.buffer_object = buffer->buffer_object; addr.addr = 0; buffer->map_ptr = wined3d_context_map_bo_address(context, &addr, resource->size, flags); + buffer->client.addr = addr; if (((DWORD_PTR)buffer->map_ptr) & (RESOURCE_ALIGNMENT - 1)) { @@ -1099,6 +1108,7 @@ static const struct wined3d_resource_ops buffer_resource_ops = buffer_resource_decref, buffer_resource_preload, buffer_resource_unload, + buffer_resource_get_client_sub_resource, buffer_resource_sub_resource_get_desc, buffer_resource_sub_resource_get_map_pitch, buffer_resource_sub_resource_map, diff --git a/dlls/wined3d/cs.c b/dlls/wined3d/cs.c index c35e85c3cc2..3e83b78999e 100644 --- a/dlls/wined3d/cs.c +++ b/dlls/wined3d/cs.c @@ -116,6 +116,14 @@ ULONG CDECL wined3d_command_list_decref(struct wined3d_command_list *list) return refcount; } +static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + memset(&sub_resource->addr, 0, sizeof(sub_resource->addr)); +} + enum wined3d_cs_op { WINED3D_CS_OP_NOP, @@ -2404,6 +2412,13 @@ static void wined3d_cs_acquire_command_list(struct wined3d_device_context *conte for (i = 0; i < list->command_list_count; ++i) wined3d_cs_acquire_command_list(context, list->command_lists[i]); + + for (i = 0; i < list->upload_count; ++i) + { + const struct wined3d_deferred_upload *upload = &list->uploads[i]; + + invalidate_persistent_map(upload->resource, upload->sub_resource_idx); + } } static void wined3d_cs_exec_preload_resource(struct wined3d_cs *cs, const void *data) @@ -2441,6 +2456,19 @@ void wined3d_cs_emit_unload_resource(struct wined3d_cs *cs, struct wined3d_resou { struct wined3d_cs_unload_resource *op; + if (resource->type == WINED3D_RTYPE_BUFFER) + { + invalidate_persistent_map(resource, 0); + } + else + { + struct wined3d_texture *texture = wined3d_texture_from_resource(resource); + unsigned int i; + + for (i = 0; i < texture->layer_count * texture->level_count; ++i) + invalidate_persistent_map(resource, i); + } + op = wined3d_device_context_require_space(&cs->c, sizeof(*op), WINED3D_CS_QUEUE_DEFAULT); op->opcode = WINED3D_CS_OP_UNLOAD_RESOURCE; op->resource = resource; @@ -2511,6 +2539,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, wined3d_resource_wait_idle(resource); + /* We might end up loading the resource into sysmem. */ + invalidate_persistent_map(resource, sub_resource_idx); + if (!(op = wined3d_device_context_require_space(context, sizeof(*op), WINED3D_CS_QUEUE_MAP))) return E_OUTOFMEMORY; op->opcode = WINED3D_CS_OP_MAP; @@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch; wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP) + if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK; } @@ -2581,8 +2612,7 @@ static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void * if (op->dst_resource->type == WINED3D_RTYPE_BUFFER) { wined3d_buffer_copy(buffer_from_resource(op->dst_resource), op->dst_box.left, - buffer_from_resource(op->src_resource), op->src_box.left, - op->src_box.right - op->src_box.left); + buffer_from_resource(op->src_resource), op->src_box.left, op->src_box.right - op->src_box.left); } else if (op->dst_resource->type == WINED3D_RTYPE_TEXTURE_3D) { @@ -2688,6 +2718,10 @@ void wined3d_device_context_emit_blt_sub_resource(struct wined3d_device_context { struct wined3d_cs_blt_sub_resource *op; + /* This might result in an implicit discard. */ + if (dst_resource->type == WINED3D_RTYPE_BUFFER && dst_box->right - dst_box->left == dst_resource->size) + invalidate_persistent_map(dst_resource, 0); + op = wined3d_device_context_require_space(context, sizeof(*op), WINED3D_CS_QUEUE_DEFAULT); op->opcode = WINED3D_CS_OP_BLT_SUB_RESOURCE; op->dst_resource = dst_resource; @@ -2727,8 +2761,22 @@ static void wined3d_cs_exec_update_sub_resource(struct wined3d_cs *cs, const voi if (resource->type == WINED3D_RTYPE_BUFFER) { struct wined3d_buffer *buffer = buffer_from_resource(resource); + size_t size = box->right - box->left; - wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, box->right - box->left); + if (op->bo.flags & UPLOAD_BO_FLUSH_ON_UNMAP) + { + wined3d_context_flush_bo_address(context, &op->bo.addr, size); + } + else + { + wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, size); + } + goto done; + } + + if (op->bo.flags & UPLOAD_BO_FLUSH_ON_UNMAP) + { + wined3d_context_flush_bo_address(context, &op->bo.addr, resource->size); goto done; } @@ -2770,6 +2818,10 @@ void wined3d_device_context_emit_update_sub_resource(struct wined3d_device_conte struct upload_bo bo; void *map_ptr; + /* This might result in an implicit discard. */ + if (resource->type == WINED3D_RTYPE_BUFFER && box->right - box->left == resource->size) + invalidate_persistent_map(resource, 0); + if ((map_ptr = context->ops->map_upload_bo(context, resource, sub_resource_idx, box, &dst_row_pitch, &dst_slice_pitch, WINED3D_MAP_WRITE))) { @@ -3094,13 +3146,37 @@ static void *wined3d_cs_map_upload_bo(struct wined3d_device_context *context, st unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, unsigned int *slice_pitch, uint32_t flags) { - /* FIXME: We would like to return mapped or newly allocated memory here. */ - return NULL; + struct wined3d_client_sub_resource *sub_resource; + struct upload_bo bo; + void *map_ptr; + + if ((map_ptr = context->device->adapter->adapter_ops->adapter_alloc_upload_bo(context->device, + resource, sub_resource_idx, box, row_pitch, slice_pitch, flags, &bo))) + { + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + sub_resource->upload_bo = bo; + sub_resource->upload_box = *box; + TRACE("Returning map pointer %p, bo %s, flags %#x, row pitch %u, slice pitch %u.\n", map_ptr, + debug_const_bo_address(&bo.addr), bo.flags, *row_pitch, *slice_pitch); + } + return map_ptr; } static bool wined3d_cs_unmap_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_box *box, struct upload_bo *bo) { + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + if (sub_resource->upload_bo.addr.buffer_object || sub_resource->upload_bo.addr.addr) + { + *bo = sub_resource->upload_bo; + *box = sub_resource->upload_box; + memset(&sub_resource->upload_bo, 0, sizeof(sub_resource->upload_bo)); + memset(&sub_resource->upload_box, 0, sizeof(sub_resource->upload_box)); + return true; + } + return false; } diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c index 990164fd68d..884eec4ac92 100644 --- a/dlls/wined3d/directx.c +++ b/dlls/wined3d/directx.c @@ -2798,6 +2798,18 @@ static void adapter_no3d_copy_bo_address(struct wined3d_context *context, memcpy(dst->addr, src->addr, size); } +static void adapter_no3d_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ +} + +static void *adapter_no3d_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo) +{ + return NULL; +} + static HRESULT adapter_no3d_create_swapchain(struct wined3d_device *device, struct wined3d_swapchain_desc *desc, struct wined3d_swapchain_state_parent *state_parent, void *parent, const struct wined3d_parent_ops *parent_ops, struct wined3d_swapchain **swapchain) @@ -3067,6 +3079,8 @@ static const struct wined3d_adapter_ops wined3d_adapter_no3d_ops = .adapter_map_bo_address = adapter_no3d_map_bo_address, .adapter_unmap_bo_address = adapter_no3d_unmap_bo_address, .adapter_copy_bo_address = adapter_no3d_copy_bo_address, + .adapter_flush_bo_address = adapter_no3d_flush_bo_address, + .adapter_alloc_upload_bo = adapter_no3d_alloc_upload_bo, .adapter_create_swapchain = adapter_no3d_create_swapchain, .adapter_destroy_swapchain = adapter_no3d_destroy_swapchain, .adapter_create_buffer = adapter_no3d_create_buffer, diff --git a/dlls/wined3d/texture.c b/dlls/wined3d/texture.c index 339bf4e205a..d96492fc9e9 100644 --- a/dlls/wined3d/texture.c +++ b/dlls/wined3d/texture.c @@ -3438,6 +3438,14 @@ static void texture_resource_unload(struct wined3d_resource *resource) resource_unload(&texture->resource); } +static struct wined3d_client_sub_resource *texture_resource_get_client_sub_resource(struct wined3d_resource *resource, + unsigned int sub_resource_idx) +{ + const struct wined3d_texture *texture = texture_from_resource(resource); + + return &texture->sub_resources[sub_resource_idx].client; +} + static HRESULT texture_resource_sub_resource_get_desc(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_sub_resource_desc *desc) { @@ -3603,6 +3611,7 @@ static const struct wined3d_resource_ops texture_resource_ops = texture_resource_decref, texture_resource_preload, texture_resource_unload, + texture_resource_get_client_sub_resource, texture_resource_sub_resource_get_desc, texture_resource_sub_resource_get_map_pitch, texture_resource_sub_resource_map, diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 87d69b7b6fe..216bffabc90 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -3329,6 +3329,7 @@ bool wined3d_driver_info_init(struct wined3d_driver_info *driver_info, UINT64 vram_bytes, UINT64 sysmem_bytes) DECLSPEC_HIDDEN; #define UPLOAD_BO_UPLOAD_ON_UNMAP 0x1 +#define UPLOAD_BO_FLUSH_ON_UNMAP 0x2 struct upload_bo { @@ -3359,6 +3360,11 @@ struct wined3d_adapter_ops unsigned int range_count, const struct wined3d_range *ranges); void (*adapter_copy_bo_address)(struct wined3d_context *context, const struct wined3d_bo_address *dst, const struct wined3d_bo_address *src, size_t size); + void (*adapter_flush_bo_address)(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size); + void *(*adapter_alloc_upload_bo)(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo); HRESULT (*adapter_create_swapchain)(struct wined3d_device *device, struct wined3d_swapchain_desc *desc, struct wined3d_swapchain_state_parent *state_parent, void *parent, @@ -4107,6 +4113,14 @@ const char *wined3d_debug_view_desc(const struct wined3d_view_desc *d, const struct wined3d_resource *resource) DECLSPEC_HIDDEN; const char *wined3d_debug_vkresult(VkResult vr) DECLSPEC_HIDDEN; +struct wined3d_client_sub_resource +{ + struct wined3d_bo_address addr; + + struct upload_bo upload_bo; + struct wined3d_box upload_box; +}; + static inline BOOL wined3d_resource_access_is_managed(unsigned int access) { return !(~access & (WINED3D_RESOURCE_ACCESS_GPU | WINED3D_RESOURCE_ACCESS_CPU)); @@ -4118,6 +4132,8 @@ struct wined3d_resource_ops ULONG (*resource_decref)(struct wined3d_resource *resource); void (*resource_preload)(struct wined3d_resource *resource); void (*resource_unload)(struct wined3d_resource *resource); + struct wined3d_client_sub_resource *(*resource_get_client_sub_resource)(struct wined3d_resource *resource, + unsigned int sub_resource_idx); HRESULT (*resource_sub_resource_get_desc)(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_sub_resource_desc *desc); void (*resource_sub_resource_get_map_pitch)(struct wined3d_resource *resource, @@ -4183,6 +4199,12 @@ static inline void wined3d_resource_release(struct wined3d_resource *resource) assert(refcount >= 0); } +static inline struct wined3d_client_sub_resource *wined3d_resource_get_client_sub_resource( + struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + return resource->resource_ops->resource_get_client_sub_resource(resource, sub_resource_idx); +} + static inline HRESULT wined3d_resource_get_sub_resource_desc(struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_sub_resource_desc *desc) { @@ -4337,6 +4359,8 @@ struct wined3d_texture struct wined3d_texture_sub_resource { + struct wined3d_client_sub_resource client; + void *parent; const struct wined3d_parent_ops *parent_ops; @@ -4978,6 +5002,8 @@ struct wined3d_buffer void *map_ptr; uintptr_t buffer_object; + struct wined3d_client_sub_resource client; + struct wined3d_range *maps; SIZE_T maps_size, modified_areas; @@ -6227,6 +6253,12 @@ static inline void wined3d_context_copy_bo_address(struct wined3d_context *conte context->device->adapter->adapter_ops->adapter_copy_bo_address(context, dst, src, size); } +static inline void wined3d_context_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ + context->device->adapter->adapter_ops->adapter_flush_bo_address(context, data, size); +} + static inline void wined3d_context_vk_reference_bo(const struct wined3d_context_vk *context_vk, struct wined3d_bo_vk *bo) { -- 2.33.0
On Thu, 7 Oct 2021 at 04:22, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+static void adapter_vk_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ + struct wined3d_context_vk *context_vk = wined3d_context_vk(context); + const struct wined3d_vk_info *vk_info; + struct wined3d_device_vk *device_vk; + VkMappedMemoryRange range; + struct wined3d_bo_vk *bo; + + if (!(bo = (struct wined3d_bo_vk *)data->buffer_object)) + return; + + vk_info = context_vk->vk_info; + device_vk = wined3d_device_vk(context->device); + + range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE; + range.pNext = NULL; + range.memory = bo->vk_memory; + range.offset = bo->memory_offset + (uintptr_t)data->addr; + range.size = size; + VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range)); +} + That has a fair amount of overlap with adapter_vk_unmap_bo_address(). Would a common helper make sense?
+static void *adapter_vk_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo) +{ + wined3d_not_from_cs(device->cs); + + /* Limit NOOVERWRITE maps to buffers for now; there are too many ways that + * a texture can be invalidated to even count. */ + if (wined3d_map_persistent() && resource->type == WINED3D_RTYPE_BUFFER && (flags & WINED3D_MAP_NOOVERWRITE)) + { + struct wined3d_client_sub_resource *sub_resource; + const struct wined3d_bo_vk *bo_vk; + uint8_t *map_ptr; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + + bo_vk = (const struct wined3d_bo_vk *)sub_resource->addr.buffer_object; + map_ptr = bo_vk ? bo_vk->map_ptr : NULL; + map_ptr += (uintptr_t)sub_resource->addr.addr; + + if (!map_ptr) + { + TRACE("Sub-resource is not persistently mapped.\n"); + return NULL; + } + + wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, row_pitch, slice_pitch); + + upload_bo->addr = *wined3d_const_bo_address(&sub_resource->addr); + upload_bo->flags = 0; + if (bo_vk) + map_ptr += bo_vk->memory_offset; + map_ptr = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box); + + if (!(bo_vk->memory_type & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) + upload_bo->flags |= UPLOAD_BO_FLUSH_ON_UNMAP; + + return map_ptr; + } + + return NULL; +}
Do we need adapter_vk_alloc_upload_bo() as such? I suppose it would make sense to expose something like wined3d_context_vk_create_bo() through the adapter ops, but the "upload bo" mechanism seems more generic than that. In its current form, this also doesn't allocate all that much, and doesn't seem that all that specific to the Vulkan backend. We use the wined3d_bo_vk structure, but the "map_ptr" and "memory_offset" would apply just as well to a GL equivalent. I.e., if we introduced a common wined3d_bo structure, this could just be in wined3d_cs_map_upload_bo().
+static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + memset(&sub_resource->addr, 0, sizeof(sub_resource->addr)); +} + Is this function correctly named? It doesn't quite do what I'd expect something called invalidate_persistent_map() to do.
@@ -2511,6 +2539,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context,
wined3d_resource_wait_idle(resource);
+ /* We might end up loading the resource into sysmem. */ + invalidate_persistent_map(resource, sub_resource_idx); + Sure, we might end up loading the resource into sysmem. That doesn't necessarily explain why we would need to call invalidate_persistent_map() though. (I.e., the issue is with invalidating the BUFFER location, not so much with loading into the SYSMEM location.)
@@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch;
wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP) + if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK; } Uploading when FLUSH_ON_UNMAP is set seems odd. I.e., should there ever be a case where FLUSH_ON_UNMAP is set, UPLOAD_ON_UNMAP is not set, and we still want to call wined3d_device_context_upload_bo()?
@@ -2581,8 +2612,7 @@ static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void * if (op->dst_resource->type == WINED3D_RTYPE_BUFFER) { wined3d_buffer_copy(buffer_from_resource(op->dst_resource), op->dst_box.left, - buffer_from_resource(op->src_resource), op->src_box.left, - op->src_box.right - op->src_box.left); + buffer_from_resource(op->src_resource), op->src_box.left, op->src_box.right - op->src_box.left); } That's a whitespace change. The change is not particularly wrong, but it doesn't seem particularly relevant to the rest of this patch either.
+ /* This might result in an implicit discard. */ + if (dst_resource->type == WINED3D_RTYPE_BUFFER && dst_box->right - dst_box->left == dst_resource->size) + invalidate_persistent_map(dst_resource, 0); + Somewhat like the other invalidate_persistent_map() comment above, it would require someone to already be fairly familiar with the code in question to understand the comment. (I.e., the issue here is that if the CS thread does end up doing a DISCARD for this operation, any subsequent operations would need to reference the new bo instead of the current one.)
@@ -2727,8 +2761,22 @@ static void wined3d_cs_exec_update_sub_resource(struct wined3d_cs *cs, const voi if (resource->type == WINED3D_RTYPE_BUFFER) { struct wined3d_buffer *buffer = buffer_from_resource(resource); + size_t size = box->right - box->left;
- wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, box->right - box->left); + if (op->bo.flags & UPLOAD_BO_FLUSH_ON_UNMAP) + { + wined3d_context_flush_bo_address(context, &op->bo.addr, size); + } + else + { + wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, size); + } + goto done; + } + It seems like we're mostly using FLUSH_ON_UNMAP here to determine whether "op->bo.addr" refers to the same GPU memory as "buffer->buffer_object". Could we perhaps just compare them?
static bool wined3d_cs_unmap_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_box *box, struct upload_bo *bo) { + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + if (sub_resource->upload_bo.addr.buffer_object || sub_resource->upload_bo.addr.addr) + { I guess that works; could perhaps do with a helper.
+ *bo = sub_resource->upload_bo; + *box = sub_resource->upload_box; + memset(&sub_resource->upload_bo, 0, sizeof(sub_resource->upload_bo)); + memset(&sub_resource->upload_box, 0, sizeof(sub_resource->upload_box)); + return true; + } + return false; }
We might as well just return if the condition is false, and save some indentation.
+struct wined3d_client_sub_resource +{ + struct wined3d_bo_address addr; + + struct upload_bo upload_bo; + struct wined3d_box upload_box; +}; + I'm tempted to think we want to track this per resource instead of per sub-resource. For buffers it doesn't really matter though, and for textures we don't implement it yet. Perhaps we should just leave textures alone for now? That would also avoid wined3d_resource_get_client_sub_resource() for the moment.
On Fri, 8 Oct 2021 at 18:24, Henri Verbeet <hverbeet(a)gmail.com> wrote:
+struct wined3d_client_sub_resource +{ + struct wined3d_bo_address addr; + + struct upload_bo upload_bo; + struct wined3d_box upload_box; +}; + I'm tempted to think we want to track this per resource instead of per sub-resource. For buffers it doesn't really matter though, and for textures we don't implement it yet. Perhaps we should just leave textures alone for now? That would also avoid wined3d_resource_get_client_sub_resource() for the moment.
Also, how does "addr" relate to "upload_bo.addr", and e.g. "buffer_object" in the case of buffer resources? I can figure it out by reading the patch, but could it perhaps be made more obvious when reading the structure definition?
On 10/8/21 11:29 AM, Henri Verbeet wrote:
On Fri, 8 Oct 2021 at 18:24, Henri Verbeet <hverbeet(a)gmail.com> wrote:
+struct wined3d_client_sub_resource +{ + struct wined3d_bo_address addr; + + struct upload_bo upload_bo; + struct wined3d_box upload_box; +}; + I'm tempted to think we want to track this per resource instead of per sub-resource. For buffers it doesn't really matter though, and for textures we don't implement it yet. Perhaps we should just leave textures alone for now? That would also avoid wined3d_resource_get_client_sub_resource() for the moment.
Also, how does "addr" relate to "upload_bo.addr", and e.g. "buffer_object" in the case of buffer resources? I can figure it out by reading the patch, but could it perhaps be made more obvious when reading the structure definition?
Hmm, I don't immediately see a nice way to change the field names. I guess I'd rename the latter two members to "mapped_bo"/"mapped_box", although that's not really unambiguous either. It could probably at least do with some comments in the struct definition.
On 10/8/21 11:24 AM, Henri Verbeet wrote:
On Thu, 7 Oct 2021 at 04:22, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+static void adapter_vk_flush_bo_address(struct wined3d_context *context, + const struct wined3d_const_bo_address *data, size_t size) +{ + struct wined3d_context_vk *context_vk = wined3d_context_vk(context); + const struct wined3d_vk_info *vk_info; + struct wined3d_device_vk *device_vk; + VkMappedMemoryRange range; + struct wined3d_bo_vk *bo; + + if (!(bo = (struct wined3d_bo_vk *)data->buffer_object)) + return; + + vk_info = context_vk->vk_info; + device_vk = wined3d_device_vk(context->device); + + range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE; + range.pNext = NULL; + range.memory = bo->vk_memory; + range.offset = bo->memory_offset + (uintptr_t)data->addr; + range.size = size; + VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range)); +} + That has a fair amount of overlap with adapter_vk_unmap_bo_address(). Would a common helper make sense?
Indeed, although the common helper may end up just being adapter_vk_flush_bo_address()...
+static void *adapter_vk_alloc_upload_bo(struct wined3d_device *device, struct wined3d_resource *resource, + unsigned int sub_resource_idx, const struct wined3d_box *box, unsigned int *row_pitch, + unsigned int *slice_pitch, uint32_t flags, struct upload_bo *upload_bo) +{ + wined3d_not_from_cs(device->cs); + + /* Limit NOOVERWRITE maps to buffers for now; there are too many ways that + * a texture can be invalidated to even count. */ + if (wined3d_map_persistent() && resource->type == WINED3D_RTYPE_BUFFER && (flags & WINED3D_MAP_NOOVERWRITE)) + { + struct wined3d_client_sub_resource *sub_resource; + const struct wined3d_bo_vk *bo_vk; + uint8_t *map_ptr; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + + bo_vk = (const struct wined3d_bo_vk *)sub_resource->addr.buffer_object; + map_ptr = bo_vk ? bo_vk->map_ptr : NULL; + map_ptr += (uintptr_t)sub_resource->addr.addr; + + if (!map_ptr) + { + TRACE("Sub-resource is not persistently mapped.\n"); + return NULL; + } + + wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, row_pitch, slice_pitch); + + upload_bo->addr = *wined3d_const_bo_address(&sub_resource->addr); + upload_bo->flags = 0; + if (bo_vk) + map_ptr += bo_vk->memory_offset; + map_ptr = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box); + + if (!(bo_vk->memory_type & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) + upload_bo->flags |= UPLOAD_BO_FLUSH_ON_UNMAP; + + return map_ptr; + } + + return NULL; +}
Do we need adapter_vk_alloc_upload_bo() as such? I suppose it would make sense to expose something like wined3d_context_vk_create_bo() through the adapter ops, but the "upload bo" mechanism seems more generic than that. In its current form, this also doesn't allocate all that much, and doesn't seem that all that specific to the Vulkan backend. We use the wined3d_bo_vk structure, but the "map_ptr" and "memory_offset" would apply just as well to a GL equivalent. I.e., if we introduced a common wined3d_bo structure, this could just be in wined3d_cs_map_upload_bo().
We don't need it yet, in this patch. We will need it later (or at least, it looks harder to make accelerated discard generic), but I believe we should be able to pull out the NOOVERWRITE parts, and, as you say, make a function called "alloc" really always allocate something.
+static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + memset(&sub_resource->addr, 0, sizeof(sub_resource->addr)); +} + Is this function correctly named? It doesn't quite do what I'd expect something called invalidate_persistent_map() to do.
I suppose your expectation is that it invalidates the map contents, which it does not, but rather it invalidates the map pointer. I don't know how better to name it to clarify that difference. (Shove a _ptr onto the end, maybe?) Essentially it means "the CS thread might reallocate the BO behind our back, thereby invalidating bo->map_ptr, which means we can't treat this one as canonical anymore". It's definitely awkward, but I don't see a better way to achieve this...
@@ -2511,6 +2539,9 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context,
wined3d_resource_wait_idle(resource);
+ /* We might end up loading the resource into sysmem. */ + invalidate_persistent_map(resource, sub_resource_idx); + Sure, we might end up loading the resource into sysmem. That doesn't necessarily explain why we would need to call invalidate_persistent_map() though. (I.e., the issue is with invalidating the BUFFER location, not so much with loading into the SYSMEM location.)
Right, I'll try to clarify that comment.
@@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch;
wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP) + if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK; } Uploading when FLUSH_ON_UNMAP is set seems odd. I.e., should there ever be a case where FLUSH_ON_UNMAP is set, UPLOAD_ON_UNMAP is not set, and we still want to call wined3d_device_context_upload_bo()?
As it stands this patch never has both FLUSH_ON_UNMAP and UPLOAD_ON_UNMAP set at the same time. I essentially was thinking of them as different things to do on an unmap. I guess in that sense "UPLOAD" should really have been "COPY" or "BLIT" or something.
@@ -2581,8 +2612,7 @@ static void wined3d_cs_exec_blt_sub_resource(struct wined3d_cs *cs, const void * if (op->dst_resource->type == WINED3D_RTYPE_BUFFER) { wined3d_buffer_copy(buffer_from_resource(op->dst_resource), op->dst_box.left, - buffer_from_resource(op->src_resource), op->src_box.left, - op->src_box.right - op->src_box.left); + buffer_from_resource(op->src_resource), op->src_box.left, op->src_box.right - op->src_box.left); } That's a whitespace change. The change is not particularly wrong, but it doesn't seem particularly relevant to the rest of this patch either.
Oops, must have been left over from something else...
+ /* This might result in an implicit discard. */ + if (dst_resource->type == WINED3D_RTYPE_BUFFER && dst_box->right - dst_box->left == dst_resource->size) + invalidate_persistent_map(dst_resource, 0); + Somewhat like the other invalidate_persistent_map() comment above, it would require someone to already be fairly familiar with the code in question to understand the comment. (I.e., the issue here is that if the CS thread does end up doing a DISCARD for this operation, any subsequent operations would need to reference the new bo instead of the current one.)
@@ -2727,8 +2761,22 @@ static void wined3d_cs_exec_update_sub_resource(struct wined3d_cs *cs, const voi if (resource->type == WINED3D_RTYPE_BUFFER) { struct wined3d_buffer *buffer = buffer_from_resource(resource); + size_t size = box->right - box->left;
- wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, box->right - box->left); + if (op->bo.flags & UPLOAD_BO_FLUSH_ON_UNMAP) + { + wined3d_context_flush_bo_address(context, &op->bo.addr, size); + } + else + { + wined3d_buffer_copy_bo_address(buffer, context, box->left, &op->bo.addr, size); + } + goto done; + } + It seems like we're mostly using FLUSH_ON_UNMAP here to determine whether "op->bo.addr" refers to the same GPU memory as "buffer->buffer_object". Could we perhaps just compare them?
I think that works.
static bool wined3d_cs_unmap_upload_bo(struct wined3d_device_context *context, struct wined3d_resource *resource, unsigned int sub_resource_idx, struct wined3d_box *box, struct upload_bo *bo) { + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + if (sub_resource->upload_bo.addr.buffer_object || sub_resource->upload_bo.addr.addr) + { I guess that works; could perhaps do with a helper.
Sure.
+ *bo = sub_resource->upload_bo; + *box = sub_resource->upload_box; + memset(&sub_resource->upload_bo, 0, sizeof(sub_resource->upload_bo)); + memset(&sub_resource->upload_box, 0, sizeof(sub_resource->upload_box)); + return true; + } + return false; }
We might as well just return if the condition is false, and save some indentation.
+struct wined3d_client_sub_resource +{ + struct wined3d_bo_address addr; + + struct upload_bo upload_bo; + struct wined3d_box upload_box; +}; + I'm tempted to think we want to track this per resource instead of per sub-resource. For buffers it doesn't really matter though, and for textures we don't implement it yet. Perhaps we should just leave textures alone for now? That would also avoid wined3d_resource_get_client_sub_resource() for the moment.
Indeed. Although I've found already that some games could really do with accelerated texture discard :-/
On Mon, 11 Oct 2021 at 05:53, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + memset(&sub_resource->addr, 0, sizeof(sub_resource->addr)); +} + Is this function correctly named? It doesn't quite do what I'd expect something called invalidate_persistent_map() to do.
I suppose your expectation is that it invalidates the map contents, which it does not, but rather it invalidates the map pointer. I don't know how better to name it to clarify that difference. (Shove a _ptr onto the end, maybe?)
Essentially it means "the CS thread might reallocate the BO behind our back, thereby invalidating bo->map_ptr, which means we can't treat this one as canonical anymore". It's definitely awkward, but I don't see a better way to achieve this...
Part of the issue is that it operates on a wined3d_resource, I think. You can tell it actually modifies the client sub-resource by looking at the implementation, but ideally you wouldn't have to. On the other hand, the fact that it's a persistent map is somewhat immaterial. I guess I would have expected something like invalidate_client_address(), or perhaps wined3d_client_sub_resource_invalidate_address().
@@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch;
wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP) + if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK; } Uploading when FLUSH_ON_UNMAP is set seems odd. I.e., should there ever be a case where FLUSH_ON_UNMAP is set, UPLOAD_ON_UNMAP is not set, and we still want to call wined3d_device_context_upload_bo()?
As it stands this patch never has both FLUSH_ON_UNMAP and UPLOAD_ON_UNMAP set at the same time. I essentially was thinking of them as different things to do on an unmap. I guess in that sense "UPLOAD" should really have been "COPY" or "BLIT" or something.
That probably works. Alternatively, could we just drop FLUSH_ON_UNMAP and set UPLOAD_ON_UNMAP any time we want to call wined3d_device_context_upload_bo()?
On 10/11/21 8:43 AM, Henri Verbeet wrote:
On Mon, 11 Oct 2021 at 05:53, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
+static void invalidate_persistent_map(struct wined3d_resource *resource, unsigned int sub_resource_idx) +{ + struct wined3d_client_sub_resource *sub_resource; + + sub_resource = wined3d_resource_get_client_sub_resource(resource, sub_resource_idx); + memset(&sub_resource->addr, 0, sizeof(sub_resource->addr)); +} + Is this function correctly named? It doesn't quite do what I'd expect something called invalidate_persistent_map() to do.
I suppose your expectation is that it invalidates the map contents, which it does not, but rather it invalidates the map pointer. I don't know how better to name it to clarify that difference. (Shove a _ptr onto the end, maybe?)
Essentially it means "the CS thread might reallocate the BO behind our back, thereby invalidating bo->map_ptr, which means we can't treat this one as canonical anymore". It's definitely awkward, but I don't see a better way to achieve this...
Part of the issue is that it operates on a wined3d_resource, I think. You can tell it actually modifies the client sub-resource by looking at the implementation, but ideally you wouldn't have to. On the other hand, the fact that it's a persistent map is somewhat immaterial. I guess I would have expected something like invalidate_client_address(), or perhaps wined3d_client_sub_resource_invalidate_address().
Sure, that makes sense.
@@ -2554,7 +2585,7 @@ HRESULT wined3d_device_context_emit_unmap(struct wined3d_device_context *context unsigned int row_pitch, slice_pitch;
wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx, &row_pitch, &slice_pitch); - if (bo.flags & UPLOAD_BO_UPLOAD_ON_UNMAP) + if (bo.flags & (UPLOAD_BO_UPLOAD_ON_UNMAP | UPLOAD_BO_FLUSH_ON_UNMAP)) wined3d_device_context_upload_bo(context, resource, sub_resource_idx, &box, &bo, row_pitch, slice_pitch); return WINED3D_OK; } Uploading when FLUSH_ON_UNMAP is set seems odd. I.e., should there ever be a case where FLUSH_ON_UNMAP is set, UPLOAD_ON_UNMAP is not set, and we still want to call wined3d_device_context_upload_bo()?
As it stands this patch never has both FLUSH_ON_UNMAP and UPLOAD_ON_UNMAP set at the same time. I essentially was thinking of them as different things to do on an unmap. I guess in that sense "UPLOAD" should really have been "COPY" or "BLIT" or something.
That probably works. Alternatively, could we just drop FLUSH_ON_UNMAP and set UPLOAD_ON_UNMAP any time we want to call wined3d_device_context_upload_bo()?
Right, that one's also fine with me.
On Thu, 7 Oct 2021 at 04:04, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
Partially undoes 61024aa12f.
We want to be able to allocate a new upload BO for a given map, but it's a bit structurally awkward to ask for a specific memory layout when parepare_upload_bo() might return different ones depending on the flags and other factors.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- This is kind of janky, though. It might not be all that unreasonable to ask prepare_upload_bo() for a specific layout.
Right; it's not spelled out above, but the issue is that in some cases we may want to return an existing allocation, in which case we wouldn't have control over its layout. The options are essentially to either ignore the requested layout in those cases, or to simply always let map_upload_bo() pick an appropriate layout. This patch (in combination with the following in the series) effectively does the latter. I suppose the question is, are there ever cases where we'd need a different layout than would be returned by map_upload_bo() by default? In terms of the commit message, note that prepare_upload_bo() no longer exists; that's map_upload_bo() now. While reviewing this patch, I noticed the wined3d_not_from_cs() call is below the map_upload_bo() block now; that seems undesirable.
@@ -2519,6 +2524,12 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, wined3d_device_context_submit(context, WINED3D_CS_QUEUE_MAP); wined3d_device_context_finish(context, WINED3D_CS_QUEUE_MAP);
+ if (SUCCEEDED(hr)) + { + map_desc->data = map_ptr; + map_desc->row_pitch = row_pitch; + map_desc->slice_pitch = slice_pitch; + } return hr; }
We might as well return on failure, and save some indentation.
On 10/8/21 11:23 AM, Henri Verbeet wrote:
On Thu, 7 Oct 2021 at 04:04, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
Partially undoes 61024aa12f.
We want to be able to allocate a new upload BO for a given map, but it's a bit structurally awkward to ask for a specific memory layout when parepare_upload_bo() might return different ones depending on the flags and other factors.
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- This is kind of janky, though. It might not be all that unreasonable to ask prepare_upload_bo() for a specific layout.
Right; it's not spelled out above, but the issue is that in some cases we may want to return an existing allocation, in which case we wouldn't have control over its layout. The options are essentially to either ignore the requested layout in those cases, or to simply always let map_upload_bo() pick an appropriate layout. This patch (in combination with the following in the series) effectively does the latter. I suppose the question is, are there ever cases where we'd need a different layout than would be returned by map_upload_bo() by default?
Right, that. I failed to correctly articulate the actual requirements. I don't believe there are actually any cases where we need a different layout that map_upload_bo() returns by default, although that's partly because we can control what map_upload_bo() returns by default. In a sense that's how it used to work. Fundamentally that means the map pitch of the entire resource for maps, and "anything, really" for update_sub_resource(). Since that's the fundamental difference, I wanted to deal with the problem of map pitch in those specific functions, rather than letting it be inferred from what essentially amounts to the flags we pass to prepare_upload_bo() [i.e. whether they include either DISCARD or NOOVERWRITE]. Though this is also why I even now am not particularly happy about the existence of that function as a combined entity :-/ But then on the other hand, it's kind of ugly to ask for a specific layout when map_upload_bo() might return an existing allocation. I guess one reason why it's better to let map_upload_bo() choose the pitch is that in the case of update_sub_resource we could potentially pick a staging texture of any size [not that I'm sure we have any reason to do that.] Or return a prior discard map if the resource hadn't been touched in the meantime [not that there's any reason for an application to do that?]
In terms of the commit message, note that prepare_upload_bo() no longer exists; that's map_upload_bo() now.
While reviewing this patch, I noticed the wined3d_not_from_cs() call is below the map_upload_bo() block now; that seems undesirable.
@@ -2519,6 +2524,12 @@ HRESULT wined3d_device_context_emit_map(struct wined3d_device_context *context, wined3d_device_context_submit(context, WINED3D_CS_QUEUE_MAP); wined3d_device_context_finish(context, WINED3D_CS_QUEUE_MAP);
+ if (SUCCEEDED(hr)) + { + map_desc->data = map_ptr; + map_desc->row_pitch = row_pitch; + map_desc->slice_pitch = slice_pitch; + } return hr; }
We might as well return on failure, and save some indentation.
On Mon, 11 Oct 2021 at 05:21, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
to do that.] Or return a prior discard map if the resource hadn't been touched in the meantime [not that there's any reason for an application to do that?]
In principle, but e.g. bug 35624 [1] is about exactly that kind of application. [1] https://bugs.winehq.org/show_bug.cgi?id=35624
participants (2)
-
Henri Verbeet -
Zebediah Figura