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

[Zebediah Figura]

On 11/5/21 12:39 PM, Henri Verbeet wrote:
> On Fri, 5 Nov 2021 at 04:52, Zebediah Figura <zfigura at 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.

Cleaning up is a bit nontrivial; I'd rather leave it as is (or use an 
assert?)

> 
>> +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().

It definitely seems that most validation logic is done within buffer.c, 
and that makes more sense to me, along the lines of letting that be a 
buffer-specific implementation detail. I can change it if you feel 
strongly otherwise, though.

> 
>> +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.
> 

Yes, which is why I wanted to turn destroy_bo() into an adapter op 
earlier. I guess if we had a wined3d_texture_set_bo() it would also be 
all generic code except for a destroy_bo() call, so maybe it does make 
sense to make destroy_bo() an adapter op, if not per se for 
unload_location() as well.