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.

Sign up for GitHub

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

Jump to bottom

Add support for VK_EXT_layer_settings extension #2095

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Add support for VK_EXT_layer_settings extension #2095

merged 5 commits into from
Dec 19, 2023

Conversation

billhollings
Copy link
Contributor

@billhollings billhollings commented Dec 13, 2023 •
edited
Loading

  • Add support for VK_EXT_layer_settings extension.
  • Support per-VkInstance configuration.
  • Add MoltenVK_Configuration_Parameters.md to document the MoltenVK configuration parameters.
  • Deprecate vkSetMoltenVKConfigurationMVK().
  • Deprecate mvk_config.h and move content to mvk_private_api.h and mvk_deprecated_api.h.

Reviewers...

There are a lot of documentation changes, especially the addition of MoltenVK_Configuration_Parameters.md, which takes a lot of the old documentation that was in mvk_config.h and recasts it into name-value pairs, instead of struct members. I've gone over it a few times, so if you want to proof-read it in detail, feel free, but that is probably not necessary at this point.

@billhollings billhollings changed the title Vk ext layer settings Add support for VK_EXT_layer_settings extension Dec 13, 2023
cdavis5e
cdavis5e requested changes Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Of course I reviewed the doc anyway.

Also, one other issue with the runcts script.

Docs/MoltenVK_Configuration_Parameters.md Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/GPUObjects/MVKDevice.h Outdated Show resolved Hide resolved
Scripts/runcts Outdated Show resolved Hide resolved
@billhollings
Copy link
Contributor Author

Of course I reviewed the doc anyway.

Thanks for your diligence! Fixes made.

juan-lunarg
juan-lunarg reviewed Dec 14, 2023
View reviewed changes
Copy link

@juan-lunarg juan-lunarg left a comment

Choose a reason for hiding this comment

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

Do you have a sample application you tested with?

May be worth updating Vulkan-Samples with a common use case so users become aware of the new functionality.

MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm Show resolved Hide resolved
@cdavis5e
Copy link
Collaborator

When you updated the branch, nothing changed. I don't see the fixes you made.

@billhollings
Update documentation for the VK_EXT_layer_settings extension.
0fc9657 
- Add MoltenVK_Configuration_Parameters.md to
  document the MoltenVK configuration parameters.
- Deprecate vkSetMoltenVKConfigurationMVK().
- Deprecate mvk_config.h and move content to mvk_private_api.h and mvk_deprecated_api.h.
- Streamline lock on retrieval of MVKLayerManager singleton (unrelated).
@billhollings
Copy link
Contributor Author

Do you have a sample application you tested with?

Not at this point. I just modified the Cube Demo to test this with some settings, including Bools, UInts, and Strings.

May be worth updating Vulkan-Samples with a common use case so users become aware of the new functionality.

I agree. We can bring this up on the next portability call.

@billhollings
Copy link
Contributor Author

When you updated the branch, nothing changed. I don't see the fixes you made.

Yeah...sorry. I brain farted and amended the commit before adding the changes. It's complete now.

cdavis5e
cdavis5e requested changes Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Sorry, but I just realized one more thing...

MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm Show resolved Hide resolved
@juan-lunarg
Copy link

I agree. We can bring this up on the next portability call.

👍🏾

@juan-lunarg
Copy link

juan-lunarg commented Dec 15, 2023 •
edited
Loading

+ @christophe-lunarg for their thoughts. Since this is the first time VK_EXT_layer_settings will be used on a driver. All the other use cases have been done with Vulkan layers (validation layers, extension layers, etc).

juan-lunarg
juan-lunarg previously approved these changes Dec 18, 2023
View reviewed changes
cdavis5e
cdavis5e requested changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@cdavis5e cdavis5e left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know this has been through a lot of back-and-forth, but please bear with me for just a little bit longer...

MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp Outdated Show resolved Hide resolved
MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp Outdated Show resolved Hide resolved
@billhollings
Fixes from dev review feedback for adding VK_EXT_layer_settings exten…
1ce40f6 
…sion.

- Document the name of the MoltenVK driver layer.
- Support future multiple string members in MVKConfiguration.
- Add static assert on number of string members in MVKConfigruation.
- Rename global mvkConfig() to getGlobalMVKConfig().
- Rename global mvkSetConfig() to mvkSetGlobalConfig().
- Remove unused mvkPrintSizeOf() macro. (unrelated).
- Trim trailing spaces from Markdown documents because
  it causes double-spaces in some Markdown readers (unrelated).
@juan-lunarg juan-lunarg dismissed their stale review December 19, 2023 17:27

I'd prefer to wait on approval from Christophe. Since there is no rush on this.

cdavis5e
cdavis5e approved these changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

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

Just one more minor thing.

MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp Outdated Show resolved Hide resolved
@billhollings @cdavis5e
Update MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp
6daa6c0 
Co-authored-by: Chip Davis <cdavis5x@gmail.com>
@billhollings billhollings merged commit 76233bc into KhronosGroup:main Dec 19, 2023
6 checks passed
@billhollings billhollings deleted the VK_EXT_layer_settings branch December 19, 2023 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophe-lunarg christophe-lunarg christophe-lunarg left review comments

@cdavis5e cdavis5e cdavis5e approved these changes

@juan-lunarg juan-lunarg juan-lunarg left review comments

@richard-lunarg richard-lunarg Awaiting requested review from richard-lunarg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@billhollings @cdavis5e @juan-lunarg @christophe-lunarg