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 used resolutions info #1167

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add used resolutions info #1167

wants to merge 2 commits into from

Conversation

bartosz-muszarski-arm
Copy link
Contributor

gfxrecon-info reports resolutions used in vulkan traces based on data provided in vkCreateSwapchainKHR and vkCreateSharedSwapchainsKHR calls.

Sample output:

Exe info:
        Application exe name: 
        Application version: 0.0.0.0
        Application Company name: 
        Product name: 
File info:
        Compression format: LZ4
        Total frames: 11

Application info:
        Application name: ray_queries
        Application version: 0
        Engine name: Vulkan Samples
        Engine version: 0
        Target API version: 4198400 (1.1.0)
        Used resolutions: 1280x720

Physical device info:
        Device name: NVIDIA GeForce RTX 2060
        Device ID: 0x1e89
        Vendor ID: 0x10de
        Driver version: 2181955968 (0x820e0180)
        API version: 4206797 (1.3.205)

Device memory allocation info:
        Total allocations: 12
        Min allocation size: 3932160
        Max allocation size: 134217728

Pipeline info:
        Total graphics pipelines: 2
        Total compute pipelines: 0

@bartosz-muszarski-arm
Add report on used resolutions
8758725 
gfxrecon-info reports resolutions used in vulkan traces based on
data provided in vkCreateSwapchainKHR and vkCreateSharedSwapchainsKHR
calls.

Change-Id: I91118d1c7749662765d45e180c61937dd0297816
@ci-tester-lunarg
Copy link

Author bartosz-muszarski-arm not on autobuild list. Waiting for curator authorization before starting CI build.

@bradgrantham-lunarg
Copy link
Contributor

Could this be accomplished by a script parsing the output of convert?

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 2227.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2896 running.

@bradgrantham-lunarg
Copy link
Contributor

As an example, here's a jq line that extracts the imageExtent dimensions from vkCreateSwapchainKHR.

$ jq-win64.exe '.[] | select(.function.name == "vkCreateSwapchainKHR") | [.function.args.pCreateInfo.imageExtent.width,.function.args.pCreateInfo.imageExtent.height]' < gfxrecon_capture_20230428T153938.json
[
  500,
  500
]

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2896 passed.

@bartosz-muszarski-arm
Copy link
Contributor Author

Yes, the same data can be generated from a json-converted trace. I would argue this is still more convenient to use on a daily basis. We use the resolution information along with application name and total frame count which we get from gfxrecon-info already.

@bradgrantham-lunarg bradgrantham-lunarg mentioned this pull request Jun 29, 2023
Merged
bradgrantham-lunarg
bradgrantham-lunarg requested changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

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

Looks good to me except if there are no swapchains created (e.g. vulkaninfo) then the output is Used resolutions and then nothing else. I think there should be no output if resolution.size() == 0.

@bradgrantham-lunarg
Copy link
Contributor

Yes, the same data can be generated from a json-converted trace. I would argue this is still more convenient to use on a daily basis. We use the resolution information along with application name and total frame count which we get from gfxrecon-info already.

Okay, sounds good to me. I added a note about empty resolutions but with that change we'll merge.

@bartosz-muszarski-arm
Remove resolution info output if none was found
85db555 
Change-Id: If09c70784503e4344bab2a3294245505027874ab
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 6518.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2983 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 2983 failed.

bradgrantham-lunarg
bradgrantham-lunarg requested changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

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

I had the one thought about vulkan.hpp.

Also your branch is failing in our internal CI because it's missing commit 32496a2 committed to dev on June 29. If you rebase on current dev I think it will probably pass our internal CI. Sorry, I think these are the last two items; thanks for being patient and sticking with us!

framework/decode/vulkan_stats_consumer.h
@@ -30,11 +30,14 @@
#include "generated/generated_vulkan_struct_decoders.h"
#include "generated/generated_vulkan_consumer.h"
#include "util/defines.h"
#include "vulkan/vulkan.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't catch this on first pass. This would be the first inclusion of VulkanHpp and I want to be a little more deliberate about adding it. Can you just use VkExtent2D?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bartosz-muszarski-arm if you haven't done this already I see how it smooths out the feature you added and I'm not in principle against it. I'm doing a straw poll to see if anyone has any issues with including VulkanHpp.

Copy link
Contributor

@andrew-lunarg andrew-lunarg Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could we at least reduce this to just including the structs header and not taking in the entire C++ wrapper?

#include <vulkan/vulkan_structs.hpp>

EDIT: vulkan_hash.hpp included on the next line includes vulkan.hpp anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

And also turn off every feature we don't need in the headers that has a macro that defaults to enabled. e.g. in our CMake configs for all projects define VULKAN_HPP_NO_STRUCT_SETTERS. We can then turn stuff on when we need it, where we need it.

andrew-lunarg
andrew-lunarg reviewed Jul 17, 2023
View reviewed changes
framework/decode/vulkan_stats_consumer.h
@@ -57,6 +60,7 @@ class VulkanStatsConsumer : public gfxrecon::decode::VulkanConsumer, public gfxr
uint64_t GetMaxAllocationSize() const { return max_allocation_size_; }
uint64_t GetAnnotationCount() const { return annotation_count_; }
const std::vector<std::string>& GetOperationAnnotationDatas() const { return operation_annotation_datas_; }
const auto& GetResolutions() const { return resolutions_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of auto here where it forces the future contributors to scroll down the file to know what the function they might want to call actually returns (std::unordered_set<vk::Extent2D>).

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 10777.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3020 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3020 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradgrantham-lunarg bradgrantham-lunarg bradgrantham-lunarg requested changes

@andrew-lunarg andrew-lunarg andrew-lunarg left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@bartosz-muszarski-arm @ci-tester-lunarg @bradgrantham-lunarg @andrew-lunarg