[PATCH 2/4] wined3d: Support buffer object backed textures in the Vulkan backend.

[Henri Verbeet]

On Thu, 2 Sept 2021 at 15:14, Jan Sikorski <jsikorski at 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.