On Thu, 2 Sept 2021 at 15:14, Jan Sikorski jsikorski@codeweavers.com wrote:
static BOOL wined3d_texture_use_pbo(const struct wined3d_texture *texture, const struct wined3d_gl_info *gl_info) {
- if (!gl_info->supported[ARB_PIXEL_BUFFER_OBJECT]
|| texture->resource.format->conv_byte_count
- if (gl_info->selected_gl_version && !gl_info->supported[ARB_PIXEL_BUFFER_OBJECT])
return FALSE;
It's perhaps a bit of a hack that the existing code was never updated to avoid using "gl_info" in what is now backend independent code, but this change doesn't make it better. This should probably just store something in the wined3d_d3d_info structure, similar to what we do with e.g. "texture_npot".
@@ -874,10 +882,11 @@ static void wined3d_texture_update_map_binding(struct wined3d_texture *texture) if (texture->sub_resources[i].locations == texture->resource.map_binding && !wined3d_texture_load_location(texture, i, context, map_binding)) ERR("Failed to load location %s.\n", wined3d_debug_location(map_binding));
if (texture->resource.map_binding == WINED3D_LOCATION_BUFFER)wined3d_texture_remove_buffer_object(texture, i, wined3d_context_gl(context));
- if (texture->resource.map_binding == WINED3D_LOCATION_BUFFER)
wined3d_texture_unload_location(texture, context, WINED3D_LOCATION_BUFFER);
This seems fine, but is somewhat of an independent change.
@@ -4673,12 +4679,6 @@ static void wined3d_texture_vk_upload_data(struct wined3d_context *context, src_row_pitch, src_slice_pitch, dst_texture, dst_sub_resource_idx, wined3d_debug_location(dst_location), dst_x, dst_y, dst_z);
- if (src_bo_addr->buffer_object)
- {
FIXME("Unhandled buffer object %#lx.\n", src_bo_addr->buffer_object);return;- }
Likewise, I think it would make sense for "Implement support for buffer objects in wined3d_texture_vk_upload_data()" to be a separate commit.
- if (!wined3d_context_vk_create_bo(context_vk, sub_resource->size,
VK_BUFFER_USAGE_TRANSFER_SRC_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, &staging_bo))
- if (!(vk_command_buffer = wined3d_context_vk_get_command_buffer(context_vk))) {
ERR("Failed to create staging bo.\n");
ERR("Failed to get command buffer.\n");if (!src_bo_addr->buffer_object)wined3d_context_vk_destroy_bo(context_vk, &staging_bo); return;
The staging bo would not have been created yet above.
- staging_bo_addr.buffer_object = (uintptr_t)&staging_bo;
- staging_bo_addr.addr = NULL;
- if (!(map_ptr = wined3d_context_map_bo_address(context, &staging_bo_addr,
sub_resource->size, WINED3D_MAP_DISCARD | WINED3D_MAP_WRITE)))
- wined3d_context_vk_end_current_render_pass(context_vk);
I assume we're ending the current render pass (if any) for vkCmdCopyBufferToImage(). In principle wined3d_context_vk_image_barrier() already takes care of that, but I suppose it makes sense to be explicit about it. Perhaps it wouldn't hurt to add a comment mentioning the reason we need to end the render pass; I didn't add those for the existing instances of wined3d_context_vk_end_current_render_pass(), and that may have been a mistake. In any case, this too is somewhat of an independent change.
{
ERR("Failed to get command buffer.\n");wined3d_context_vk_destroy_bo(context_vk, &staging_bo);return;
vk_barrier.sType = VK_STRUCTURE_TYPE_BUFFER_MEMORY_BARRIER;vk_barrier.pNext = NULL;vk_barrier.srcAccessMask = vk_access_mask_from_buffer_usage(src_bo->usage) & ~WINED3D_READ_ONLY_ACCESS_FLAGS;vk_barrier.dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT;vk_barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;vk_barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;vk_barrier.buffer = src_bo->vk_buffer;vk_barrier.offset = src_bo->buffer_offset;vk_barrier.size = src_bo->size;
This adds a barrier for the entire bo, not just the part we're reading from. Perhaps that's ok though.
- region.bufferOffset = staging_bo.buffer_offset;
- region.bufferRowLength = (dst_row_pitch / src_format->block_byte_count) * src_format->block_width;
- if (dst_row_pitch)
region.bufferImageHeight = (dst_slice_pitch / dst_row_pitch) * src_format->block_height;
- region.bufferOffset = src_bo->buffer_offset + src_offset;
That ignores "src_bo_addr->addr". Currently that's probably always going to be 0 in practice, but it wouldn't necessarily be if we e.g. switched to using a single bo per texture instead of using a bo for each sub-resource.
- if (src_bo_addr->buffer_object)
- {
if (vk_barrier.srcAccessMask)VK_CALL(vkCmdPipelineBarrier(vk_command_buffer, VK_PIPELINE_STAGE_TRANSFER_BIT,bo_stage_flags, 0, 0, NULL, 0, NULL, 0, NULL));- }
- else
- {
wined3d_context_vk_destroy_bo(context_vk, &staging_bo);- }
Alternatively, we also sometimes write checks like these as "if (src_bo == &staging_bo)". That may be slightly clearer, but I don't feel strongly about it.
@@ -4839,12 +4882,6 @@ static void wined3d_texture_vk_download_data(struct wined3d_context *context, return; }
- if (dst_bo_addr->buffer_object)
- {
FIXME("Unhandled buffer object %#lx.\n", dst_bo_addr->buffer_object);return;- }
Some of the comments for wined3d_texture_vk_upload_data() apply to wined3d_texture_vk_download_data() as well.
- if (dst_bo_addr->buffer_object) {
ERR("Failed to map staging bo.\n");wined3d_context_vk_destroy_bo(context_vk, &staging_bo);return;
vk_barrier.dstAccessMask = vk_barrier.srcAccessMask | VK_ACCESS_HOST_READ_BIT;vk_barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT;VK_CALL(vkCmdPipelineBarrier(vk_command_buffer, VK_PIPELINE_STAGE_TRANSFER_BIT,bo_stage_flags | VK_PIPELINE_STAGE_HOST_BIT, 0, 0, NULL, 1, &vk_barrier, 0, NULL));
Do we need the HOST_READ bits in this patch? This seems like something that belongs in 3/4, and should probably only be done for bo's that have the "host_synced" flag set.
@@ -4287,7 +4295,11 @@ struct wined3d_texture unsigned int map_count; uint32_t map_flags; DWORD locations;
struct wined3d_bo_gl bo;
union{struct wined3d_bo_gl gl;struct wined3d_bo_vk vk;} bo;
This is probably fine for the moment, but ideally we'd avoid backend specific fields in the wined3d_texture_sub_resource structure. Relatedly, we may want to use a single bo for the entire texture, similar to how we use a single allocation for the system memory location.