[PATCH v3 1/5] wined3d: Handle blits to Vulkan swapchains not matching the D3D swapchain.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50119 Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- v2: Fix off-by-one (not sure how I convinced myself that right and bottom coordinates were inclusive, they clearly are not), also recreate swapchain on VK_SUBOPTIMAL_KHR, retry to present the image after swapchain recreation. v3: Fix swapchain recreation and blit retry.
dlls/wined3d/swapchain.c | 72 ++++++++++++++++++++++------------ dlls/wined3d/wined3d_private.h | 1 + 2 files changed, 49 insertions(+), 24 deletions(-)
diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c index 0683ffef6fe..f362408fa7b 100644 --- a/dlls/wined3d/swapchain.c +++ b/dlls/wined3d/swapchain.c @@ -998,6 +998,9 @@ static HRESULT wined3d_swapchain_vk_create_vulkan_swapchain(struct wined3d_swapc goto fail; }
+ swapchain_vk->width = width; + swapchain_vk->height = height; + return WINED3D_OK;
fail: @@ -1031,7 +1034,7 @@ static void wined3d_swapchain_vk_set_swap_interval(struct wined3d_swapchain_vk * wined3d_swapchain_vk_recreate(swapchain_vk); }
-static void wined3d_swapchain_vk_blit(struct wined3d_swapchain_vk *swapchain_vk, +static VkResult wined3d_swapchain_vk_blit(struct wined3d_swapchain_vk *swapchain_vk, struct wined3d_context_vk *context_vk, const RECT *src_rect, const RECT *dst_rect, unsigned int swap_interval) { struct wined3d_texture_vk *back_buffer_vk = wined3d_texture_vk(swapchain_vk->s.back_buffers[0]); @@ -1044,34 +1047,36 @@ static void wined3d_swapchain_vk_blit(struct wined3d_swapchain_vk *swapchain_vk, unsigned int present_idx; VkImageLayout vk_layout; uint32_t image_idx; + RECT dst_rect_tmp; VkImageBlit blit; + VkFilter filter; VkResult vr; - HRESULT hr;
static const VkPipelineStageFlags wait_stage = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT;
+ TRACE("swapchain_vk %p, context_vk %p, src_rect %s, dst_rect %s, swap_interval %u.\n", + swapchain_vk, context_vk, wine_dbgstr_rect(src_rect), wine_dbgstr_rect(dst_rect), swap_interval); + wined3d_swapchain_vk_set_swap_interval(swapchain_vk, swap_interval);
present_idx = swapchain_vk->current++ % swapchain_vk->image_count; wined3d_context_vk_wait_command_buffer(context_vk, swapchain_vk->vk_semaphores[present_idx].command_buffer_id); - vr = VK_CALL(vkAcquireNextImageKHR(device_vk->vk_device, swapchain_vk->vk_swapchain, UINT64_MAX, - swapchain_vk->vk_semaphores[present_idx].available, VK_NULL_HANDLE, &image_idx)); - if (vr == VK_ERROR_OUT_OF_DATE_KHR) - { - if (FAILED(hr = wined3d_swapchain_vk_recreate(swapchain_vk))) - { - ERR("Failed to recreate swapchain, hr %#x.\n", hr); - return; - } - vr = VK_CALL(vkAcquireNextImageKHR(device_vk->vk_device, swapchain_vk->vk_swapchain, UINT64_MAX, - swapchain_vk->vk_semaphores[present_idx].available, VK_NULL_HANDLE, &image_idx)); - } - if (vr < 0) - { - ERR("Failed to acquire next Vulkan image, vr %s.\n", wined3d_debug_vkresult(vr)); - return; - } - + if ((vr = VK_CALL(vkAcquireNextImageKHR(device_vk->vk_device, swapchain_vk->vk_swapchain, UINT64_MAX, + swapchain_vk->vk_semaphores[present_idx].available, VK_NULL_HANDLE, &image_idx))) < 0) + return vr; + + if (dst_rect->right > swapchain_vk->width || dst_rect->bottom > swapchain_vk->height) + { + dst_rect_tmp = *dst_rect; + if (dst_rect->right > swapchain_vk->width) + dst_rect_tmp.right = swapchain_vk->width; + if (dst_rect->bottom > swapchain_vk->height) + dst_rect_tmp.bottom = swapchain_vk->height; + dst_rect = &dst_rect_tmp; + } + filter = src_rect->right - src_rect->left != dst_rect->right - dst_rect->left + || src_rect->bottom - src_rect->top != dst_rect->bottom - dst_rect->top + ? VK_FILTER_LINEAR : VK_FILTER_NEAREST; vk_command_buffer = wined3d_context_vk_get_command_buffer(context_vk);
wined3d_context_vk_end_current_render_pass(context_vk); @@ -1115,7 +1120,7 @@ static void wined3d_swapchain_vk_blit(struct wined3d_swapchain_vk *swapchain_vk, VK_CALL(vkCmdBlitImage(vk_command_buffer, back_buffer_vk->vk_image, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, swapchain_vk->vk_images[image_idx], VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, - 1, &blit, VK_FILTER_NEAREST)); + 1, &blit, filter));
wined3d_context_vk_reference_texture(context_vk, back_buffer_vk); wined3d_context_vk_image_barrier(context_vk, vk_command_buffer, @@ -1149,8 +1154,7 @@ static void wined3d_swapchain_vk_blit(struct wined3d_swapchain_vk *swapchain_vk, present_desc.pSwapchains = &swapchain_vk->vk_swapchain; present_desc.pImageIndices = &image_idx; present_desc.pResults = NULL; - if ((vr = VK_CALL(vkQueuePresentKHR(device_vk->vk_queue, &present_desc)))) - ERR("Present returned vr %s.\n", wined3d_debug_vkresult(vr)); + return VK_CALL(vkQueuePresentKHR(device_vk->vk_queue, &present_desc)); }
static void wined3d_swapchain_vk_rotate(struct wined3d_swapchain *swapchain, struct wined3d_context_vk *context_vk) @@ -1222,15 +1226,35 @@ static void swapchain_vk_present(struct wined3d_swapchain *swapchain, const RECT struct wined3d_swapchain_vk *swapchain_vk = wined3d_swapchain_vk(swapchain); struct wined3d_texture *back_buffer = swapchain->back_buffers[0]; struct wined3d_context_vk *context_vk; + VkResult vr; + HRESULT hr;
context_vk = wined3d_context_vk(context_acquire(swapchain->device, back_buffer, 0));
wined3d_texture_load_location(back_buffer, 0, &context_vk->c, back_buffer->resource.draw_binding);
if (swapchain_vk->vk_swapchain) - wined3d_swapchain_vk_blit(swapchain_vk, context_vk, src_rect, dst_rect, swap_interval); + { + if ((vr = wined3d_swapchain_vk_blit(swapchain_vk, context_vk, src_rect, dst_rect, swap_interval))) + { + if (vr == VK_ERROR_OUT_OF_DATE_KHR || vr == VK_SUBOPTIMAL_KHR) + { + if (FAILED(hr = wined3d_swapchain_vk_recreate(swapchain_vk))) + ERR("Failed to recreate swapchain, hr %#x.\n", hr); + else if (vr == VK_ERROR_OUT_OF_DATE_KHR && (vr = wined3d_swapchain_vk_blit( + swapchain_vk, context_vk, src_rect, dst_rect, swap_interval))) + ERR("Failed to blit image, vr %s.\n", wined3d_debug_vkresult(vr)); + } + else + { + ERR("Failed to blit image, vr %s.\n", wined3d_debug_vkresult(vr)); + } + } + } else + { swapchain_blit_gdi(swapchain, &context_vk->c, src_rect, dst_rect); + }
wined3d_swapchain_vk_rotate(swapchain, context_vk);
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 4ad0c05d34a..2b7e4186bc7 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -5304,6 +5304,7 @@ struct wined3d_swapchain_vk uint64_t command_buffer_id; } *vk_semaphores; unsigned int current, image_count; + unsigned int width, height; };
static inline struct wined3d_swapchain_vk *wined3d_swapchain_vk(struct wined3d_swapchain *swapchain)
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- dlls/wined3d/wined3d_main.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/dlls/wined3d/wined3d_main.c b/dlls/wined3d/wined3d_main.c index 293359714c3..7e6ef2a78ca 100644 --- a/dlls/wined3d/wined3d_main.c +++ b/dlls/wined3d/wined3d_main.c @@ -302,7 +302,10 @@ static BOOL wined3d_dll_init(HINSTANCE hInstDLL) } if (!get_config_key(hkey, appkey, "OffscreenRenderingMode", buffer, size) && !strcmp(buffer,"backbuffer")) + { + ERR_(winediag)("Offscreen rendering mode "backbuffer" selected by registry key.\n"); wined3d_settings.offscreen_rendering_mode = ORM_BACKBUFFER; + } if ( !get_config_key_dword( hkey, appkey, "VideoPciDeviceID", &tmpvalue) ) { int pci_device_id = tmpvalue;
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
if (!get_config_key(hkey, appkey, "OffscreenRenderingMode", buffer, size) && !strcmp(buffer,"backbuffer"))
{ERR_(winediag)("Offscreen rendering mode \"backbuffer\" selected by registry key.\n"); wined3d_settings.offscreen_rendering_mode = ORM_BACKBUFFER;}
Do we need this? We also print a message in wined3d_adapter_gl_init().
On Thu, Mar 18, 2021 at 2:33 PM Henri Verbeet hverbeet@gmail.com wrote:
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
if (!get_config_key(hkey, appkey, "OffscreenRenderingMode", buffer, size) && !strcmp(buffer,"backbuffer"))
{ERR_(winediag)("Offscreen rendering mode \"backbuffer\" selected by registry key.\n"); wined3d_settings.offscreen_rendering_mode = ORM_BACKBUFFER;}Do we need this? We also print a message in wined3d_adapter_gl_init().
Indeed, that's also better placed. I guess I forgot about it :/
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- dlls/wined3d/utils.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c index 11284d19811..dc7823b9534 100644 --- a/dlls/wined3d/utils.c +++ b/dlls/wined3d/utils.c @@ -3071,7 +3071,9 @@ static void query_internal_format(struct wined3d_adapter *adapter, query_view_class(format);
if (format->internal && format->f.flags[WINED3D_GL_RES_TYPE_RB] - & (WINED3DFMT_FLAG_RENDERTARGET | WINED3DFMT_FLAG_DEPTH_STENCIL)) + & (WINED3DFMT_FLAG_RENDERTARGET | WINED3DFMT_FLAG_DEPTH_STENCIL) + && (gl_info->supported[ARB_FRAMEBUFFER_OBJECT] || gl_info->supported[EXT_FRAMEBUFFER_MULTISAMPLE]) + && wined3d_settings.offscreen_rendering_mode == ORM_FBO) { if (gl_info->supported[ARB_INTERNALFORMAT_QUERY]) {
Signed-off-by: Henri Verbeet hverbeet@codeweavers.com
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- dlls/wined3d/surface.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c index d5d07d8c401..cb03b99fc97 100644 --- a/dlls/wined3d/surface.c +++ b/dlls/wined3d/surface.c @@ -422,6 +422,10 @@ void texture2d_read_from_framebuffer(struct wined3d_texture *texture, unsigned i unsigned int i; BYTE *mem;
+ TRACE("texture %p, sub_resource_idx %u, context %p, src_location %#x, dst_location %#x.\n", + texture, sub_resource_idx, context, src_location, dst_location); + + wined3d_texture_prepare_location(texture, sub_resource_idx, context, dst_location); wined3d_texture_get_memory(texture, sub_resource_idx, &data, dst_location);
restore_texture = context->current_rt.texture;
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -422,6 +422,10 @@ void texture2d_read_from_framebuffer(struct wined3d_texture *texture, unsigned i unsigned int i; BYTE *mem;
- TRACE("texture %p, sub_resource_idx %u, context %p, src_location %#x, dst_location %#x.\n",
texture, sub_resource_idx, context, src_location, dst_location);- wined3d_texture_prepare_location(texture, sub_resource_idx, context, dst_location); wined3d_texture_get_memory(texture, sub_resource_idx, &data, dst_location);
We shouldn't need this (texture2d_read_from_framebuffer() is ultimately a helper for wined3d_texture_gl_load_location(), which calls wined3d_texture_gl_prepare_location() before calling any of its helpers), and in principle we don't make prepare calls in equivalent functions.
On Thu, Mar 18, 2021 at 2:33 PM Henri Verbeet hverbeet@gmail.com wrote:
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -422,6 +422,10 @@ void texture2d_read_from_framebuffer(struct wined3d_texture *texture, unsigned i unsigned int i; BYTE *mem;
- TRACE("texture %p, sub_resource_idx %u, context %p, src_location %#x, dst_location %#x.\n",
texture, sub_resource_idx, context, src_location, dst_location);- wined3d_texture_prepare_location(texture, sub_resource_idx, context, dst_location); wined3d_texture_get_memory(texture, sub_resource_idx, &data, dst_location);
We shouldn't need this (texture2d_read_from_framebuffer() is ultimately a helper for wined3d_texture_gl_load_location(), which calls wined3d_texture_gl_prepare_location() before calling any of its helpers), and in principle we don't make prepare calls in equivalent functions.
I think this helped to some degree for backbuffer ORM, at some point in the past. I need to recheck but I agree that it would be the responsibility of the caller to prepare the destination anyway. Actually, maybe this was "required" by patch 5/5.
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- dlls/wined3d/surface.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/dlls/wined3d/surface.c b/dlls/wined3d/surface.c index cb03b99fc97..ca8e6ba89d5 100644 --- a/dlls/wined3d/surface.c +++ b/dlls/wined3d/surface.c @@ -1666,6 +1666,16 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_ TRACE("Not doing download because of partial download (src).\n"); else if (!wined3d_texture_is_full_rect(dst_texture, dst_sub_resource_idx % dst_texture->level_count, &dst_rect)) TRACE("Not doing download because of partial download (dst).\n"); + else if (src_sub_resource->locations == WINED3D_LOCATION_DRAWABLE) + { + context = context_acquire(device, src_texture, src_sub_resource_idx); + texture2d_read_from_framebuffer(src_texture, src_sub_resource_idx, context, + WINED3D_LOCATION_DRAWABLE, dst_texture->resource.map_binding); + wined3d_texture_validate_location(src_texture, src_sub_resource_idx, dst_texture->resource.map_binding); + context_release(context); + return texture2d_blt(dst_texture, dst_sub_resource_idx, dst_box, + src_texture, src_sub_resource_idx, src_box, flags, fx, filter); + } else { wined3d_texture_download_from_texture(dst_texture, dst_sub_resource_idx, src_texture,
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -1666,6 +1666,16 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_ TRACE("Not doing download because of partial download (src).\n"); else if (!wined3d_texture_is_full_rect(dst_texture, dst_sub_resource_idx % dst_texture->level_count, &dst_rect)) TRACE("Not doing download because of partial download (dst).\n");
else if (src_sub_resource->locations == WINED3D_LOCATION_DRAWABLE){context = context_acquire(device, src_texture, src_sub_resource_idx);texture2d_read_from_framebuffer(src_texture, src_sub_resource_idx, context,WINED3D_LOCATION_DRAWABLE, dst_texture->resource.map_binding);wined3d_texture_validate_location(src_texture, src_sub_resource_idx, dst_texture->resource.map_binding);context_release(context);return texture2d_blt(dst_texture, dst_sub_resource_idx, dst_box,src_texture, src_sub_resource_idx, src_box, flags, fx, filter);}
It's perhaps not immediately obvious from the function name, but texture2d_read_from_framebuffer() is specific to the GL backend. Calling it from common code like texture2d_blt() is problematic.
On Thu, Mar 18, 2021 at 2:33 PM Henri Verbeet hverbeet@gmail.com wrote:
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -1666,6 +1666,16 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_ TRACE("Not doing download because of partial download (src).\n"); else if (!wined3d_texture_is_full_rect(dst_texture, dst_sub_resource_idx % dst_texture->level_count, &dst_rect)) TRACE("Not doing download because of partial download (dst).\n");
else if (src_sub_resource->locations == WINED3D_LOCATION_DRAWABLE){context = context_acquire(device, src_texture, src_sub_resource_idx);texture2d_read_from_framebuffer(src_texture, src_sub_resource_idx, context,WINED3D_LOCATION_DRAWABLE, dst_texture->resource.map_binding);wined3d_texture_validate_location(src_texture, src_sub_resource_idx, dst_texture->resource.map_binding);context_release(context);return texture2d_blt(dst_texture, dst_sub_resource_idx, dst_box,src_texture, src_sub_resource_idx, src_box, flags, fx, filter);}It's perhaps not immediately obvious from the function name, but texture2d_read_from_framebuffer() is specific to the GL backend. Calling it from common code like texture2d_blt() is problematic.
You're right :/ For some background, this patch was also related to backbuffer ORM: IIRC after this the d3d tests start to give sensible results (mostly broken, of course, but at least backbuffer readback works so SOME tests pass).
I'll retest and rework these as necessary.
On Sun, 21 Mar 2021 at 22:06, Matteo Bruni matteo.mystral@gmail.com wrote:
On Thu, Mar 18, 2021 at 2:33 PM Henri Verbeet hverbeet@gmail.com wrote:
On Wed, 17 Mar 2021 at 13:35, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -1666,6 +1666,16 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_ TRACE("Not doing download because of partial download (src).\n"); else if (!wined3d_texture_is_full_rect(dst_texture, dst_sub_resource_idx % dst_texture->level_count, &dst_rect)) TRACE("Not doing download because of partial download (dst).\n");
else if (src_sub_resource->locations == WINED3D_LOCATION_DRAWABLE){context = context_acquire(device, src_texture, src_sub_resource_idx);texture2d_read_from_framebuffer(src_texture, src_sub_resource_idx, context,WINED3D_LOCATION_DRAWABLE, dst_texture->resource.map_binding);wined3d_texture_validate_location(src_texture, src_sub_resource_idx, dst_texture->resource.map_binding);context_release(context);return texture2d_blt(dst_texture, dst_sub_resource_idx, dst_box,src_texture, src_sub_resource_idx, src_box, flags, fx, filter);}It's perhaps not immediately obvious from the function name, but texture2d_read_from_framebuffer() is specific to the GL backend. Calling it from common code like texture2d_blt() is problematic.
You're right :/ For some background, this patch was also related to backbuffer ORM: IIRC after this the d3d tests start to give sensible results (mostly broken, of course, but at least backbuffer readback works so SOME tests pass).
I didn't write the patch, but my guess would be that the issue was that wined3d_texture_download_from_texture() doesn't handle resources in WINED3D_LOCATION_DRAWABLE. Perhaps the most straightforward way to address that would be to simply implement downloading from WINED3D_LOCATION_DRAWABLE in wined3d_texture_download_from_texture() and wined3d_texture_gl_download_data(). Alternatively: - We could detect that case here, skip wined3d_texture_download_from_texture(), and let the blitter figure it out. That should work, although it may introduce more copies between the DRAWABLE/TEXTURE/SYSMEM locations than desirable. - Instead of skipping wined3d_texture_download_from_texture(), we could call wined3d_texture_load_location() before calling it. The main disadvantage of that approach is that for on-screen resources like the back-buffer that ends up doing a flip in system memory first. In that context, it may be worth considering using the "AlwaysOffscreen" approach for backbuffer ORM as well. That would introduce an extra blit for presents, but would simplify some other, potentially more expensive things, like the issue we're discussing here.
-
Henri Verbeet -
Henri Verbeet -
Matteo Bruni -
Matteo Bruni