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