From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 5/6] wined3d: Pass map flags to wined3d_context_copy_bo_address(). Message-Id: Date: Fri, 18 Jun 2021 10:50:28 -0500 In-Reply-To: References: <20210618041939.228411-1-z.figura12@gmail.com> <20210618041939.228411-5-z.figura12@gmail.com> On 6/18/21 7:24 AM, Henri Verbeet wrote: > On Fri, 18 Jun 2021 at 06:20, Zebediah Figura wrote: >> dlls/wined3d/adapter_gl.c | 6 +++--- >> dlls/wined3d/adapter_vk.c | 14 +++++++------- >> dlls/wined3d/buffer.c | 8 ++++++-- >> dlls/wined3d/context_gl.c | 2 +- >> dlls/wined3d/directx.c | 4 ++-- >> dlls/wined3d/view.c | 2 +- >> dlls/wined3d/wined3d_private.h | 12 ++++++------ >> 7 files changed, 26 insertions(+), 22 deletions(-) >> > This may be ok, but I'm curious about the reasoning. In particular, is > there ever a case where you'd want to pass WINED3D_MAP_DISCARD to > wined3d_context_copy_bo_address(), instead of just using > wined3d_context_map_bo_address() with WINED3D_MAP_DISCARD? > > And mostly to illustrate the point: > >> @@ -1084,8 +1084,8 @@ static void adapter_vk_copy_bo_address(struct wined3d_context *context, >> >> staging.buffer_object = (uintptr_t)&staging_bo; >> staging.addr = NULL; >> - adapter_vk_copy_bo_address(context, &staging, src, size); >> - adapter_vk_copy_bo_address(context, dst, &staging, size); >> + adapter_vk_copy_bo_address(context, &staging, src, size, WINED3D_MAP_WRITE | WINED3D_MAP_DISCARD); >> + adapter_vk_copy_bo_address(context, dst, &staging, size, map_flags); >> > We just created the staging bo, so it's idle by definition. > >> @@ -1104,8 +1104,8 @@ static void adapter_vk_copy_bo_address(struct wined3d_context *context, >> >> staging.buffer_object = (uintptr_t)&staging_bo; >> staging.addr = NULL; >> - adapter_vk_copy_bo_address(context, &staging, src, size); >> - adapter_vk_copy_bo_address(context, dst, &staging, size); >> + adapter_vk_copy_bo_address(context, &staging, src, size, WINED3D_MAP_WRITE | WINED3D_MAP_DISCARD); >> + adapter_vk_copy_bo_address(context, dst, &staging, size, map_flags); >> > Likewise. > >> @@ -1012,13 +1012,17 @@ static HRESULT buffer_resource_sub_resource_unmap(struct wined3d_resource *resou >> void wined3d_buffer_copy_bo_address(struct wined3d_buffer *dst_buffer, struct wined3d_context *context, >> unsigned int dst_offset, const struct wined3d_const_bo_address *src_addr, unsigned int size) >> { >> + DWORD map_flags = WINED3D_MAP_WRITE; >> struct wined3d_bo_address dst_addr; >> DWORD dst_location; >> >> + if (!dst_offset && size == dst_buffer->resource.size) >> + map_flags |= WINED3D_MAP_DISCARD; >> + >> dst_location = wined3d_buffer_get_memory(dst_buffer, context, &dst_addr); >> dst_addr.addr += dst_offset; >> >> - wined3d_context_copy_bo_address(context, &dst_addr, (const struct wined3d_bo_address *)src_addr, size); >> + wined3d_context_copy_bo_address(context, &dst_addr, (const struct wined3d_bo_address *)src_addr, size, map_flags); >> wined3d_buffer_invalidate_range(dst_buffer, ~dst_location, dst_offset, size); >> } >> > Sure, but if that's worth doing, we may as well do it in the > wined3d_context_copy_bo_address() implementations. This is the case I care about mostly. I didn't actually bother to check whether we had access to the BO size, but evidently we do. I'll alter the implementation accordingly. > >> @@ -1526,7 +1530,7 @@ static void wined3d_buffer_vk_upload_ranges(struct wined3d_buffer *buffer, struc >> >> src.addr = (uint8_t *)data + range->offset - data_offset; >> dst.addr = (void *)(uintptr_t)range->offset; >> - wined3d_context_copy_bo_address(context, &dst, &src, range->size); >> + wined3d_context_copy_bo_address(context, &dst, &src, range->size, flags); >> } >> > "flags" never contains DISCARD here if "dst" is mappable, and if it's > not, it doesn't matter. > >> @@ -1597,7 +1597,7 @@ void wined3d_unordered_access_view_set_counter(struct wined3d_unordered_access_v >> dst.buffer_object = view->counter_bo; >> dst.addr = NULL; >> >> - wined3d_context_copy_bo_address(context, &dst, &src, sizeof(uint32_t)); >> + wined3d_context_copy_bo_address(context, &dst, &src, sizeof(uint32_t), WINED3D_MAP_WRITE | WINED3D_MAP_DISCARD); >> >> context_release(context); >> } > Sure, but like further above, this could be caught in the > wined3d_context_copy_bo_address() implementations. >