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