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 <zfigura(a)codeweavers.com> 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.