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

Added support for Layer Interface Version 0 in the pipeline runtime layer #157

Merged
merged 9 commits into from
Jun 12, 2023

Conversation

mdagois
Copy link
Contributor

@mdagois mdagois commented Jun 8, 2023

Layer Interface Version 0 support + Android in the pipeline runtime layer

  • Added support for Layer Interface Version 0, which is required for Android
  • Fixed compilation errors under Android

@mdagois mdagois marked this pull request as ready for review June 8, 2023 07:42
Copy link
Contributor

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Thanks for improving this, @mdagois!

This seems to only update the runtime layers -- did you omit the other layers intentionally? If so, could you update the PR description and title to mention that this is for the runtime layer only?

layer/support/trace_event_logging.cc Outdated Show resolved Hide resolved
Comment on lines 36 to 39
#define LAYER_NAME "VK_LAYER_STADIA_pipeline_runtime"

constexpr uint32_t kRuntimeLayerVersion = 1;
constexpr char kLayerName[] = "VK_LAYER_STADIA_pipeline_runtime";
constexpr char kLayerName[] = LAYER_NAME;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this define? I don't see it being used anywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry, I added the LAYER_NAME define and forgot to actually use it.

LAYER_NAME is now also used to build the entry point function names for Layer Interface Version 0 (see LAYER_NAME_FUNCTION macro).
I realized there were several issues around those macros, so I fixed them.
It requires a lot of macro gymnastics, but the intent is to have the layer name defined in one single place, because
if we change the layer name later and forget to update the entry point names, there won't be any compilation error, but the layer won't work on Android anymore.

I hope that makes sense.

@mdagois mdagois changed the title Added support for Layer Interface Version 0 Added support for Layer Interface Version 0 in the pipeline runtime layer Jun 9, 2023
@mdagois
Copy link
Contributor Author

mdagois commented Jun 9, 2023

This seems to only update the runtime layers -- did you omit the other layers intentionally? If so, could you update the PR description and title to mention that this is for the runtime layer only?

Yes, it was intentional.
My intent is to eventually add support to all layers once we get that one fully working.
I updated the description and title of the PR.

layer/runtime/runtime_layer.cc Outdated Show resolved Hide resolved
layer/support/trace_event_logging.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

LGTM

@kuhar kuhar merged commit 9b3d9d2 into google:main Jun 12, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants