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

[Henri Verbeet]

On Fri, 5 Nov 2021 at 04:52, Zebediah Figura  wrote:
> +struct wined3d_client_bo_vk_map_ctx
> +{
> - struct wined3d_device *device;
> - struct wined3d_client_bo_vk *client_bo;
>
> +};
> -

We don't need this anymore.
> +static bool adapter_vk_alloc_bo(struct wined3d_device *device, struct wined3d_resource *resource,
> -
>    unsigned int sub_resource_idx, struct wined3d_bo_address *addr)
>
> +{
> - struct wined3d_device_vk *device_vk = wined3d_device_vk(device);
> - struct wined3d_context_vk *context_vk = &device_vk->context_vk;
> -
> - wined3d_not_from_cs(device->cs);
> - assert(device->context_count);
> -
> - if (resource->type == WINED3D_RTYPE_BUFFER)
> - {
> -
>    struct wined3d_bo_vk *bo_vk;
> -
> -
>    if (!(bo_vk = heap_alloc(sizeof(*bo_vk))))
> -
>        return false;
> -
> -
>    if (!(wined3d_context_vk_create_bo(context_vk, resource->size,
> -
>            vk_buffer_usage_from_bind_flags(resource->bind_flags),
> -
>            vk_memory_type_from_access_flags(resource->access, resource->usage), bo_vk)))
> -
>    {
> -
>        WARN("Failed to create Vulkan buffer.\n");
> -
>        return false;
> -
>    }

We leak "bo_vk" here.
> -
>    if (!bo_vk->b.map_ptr)
> -
>    {
> -
>        WARN_(d3d_perf)("BO %p (chunk %p, slab %p) is not persistently mapped.\n",
> -
>                bo_vk, bo_vk->memory ? bo_vk->memory->chunk : NULL, bo_vk->slab);
> -
> -
>        if (!wined3d_bo_vk_map(bo_vk, context_vk))
> -
>            ERR("Failed to map bo.\n");
> -
>    }
> -

I suppose we could clean up and return "false" if mapping the bo fails
here. On the other hand, I suppose it shouldn't fail in the first
place.
> +void wined3d_buffer_set_bo(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_set_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);
>
> +}
> -

Is wined3d_buffer_set_bo() the proper place for the
validate/invalidate? Arguably it would be more appropriate to do that
in wined3d_cs_exec_update_sub_resource(). I suppose it mirrors
wined3d_buffer_copy_bo_address().
> +static void wined3d_buffer_vk_set_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);
> -
> - /* We can't just copy the contents of bo_vk into buffer_vk->bo, because the
> -
> * new BO might still be in use by the client thread. We could allow both to
> -
> * be valid, although knowing when to destroy a BO then becomes tricky, and
> -
> * ensuring it's not mapped more than once also becomes tricky. */
> -

There's probably less of a need for the comment now.
> - 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);
> -
> -
>    wined3d_context_vk_destroy_bo(context_vk, prev_bo);
> -
>    heap_free(prev_bo);
> - }
> - else
> - {
> -
>    list_add_head(&bo_vk->b.users, &buffer_vk->b.bo_user.entry);
> - }
>
> +}
> -

Note that aside from the wined3d_context_vk_destroy_bo() call, this is
all just generic code now.