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

[Zebediah Figura]

On 11/3/21 12:11 PM, Henri Verbeet wrote:
> On Wed, 3 Nov 2021 at 00:37, Zebediah Figura <zfigura at codeweavers.com> 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()...

Indeed, except that WINED3D_CS_OP_MAP takes the resource. I don't think 
we want to change that, either, although we could create a new CS op 
instead of using WINED3D_CS_OP_CALLBACK.

(Now that I think about it, *is* there any sort of a rule as to what 
deserves WINED3D_CS_OP_CALLBACK and what doesn't? I know that currently 
it's just a matter of init and destroy, but it seeems a bit arbitrary on 
reflection.)

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

Right, the basic reason is that the client thread might still be holding 
a pointer to "bo_vk" in the wined3d_client_resource structure. I'll add 
a comment to that effect...