Re: [PATCH v2 7/7] wined3d: Handle NOOVERWRITE maps on persistently mapped Vulkan buffers from the client thread.

[Henri Verbeet]

On Tue, 12 Oct 2021 at 23:31, Zebediah Figura  wrote:
> +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);
>
> +}
> -

Note that we do have an implementation for this in
wined3d_context_gl_unmap_bo_address().
> +static void flush_buffer_range(struct wined3d_context_vk *context_vk,
> -
>    struct wined3d_bo_vk *bo, unsigned int offset, unsigned int size)
>
> +{
> - struct wined3d_device_vk *device_vk = wined3d_device_vk(context_vk->c.device);
> - const struct wined3d_vk_info *vk_info = context_vk->vk_info;
> - VkMappedMemoryRange range;
> -
> - range.sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE;
> - range.pNext = NULL;
> - range.memory = bo->vk_memory;
> - range.offset = bo->b.memory_offset + offset;
> - range.size = size;
> - VK_CALL(vkFlushMappedMemoryRanges(device_vk->vk_device, 1, &range));
>
> +}
> -

flush_buffer_range() doesn't actually operate on a buffer though...
> @@ -938,6 +938,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->resource.client.addr = addr;

Does that update need to be atomic? I.e., is there any risk of the
application thread seeing e.g. a new "addr.buffer_object" in
combination with an old "addr.addr"? I think the idea is that we're
protected from that by needing to go through the CS if the buffer
isn't mapped yet, right?
> static bool wined3d_cs_map_upload_bo(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, uint32_t flags)
>  {
> - /* FIXME: We would like to return mapped or newly allocated memory here. */
> - /* 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_resource *client = &resource->client;
> -
>    const struct wined3d_bo *bo;
> -
>    uint8_t *map_ptr;
> -
> -
>    bo = (const struct wined3d_bo *)client->addr.buffer_object;
> -
>    map_ptr = bo ? bo->map_ptr : NULL;
> -
>    map_ptr += (uintptr_t)client->addr.addr;
> -
> -
>    if (!map_ptr)
> -
>    {
> -
>        TRACE("Sub-resource is not persistently mapped.\n");
> -
>        return false;
> -
>    }
> -

So if "bo" is NULL, "map_ptr" can in theory end up being non-NULL
here. I guess that in practice "client->addr.addr" is always going to
be NULL if "bo" is, but in that case there's not much point in adding
the offset before checking "map_ptr" either.
> -
>    wined3d_resource_get_sub_resource_map_pitch(resource, sub_resource_idx,
> -
>            &map_desc->row_pitch, &map_desc->slice_pitch);
> -
> -
>    client->mapped_upload.addr = *wined3d_const_bo_address(&client->addr);
> -
>    client->mapped_upload.flags = 0;
> -
>    if (bo)
> -
>        map_ptr += bo->memory_offset;

Then we account for "bo" potentially being NULL here,
> -
>    map_desc->data = resource_offset_map_pointer(resource, sub_resource_idx, map_ptr, box);
> -
> -
>    if (!bo->coherent)
> -
>        client->mapped_upload.flags |= UPLOAD_BO_UPLOAD_ON_UNMAP;
> -

but not here.