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

[Impeller] Fix render pass layout transition when resolve texture has mip levels. #57211

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jonahwilliams
Copy link
Member

This fixed the validation error produced in 159876. Not clear yet if it fixes the actual rendering error.

);
TextureVK::Cast(*attachment.texture)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the render pass is cached and reused, these calls to SetLayoutWithoutEncoding were only running the first time. I moved them into the constructor so they correctly run each time.

This could be the cause of other bugs, as it could leave our own book keeping out of sync with the actual image layout.

@@ -24,8 +24,6 @@
#include "impeller/renderer/backend/vulkan/sampler_vk.h"
#include "impeller/renderer/backend/vulkan/shared_object_vk.h"
#include "impeller/renderer/backend/vulkan/texture_vk.h"
#include "vulkan/vulkan.hpp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect includes.

@@ -12,7 +12,6 @@
#include "impeller/renderer/command_buffer.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/render_target.h"
#include "vulkan/vulkan_handles.hpp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect includes.

@@ -202,6 +193,30 @@ RenderPassVK::RenderPassVK(const std::shared_ptr<const Context>& context,
pass_info.setPClearValues(clears.data());
pass_info.setClearValueCount(clear_count);

if (resolve_image_vk_) {
// If the resolve image has mip levels, only mip level 0 will be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not make the pass perform the transition for all levels? In fact, I already think it does. The only way it may not is if the image descriptor doesn't contain the right mip count.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That value can also be VK_REMAINING_MIP_LEVELS. But again, I suppose I don't know how we could only perform the transition for some levels.

@chinmaygarde
Copy link
Member

chinmaygarde commented Dec 16, 2024

I think I am missing something because I don't understand the underlying issue.

@jonahwilliams
Copy link
Member Author

@chinmaygarde I was planning to ask you about this. I think this might be something like a bug in the validation layers: when the render pass transitions mip level 0 of a texture to eGeneral, the remaining mipLevels are left in eUndefined.

Its possible since the ImageView that the render pass uses only has one mip level this is WAI though, and the other mip levels must be transitioned manually.

But it might not matter at all since we're going to blow away all the mip level content by populating them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants