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

Vulkan support in the Embedder API #29391

Merged
merged 60 commits into from
Feb 2, 2022
Merged

Conversation

bdero
Copy link
Member

@bdero bdero commented Oct 28, 2021

Please take a look and see if the embedder.h API looks like it works for your use case. /cc @jwinarske @arbreng @chinmaygarde

See FlutterVulkanBackingStore.image and FlutterVulkanRendererConfig.get_next_image_callback/present_image_callback for a full description of the default synchronization behavior for both the layer and root image writing cases respectively (the behavior is essentially the same).

My thought is that by default, we can simply have the engine internally host sync (with VkFences) before calling "present" callbacks. This matches the Fuchsia embedder's current behavior.

Later on, we can enable embedders to take full control over synchronization by adding an optional FlutterVulkanImage.semaphore field that, when present, will trigger the engine to automatically disable the default fence+present callback behavior, and the engine will instead signal the semaphore on device after pipeline execution. If the embedder provides a semaphore, the embedder can then submit pipeline commands ahead of time that will start executing on device immediately after the engine is done writing the image, eliminating an unnecessary device->host sync + host->device queue submit from the rendering time in cases where it's not desirable to render directly into swapchain images.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-android labels Oct 28, 2021
@google-cla google-cla bot added the cla: yes label Oct 28, 2021
Copy link
Contributor

@arbreng arbreng left a comment

Choose a reason for hiding this comment

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

embedder.h LGTM so far :)

const FlutterVulkanImage* /* image */);
typedef uint64_t* (*Uint64Callback)(void* /* user data */);

typedef uint64_t* (*FlutterVulkanPresentImageCallback)(void* /* user data */,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, removed this.

FlutterVulkanPhysicalDeviceHandle physical_device;
// VkDevice handle.
FlutterVulkanDeviceHandle device;
ProcResolver get_proc_address_callback;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is "ProcResolver" defined? How is this one different from "get_instance_proc_address_callback"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this signature is used in the GL renderer. I think this was supposed to be instance-less vkGetInstanceProcAddr in the doc, but on second thought I don't see a reason why this would be useful as a separate callback. Removed.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Nov 11, 2021
@bdero bdero force-pushed the bdero/vulkan-embedder branch 3 times, most recently from 62671fa to f3ecb6e Compare November 14, 2021 07:18
@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:13
@arbreng
Copy link
Contributor

arbreng commented Nov 16, 2021

Is this in a state where I could try testing it w/ Fuchsia code -- IE try to get Fuchsia to use EmbedderSurfaceVulkan?

@bdero bdero changed the base branch from master to main November 17, 2021 12:43
@bdero
Copy link
Member Author

bdero commented Nov 17, 2021

I think this is about ready for trying out through the Embedder API surface itself (compositor and all), but the build's broken right now due to test harness stuff; I'll be fixing up the build later today

@chinmaygarde chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Nov 19, 2021
@chinmaygarde chinmaygarde changed the title WIP: Vulkan support in the Embedder API Vulkan support in the Embedder API Nov 19, 2021
@jwinarske
Copy link
Contributor

@bdero thanks for your hard work on this. I will start working with this on Monday.

@bdero bdero force-pushed the bdero/vulkan-embedder branch 2 times, most recently from 8b91867 to 55f571a Compare November 20, 2021 05:04
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021
…) to add missing implementations in SubzeroReactor.cpp.

Corresponding buildroot PR: flutter/buildroot#529

Needed for flutter#29391.
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021
…) to add missing implementations in SubzeroReactor.cpp.

Corresponding buildroot PR: flutter/buildroot#529

Needed for flutter#29391.

(cherry picked from commit dca7211)
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021
…) to add missing implementations in SubzeroReactor.cpp.

Corresponding buildroot PR: flutter/buildroot#529

Needed for flutter#29391.
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021
…) to add missing implementations in SubzeroReactor.cpp.

Corresponding buildroot PR: flutter/buildroot#529

Needed for flutter#29391.
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 2, 2021
…) to add missing implementations in SubzeroReactor.cpp.

Corresponding buildroot PR: flutter/buildroot#529

Needed for flutter#29391.
…sions; don't enable get_memory_requirements_2 automatically
… ensure it can never outlive the vulkan device & instance
This reverts commit 01dee39.
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.

6 participants