[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

jonahwilliams · 2023-11-28T16:39:36Z

Support framebuffer fetch on devices that have the extension VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS which gives us a fairly easy way to add subpass self dependencies.

Part of flutter/flutter#120223

impeller/compiler/reflector.cc

jonahwilliams · 2023-11-28T16:39:57Z

impeller/renderer/backend/vulkan/binding_helpers_vk.cc

+
+      vk::WriteDescriptorSet write_set;
+      write_set.dstSet = descriptor_sets[desc_index];
+      // MAGIC! see description in pipeline_library_vk.cc


Maybe just look this up on the descriptor sets.

because the subpass doesn't show up in host builds I can't use any of the constants.

impeller/renderer/backend/vulkan/capabilities_vk.cc

impeller/renderer/backend/vulkan/capabilities_vk.h

…rder_attachment_access

dnfield · 2023-11-29T16:41:39Z

impeller/entity/contents/content_context.h

@@ -713,13 +713,15 @@ class ContentContext {

    void CreateDefault(const Context& context,
                       const ContentContextOptions& options,
-                       const std::initializer_list<int32_t>& constants = {}) {
+                       const std::initializer_list<int32_t>& constants = {},
+                       bool has_subpass_input = false) {


Consider using an enum instead of a bool so that this reads more nicely at the callsites, e.g. enum class UseSubpassInput { kYes, kNo, };

One of the nice parts of clangd is that it does this for me 😸

Alternatively use the google-style argument comments, but I prefer the enum. I don't have clangd when I'mr eading the callsites in your patch :D

I think this is why objective-c has YES and NO constants 😄

dnfield · 2023-11-29T16:43:29Z

impeller/renderer/backend/vulkan/capabilities_vk.h

@@ -110,6 +111,8 @@ class CapabilitiesVK final : public Capabilities,
  vk::PhysicalDeviceProperties device_properties_;
  bool supports_compute_subgroups_ = false;
  bool supports_device_transient_textures_ = false;
+  bool supports_framebuffer_fetch_ = false;
+  ;


dnfield · 2023-11-29T16:46:58Z

impeller/renderer/backend/vulkan/pipeline_library_vk.cc

+    // header generated from a shader compiled with Vulkan semantics will have
+    // this binding, but today its unspecified if we use the GLES or Metal
+    // version based on host platform. To that end, it would be safer to leave
+    // this as a hardcoded value until such a time as the bindings are fixed.


I'm running into a similar issue looking at the fragment program API.

For this specific case, I think we could add another value to the generated header for this and only use it when we're in the Vulkan backend, right? IOW, if generated with vulkan semantics, the number is "real", otherwise it's just a placeholder.

However, I think to do runtime stage multi-plat right we'll want to have multiple compiler backends producing output. That can probably be limited just to runtime stage stuff, but maybe it's better to extend it to all the platforms.

The problem is that subpass input is a vulkan only concept. if the header is GLSL or Metal generated, you won't have it. Which generated header do we use? It doesn't seem well defined. FWIW I will make the compiler output this value and see if it happens to work, but it still feels like a landmine.

We need different headers per platform/driver

This is the same problem that has prevented the usage of half precision on Vulkan.

dnfield

Seems pretty good, mostly nits, if we can it'd be nice to add the change for impellerc in this patch rather than relying on a hard coded constant that we'll forget to update when we add/remove shaders.

jonahwilliams · 2023-11-30T02:09:51Z

impeller/entity/shaders/blending/framebuffer_blend.frag

+#else
+layout(set = 0,
+       binding = 0,
+       input_attachment_index = 0) uniform subpassInputMS uSub;


😮‍💨

impeller/renderer/backend/vulkan/render_pass_vk.cc

jonahwilliams · 2023-11-30T16:05:13Z

impeller/renderer/backend/vulkan/render_pass_vk.cc

-    color_refs[bind_point] =
-        vk::AttachmentReference{static_cast<uint32_t>(attachments.size()),
-                                vk::ImageLayout::eColorAttachmentOptimal};
+    color_refs[bind_point] = vk::AttachmentReference{


note: changed to general, investigating.

We need the state to be in general layout to do framebuffer fetch

jonahwilliams · 2023-12-01T20:03:43Z

I've added some trivial tests for this, but @dnfield @matanlurey I'm curious to get your thoughts on real tests. None of the engine presubmit infra has support for this feature, but the devicelab phones do - and we have a benchmark that runs a specific advanced blend benchmark I'm interested in improving.

But verifying correctness of the image layouts/transitions beyond running with validation layers, I don't have any ideas there

chinmaygarde

Small nits and my previous attempt to do this should offer some clues if you care to handle them. Otherwise lgtm. We can file followups if needed.

chinmaygarde · 2023-12-02T00:15:47Z

impeller/compiler/code_gen_template.h

+  static constexpr std::array<DescriptorSetLayout,{{length(buffers)+length(sampled_images)+length(subpass_inputs)}}> kDescriptorSetLayouts{
+{% for subpass_input in subpass_inputs %}
+    DescriptorSetLayout{
+      {{subpass_input.binding}}, // binding = {{subpass_input.binding}}


I think its a bit of a mistake to assume that all bindings are in the set index zero. We should fix that separately as that affects all instances of DescriptorSetLayout in the generated header. For example, if we were to manually specify layout(set=2, binding=n), we'd keel over.

In the same vein, I think its a mistake to assume that we can only have one input attachment and at index zero. I'd also put the input_attachment_index in the descriptor set layout. The previous attempt at this patch did the same.

You should be able to reproduce this by manually specifying the input_attachment_index to anything other than zero and checking the validity of the setup.

For the descriptor set index, we can file a separate followup issue.

I think practically speaking we can't really do bindings any other way. The actual render_pass_vk logic can only handle descriptors at index set zero.

Perhaps it would be possible with some refactoring, but it seems really in the weeds of what will yield a performance improvement.

chinmaygarde · 2023-12-02T00:19:35Z

impeller/entity/shaders/blending/framebuffer_blend.vert

@@ -5,6 +5,9 @@
 #include <impeller/conversions.glsl>
 #include <impeller/types.glsl>

+// Warning: if any of the constant values or layouts are changed in this
+// file, then the hard-coded constant value in
+// impeller/renderer/backend/vulkan/binding_helpers_vk.cc


Thinking out aloud on how to address this issue (in a separate patch later of course): We should be able to import a single header file from both a C++ TU and a glsl header. The same macro expansion capabilities as the C pre-processor are available in impellerc as well. As the IMPELLER_DEVICE macro is set in ImpellerC so we should be able to set it. So we can use that to ifdef away C++/GLSL-isms from the other TU.

I believe the default impellerc shaders GN rule set also add the header include paths during the impellerc invocation.

Just a nit; But I am just nervous about "please keep these in sync manually" type comments.

I think that won't work in this case, because its not that we're setting to binding 64 or w/e, that is just the first available binding in the fragment stage. So adding a new binding to the vertex stage might also change that? Not sure.

What I would really like is to have separate GLLS, Vulkan, and Metal headers. As this would also solve the problem of conditionally using float16.

jonahwilliams · 2023-12-05T18:50:39Z

I found Skia's code for setting up render passes which confirms that eGeneral is the correct state: https://github.com/google/skia/blob/b7ac9da3e5ac34baf5da70b3e37cdbfd898ed54b/src/gpu/ganesh/vk/GrVkRenderPass.cpp#L158-L163

jonahwilliams · 2023-12-05T22:39:15Z

I've added a test that should fail if we ever change the binding value, which isn't great but should at least protect us from accidentally breaking ourself (beyond the integration test in the framework repo)

…rder_attachment_access

…N_ORDER_ATTACHMENT_ACCESS (flutter/engine#48458)

…139612) flutter/engine@3e661e0...fe96317 2023-12-06 jonahwilliams@google.com [Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS (flutter/engine#48458) 2023-12-06 godofredoc@google.com Remove fuchsia v1 builder. (flutter/engine#48703) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#139612) flutter/engine@3e661e0...fe96317 2023-12-06 jonahwilliams@google.com [Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS (flutter/engine#48458) 2023-12-06 godofredoc@google.com Remove fuchsia v1 builder. (flutter/engine#48703) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] Vulkan framebuffer fetch

9180bfb

jonahwilliams commented Nov 28, 2023

View reviewed changes

impeller/compiler/reflector.cc Outdated Show resolved Hide resolved

github-actions bot added platform-android e: impeller labels Nov 28, 2023

jonahwilliams commented Nov 28, 2023

View reviewed changes

impeller/renderer/backend/vulkan/capabilities_vk.cc Outdated Show resolved Hide resolved

jonahwilliams commented Nov 28, 2023

View reviewed changes

impeller/renderer/backend/vulkan/capabilities_vk.h Show resolved Hide resolved

jonahwilliams added 5 commits November 28, 2023 13:40

adjustments

a9fd144

still ARM extension on ... ARM

6921c1b

Merge branch 'main' of github.com:flutter/engine into rasterization_o…

5c395cb

…rder_attachment_access

remove command flag.

b6cb2cb

++

1869497

dnfield reviewed Nov 29, 2023

View reviewed changes

jonahwilliams added 3 commits November 29, 2023 12:47

generate binding in template

b428f5f

Cleanups to layout and barriers.

808d257

enums instead of bool :/

4e79b47

jonahwilliams marked this pull request as ready for review November 29, 2023 21:23

subpassInputMS

a1153d9

jonahwilliams commented Nov 30, 2023

View reviewed changes

impeller/renderer/backend/vulkan/render_pass_vk.cc Outdated Show resolved Hide resolved

jonahwilliams added 3 commits November 29, 2023 19:31

more tweaks

76c4336

++

d5bff60

include shaders.

cef3581

jonahwilliams commented Nov 30, 2023

View reviewed changes

jonahwilliams added 3 commits November 30, 2023 12:42

WIP

371fbdd

testing adjustments.

55d111d

++

f5949b8

warning

8dfb942

chinmaygarde approved these changes Dec 2, 2023

View reviewed changes

chinmaygarde assigned jonahwilliams Dec 2, 2023

Merge branch 'main' into rasterization_order_attachment_access

da822f4

add test

4501eb3

jonahwilliams added 2 commits December 5, 2023 15:20

Merge branch 'main' of github.com:flutter/engine into rasterization_o…

7bf4115

…rder_attachment_access

++

b9ef69b

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2023

auto-submit bot merged commit fe96317 into flutter:main Dec 6, 2023
28 checks passed

jonahwilliams deleted the rasterization_order_attachment_access branch December 6, 2023 01:40

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 3e661e0a58cc to fe96317750ff (2 revisions) flutter/flutter#139612

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

fe9631775 [Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATIO…

a75925d

…N_ORDER_ATTACHMENT_ACCESS (flutter/engine#48458)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

jonahwilliams commented Nov 28, 2023 •

edited

jonahwilliams Nov 28, 2023

jonahwilliams Nov 29, 2023

jonahwilliams Dec 1, 2023

dnfield Nov 29, 2023

jonahwilliams Nov 29, 2023

dnfield Nov 29, 2023

jonahwilliams Nov 29, 2023 •

edited

dnfield Nov 29, 2023

dnfield Nov 29, 2023

jonahwilliams Nov 29, 2023

jonahwilliams Nov 29, 2023

dnfield left a comment

jonahwilliams Nov 30, 2023

jonahwilliams Nov 30, 2023

jonahwilliams Dec 5, 2023

jonahwilliams commented Dec 1, 2023

chinmaygarde left a comment

chinmaygarde Dec 2, 2023 •

edited

jonahwilliams Dec 2, 2023

chinmaygarde Dec 2, 2023

chinmaygarde Dec 2, 2023

chinmaygarde Dec 2, 2023

jonahwilliams Dec 2, 2023

jonahwilliams commented Dec 5, 2023

jonahwilliams commented Dec 5, 2023

[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

[Impeller] Vulkan framebuffer fetch via VK_ARM_RASTERIZATION_ORDER_ATTACHMENT_ACCESS #48458

Conversation

jonahwilliams commented Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams Nov 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 1, 2023

chinmaygarde left a comment

Choose a reason for hiding this comment

chinmaygarde Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Dec 5, 2023

jonahwilliams commented Dec 5, 2023

jonahwilliams commented Nov 28, 2023 •

edited

jonahwilliams Nov 29, 2023 •

edited

chinmaygarde Dec 2, 2023 •

edited