Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vk] check for attachment format properties before pre-creating render passes #5109

Merged
merged 3 commits into from Sep 11, 2018

Conversation

Projects
None yet
4 participants
@kvark
Copy link
Contributor

kvark commented Sep 11, 2018

Fixes #5108
Ideally, the gpu_formats_support struct would know which of the formats we can use as render targets. However, it was tempting enough to performs the checks in-place (at get_precomputed_render_passes) as opposed to having a field per format in the support struct. So I went the easier route. Let me know if you'd like the support struct to do the queries instead.

@elhaya

This comment has been minimized.

Copy link

elhaya commented Sep 11, 2018

all games i have tried crashed with this message:
image

@kd-11
Copy link
Contributor

kd-11 left a comment

Coding style issues aside, isnt this incomplete? What happens when the application requests a format to draw into that is not supported? You will end up with a null render pass handle. Also, due to complicated reasons, you cannot just exchange/emulate color formats easily due to how games use them. Which are these formats missing from metal/gfxrs? At least throw when use of unsupported formats is attempted and mention that its because of driver incompatibility.

@kvark

This comment has been minimized.

Copy link
Contributor Author

kvark commented Sep 11, 2018

@elhaya thanks for testing it! I'll debug it on a real Vulkan driver to see what's going on.
Did you build it yourself on Windows? In which case, could you build it with Debug information and provide the full call stack? That would be very helpful.

@kd-11

What happens when the application requests a format to draw into that is not supported? You will end up with a null render pass handle.

Yes, if a render pass is not possible on the Vulkan driver, then RPCS3 will need to crash gracefully on an attempt to use such a pass. This is better than crashing at initialization without knowing if the pass is going to be ever needed in the first place.

Which are these formats missing from metal/gfxrs?

Unfortunately, Metal on MacOS (specifically, there is no such restriction on iOS/tvOS) doesn't support packed 16bit color formats, such as R5G6B5 or A1R5G5B5.

At least throw when use of unsupported formats is attempted and mention that its because of driver incompatibility.

I can add this throw for sure.

@elhaya

This comment has been minimized.

Copy link

elhaya commented Sep 11, 2018

@kvark no sorry.. i downloaded precompiled from appveyor. would have gladly helped but i deleted my entire compiling rig (eg qt3 ..)

@MSuih

This comment has been minimized.

Copy link
Contributor

MSuih commented Sep 11, 2018

Similar error appears on AMD drivers as well. Can't provide stacktrace either but if I enable debug output the following lines appear in the log as it crashes:

·E 0:00:06.822774 {rsx::thread} RSX: ERROR: [Validation] Code -1 : [ UNASSIGNED-GeneralParameterError-RequiredParameter ] Object: VK_NULL_HANDLE (Type = 0) | vkCreateFramebuffer: required parameter pCreateInfo->renderPass specified as VK_NULL_HANDLE
·E 0:00:06.822794 {rsx::thread} RSX: ERROR: [Validation] Code 155364865 : [ VUID-VkFramebufferCreateInfo-renderPass-parameter ] Object: VK_NULL_HANDLE (Type = 18) | Invalid RenderPass Object 0x0. The spec valid usage text states 'renderPass must be a valid VkRenderPass handle' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkFramebufferCreateInfo-renderPass-parameter)
·E 0:00:06.888927 {rsx::thread} RSX: ERROR: [Validation] Code -1 : [ UNASSIGNED-GeneralParameterError-RequiredParameter ] Object: VK_NULL_HANDLE (Type = 0) | vkCreateGraphicsPipelines: required parameter pCreateInfos[0].renderPass specified as VK_NULL_HANDLE
·E 0:00:06.888962 {rsx::thread} RSX: ERROR: [Validation] Code 157462017 : [ VUID-VkGraphicsPipelineCreateInfo-renderPass-parameter ] Object: VK_NULL_HANDLE (Type = 18) | Invalid RenderPass Object 0x0. The spec valid usage text states 'renderPass must be a valid VkRenderPass handle' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkGraphicsPipelineCreateInfo-renderPass-parameter)

@kvark

This comment has been minimized.

Copy link
Contributor Author

kvark commented Sep 11, 2018

@MSuih thanks for the info!

@kd-11 also note that precompute_render_pass was already returning nullptr in some cases (no color or depth buffers), so my PR doesn't introduce the case where a render pass is not created, it just makes this case happen more often, legally.

@kvark kvark force-pushed the kvark:vk-pass-check branch from 9a93601 to dc98412 Sep 11, 2018

@kvark

This comment has been minimized.

Copy link
Contributor Author

kvark commented Sep 11, 2018

I updated the changes now:

  • rebased and formatted
  • fixed the main problem of handing the VK_FORMAT_UNDEFINED that is tested for the depth/stencil
  • sprinkled exception throwing in all the places that try to access the render passes (second commit)

@kd-11 please take another look

VkResult support = vkGetPhysicalDeviceImageFormatProperties(gpu, color_format, VK_IMAGE_TYPE_2D, VK_IMAGE_TILING_OPTIMAL, VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT, 0, &props);
if (support != VK_SUCCESS)
{
assert(support == VK_ERROR_FORMAT_NOT_SUPPORTED);

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

verify(HERE), support == VK_ERROR_FORMAT_NOT_SUPPORTED;

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

Add an error message LOG_ERROR(RSX, "Format 0x%x is not supported for rendertarget usage by your GPU driver. Crashes may arise.", format);

VkResult support = vkGetPhysicalDeviceImageFormatProperties(gpu, depth_stencil_format, VK_IMAGE_TYPE_2D, VK_IMAGE_TILING_OPTIMAL, VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT, 0, &props);
if (depth_stencil_format != VK_FORMAT_UNDEFINED && support != VK_SUCCESS)
{
assert(support == VK_ERROR_FORMAT_NOT_SUPPORTED);

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

same as above

@@ -255,6 +255,11 @@ namespace vk

void init(vk::render_device &dev, VkRenderPass &render_pass)
{
if (render_pass == 0)

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

this is not useful

@@ -206,6 +206,11 @@ class VKProgramBuffer : public program_state_cache<VKTraits>
void add_pipeline_entry(RSXVertexProgram &vp, RSXFragmentProgram &fp, vk::pipeline_props &props, Args&& ...args)
{
props.render_pass = m_render_pass_data[props.render_pass_location];
if (props.render_pass == 0)

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

given how pointless the render pass id is, just replace these with a verify("Usupported renderpass configuration" HERE), render_pass_id != VK_NULL_HANDLE;

@@ -2699,7 +2720,8 @@ void VKGSRender::prepare_rtts(rsx::framebuffer_creation_context context)
}

auto vk_depth_format = (layout.zeta_address == 0) ? VK_FORMAT_UNDEFINED : vk::get_compatible_depth_surface_format(m_device->get_formats_support(), layout.depth_format);
m_current_renderpass_id = vk::get_render_pass_location(vk::get_compatible_surface_format(layout.color_format).first, vk_depth_format, (u8)m_draw_buffers.size());
auto vk_color_format = vk::get_compatible_surface_format(layout.color_format).first;

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

why is this necessary?

This comment has been minimized.

Copy link
@kvark

kvark Sep 11, 2018

Author Contributor

moving this out as a local variable allowed me to use it in the exception text, as well as arguably makes the code more readable

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

makes sense for the variable usage, but I still think a helper function is better suited for that.

@@ -1137,6 +1149,11 @@ void VKGSRender::end()
// TODO: Partial memory transfer
auto rp = vk::get_render_pass_location(VK_FORMAT_UNDEFINED, ds->info.format, 0);
auto render_pass = m_render_passes[rp];
if (render_pass == 0)

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

I feel all these can be simplified by using a helper function in the anonymous namespace that has the precomputed passes func to avoid duplication all over the place. Declare as inline ofc.

@kd-11

This comment has been minimized.

Copy link
Contributor

kd-11 commented Sep 11, 2018

note that precompute_render_pass was already returning nullptr in some cases

This should not have been an issue since there is no renderpass configuration with no color or depth buffers. Any such configuration sets the framebuffer_status_invalid flag and no rendering takes place. This is true for both OGL and Vulkan.

@@ -255,6 +255,11 @@ namespace vk

void init(vk::render_device &dev, VkRenderPass &render_pass)
{
if (render_pass == 0)

This comment has been minimized.

Copy link
@kd-11

kd-11 Sep 11, 2018

Contributor

We use BGRA8_UNORM for backbuffer surfaces, and the text-out pipe writes directly to it. If you intend to keep this as a debug assertion for documentation reasons just simplify this to a one line verify(HERE) renderpass != VK_NULL_HANDLE; since its technically impossible to hit this assert.

@kvark kvark force-pushed the kvark:vk-pass-check branch from dc98412 to f493d48 Sep 11, 2018

@kvark

This comment has been minimized.

Copy link
Contributor Author

kvark commented Sep 11, 2018

@kd-11 thanks for the quick review! I addressed all the notes except for the helper method.

@kvark kvark force-pushed the kvark:vk-pass-check branch from f493d48 to f266f21 Sep 11, 2018

@kd-11

kd-11 approved these changes Sep 11, 2018

Copy link
Contributor

kd-11 left a comment

Good to merge once @MSuih confirms his crash issue is resolved.

@MSuih

This comment has been minimized.

Copy link
Contributor

MSuih commented Sep 11, 2018

Did a couple of quick test runs and coudn't trigger any crashes.

@MSuih

MSuih approved these changes Sep 11, 2018

@kd-11 kd-11 merged commit 7c4693e into RPCS3:master Sep 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kvark kvark deleted the kvark:vk-pass-check branch Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.