Re: [PATCH 8/8] wined3d: Try to allocate new Vulkan BOs from the client thread for DISCARD maps.

[Henri Verbeet]

On Wed, 3 Nov 2021 at 00:37, Zebediah Figura  wrote:
> +static bool adapter_vk_alloc_bo(struct wined3d_device *device, struct wined3d_resource *resource,
> -
>    unsigned int sub_resource_idx, struct wined3d_bo_address *addr)
>
> +{
> - wined3d_not_from_cs(device->cs);
> -
> - if (resource->type == WINED3D_RTYPE_BUFFER)
> - {
> -
>    struct wined3d_buffer_vk *buffer_vk = wined3d_buffer_vk(buffer_from_resource(resource));
> -
>    struct wined3d_client_bo_vk *client_bo;
> -
> -
>    if (!(client_bo = heap_alloc(sizeof(*client_bo))))
> -
>        return false;
> -
> -
>    if (!wined3d_buffer_vk_create_buffer_object(buffer_vk, NULL, &client_bo->bo))
> -
>    {
> -
>        heap_free(client_bo);
> -
>        return false;
> -
>    }
> -
> -
>    if (!client_bo->bo.b.map_ptr)
> -
>    {
> -
>        struct wined3d_client_bo_vk_map_ctx ctx = {.device = device, .client_bo = client_bo};
> -
> -
>        WARN_(d3d_perf)("BO %p (chunk %p, slab %p) is not persistently mapped.\n", &client_bo->bo,
> -
>                client_bo->bo.memory ? client_bo->bo.memory->chunk : NULL, client_bo->bo.slab);
> -
> -
>        wined3d_cs_map_object(device->cs, wined3d_client_bo_vk_map_cs, &ctx);
> -
>        wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_MAP);
> -
>    }

wined3d_cs_map_object() almost sounds like it would emit a
WINED3D_CS_OP_MAP, somewhat like wined3d_device_context_map() back
when that was still called wined3d_cs_map()...
Here too, Vulkan could map from application threads, it just needs to
be synchronised. In the general case we'd need to make sure the GPU
isn't still using the bo, but that's not an issue for newly allocated
bo's.
> +void wined3d_buffer_rename(struct wined3d_buffer *buffer, struct wined3d_context *context, struct wined3d_bo *bo)
> +{
> - TRACE("buffer %p, context %p, bo %p.\n", buffer, context, bo);
> -
> - buffer->buffer_ops->buffer_rename_bo(buffer, context, bo);
> - buffer->buffer_object = (uintptr_t)bo;
> - wined3d_buffer_validate_location(buffer, WINED3D_LOCATION_BUFFER);
> - wined3d_buffer_invalidate_location(buffer, ~WINED3D_LOCATION_BUFFER);
>
> +}
> -

If we're doing this at the wined3d_buffer level, perhaps it would make
sense to name this something like wined3d_buffer_set_bo(),
wined3d_buffer_assign_bo(), or something along those lines.
> +static void wined3d_buffer_vk_rename_bo(struct wined3d_buffer *buffer,
> -
>    struct wined3d_context *context, struct wined3d_bo *bo)
>
> +{
> - struct wined3d_bo_vk *prev_bo = (struct wined3d_bo_vk *)buffer->buffer_object;
> - struct wined3d_context_vk *context_vk = wined3d_context_vk(context);
> - struct wined3d_buffer_vk *buffer_vk = wined3d_buffer_vk(buffer);
> - struct wined3d_bo_vk *bo_vk = wined3d_bo_vk(bo);
> -
> - if (prev_bo)
> - {
> -
>    struct wined3d_bo_user *bo_user;
> -
> -
>    LIST_FOR_EACH_ENTRY(bo_user, &prev_bo->b.users, struct wined3d_bo_user, entry)
> -
>        bo_user->valid = false;
> -
>    assert(list_empty(&bo_vk->b.users));
> -
>    list_move_head(&bo_vk->b.users, &prev_bo->b.users);
> -
> -
>    if (prev_bo != &buffer_vk->bo)
> -
>    {
> -
>        struct wined3d_client_bo_vk *client_bo = CONTAINING_RECORD(prev_bo, struct wined3d_client_bo_vk, bo);
> -
> -
>        wined3d_context_vk_destroy_bo(context_vk, &client_bo->bo);
> -
>        heap_free(client_bo);
> -
>    }
> - }
> - else
> - {
> -
>    list_add_head(&bo_vk->b.users, &buffer_vk->b.bo_user.entry);
> - }
>
> +}
> -

I admit I didn't try very hard, but it's not immediately clear to me
why we wouldn't always assign the new bo to "buffer_vk->bo" here, and
mark the previous one for destruction.