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

[Henri Verbeet]

On Mon, 8 Nov 2021 at 18:01, Zebediah Figura <zfigura at codeweavers.com> wrote:
> 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:
> >> +        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?)
>
The ERR is fine.

> >> +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.
>
The issue isn't so much that it's in buffer.c; in principle it would
be fine to introduce e.g. wined3d_buffer_update_sub_resource()
containing the implementation of wined3d_cs_exec_update_sub_resource()
for buffers. The reason that doesn't already exist is mostly just that
the buffer part of wined3d_cs_exec_update_sub_resource() is fairly
straightforward. The issue here is more that wined3d_buffer_set_bo()
seems like something that's conceptually on a lower level than
sub-resource updates, much like e.g.
wined3d_resource_prepare_sysmem(),
wined3d_buffer_vk_create_buffer_object(), or
wined3d_buffer_gl_create_buffer_object(). I.e., it's not obvious to me
that replacing a buffer's bo should modify its location flags.

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

Right, I think doing bo destruction through the adapter ops would make
sense for this purpose.