On Mon, 8 Nov 2021 at 18:01, Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 11/5/21 12:39 PM, Henri Verbeet wrote:
On Fri, 5 Nov 2021 at 04:52, Zebediah Figura <zfigura(a)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.