-
Notifications
You must be signed in to change notification settings - Fork 327
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
Pipe validation output to report view #1931
Conversation
gapis/api/vulkan/find_issues.go
Outdated
// TODO: For MEC, we need to minus the number of initial commands | ||
// TODO: Figure out what to do for commands inserted by GAPID, whose label is ~0. | ||
issue.Command = api.CmdID(n.GetLabel()) | ||
issue.Severity = service.Severity(uint32(n.GetSeverity())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a better way to enforce this? I found in gapis/service/service.proto, and core/log, we also just note in the comments to say their value should be the same.
} | ||
pInfo->enabledLayerCount = layer_count; | ||
if (!has_debug_report && addValidationLayer) { | ||
exts[ext_count++] = debugReportName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VK_EXT_debug_report is an instance extension so we should not enable it for device. This code is just added here to test the fallback route. Will be removed later in this PR.
7df78e1
to
86d1a89
Compare
core/log/log_pb/BUILD.bazel
Outdated
@@ -51,3 +51,9 @@ java_proto_library( | |||
visibility = ["//visibility:public"], | |||
deps = [":log_pb_proto"], | |||
) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used, remove this.
@@ -115,7 +115,7 @@ VKAPI_ATTR VkResult VKAPI_CALL vkGetPhysicalDeviceSurfaceCapabilitiesKHR( | |||
// TODO(awoloszyn): Handle all of the composite types. | |||
|
|||
pSurfaceCapabilities->supportedUsageFlags = | |||
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT; | |||
VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT|VK_IMAGE_USAGE_TRANSFER_SRC_BIT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AWoloszyn . Seems like we should have this usage bit set for the images created in our virtual swapchain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that is fair, although PROBABLY should be another CL.
gapir/cc/context.h
Outdated
@@ -134,6 +134,9 @@ class Context : private Renderer::Listener { | |||
// The currently running interpreter. | |||
// Only valid for the duration of interpret() | |||
std::unique_ptr<Interpreter> mInterpreter; | |||
|
|||
// total number of notifications issued by this context | |||
uint64_t mNumNotifications; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to:
// The total number of debug messages sent to GAPIS by this context.
uint64_t mNumSentDebugMessages
gapir/cc/mock_replay_connection.h
Outdated
@@ -41,6 +41,9 @@ class MockReplayConnection : public ReplayConnection { | |||
return mockedSendPostData(posts.get()); | |||
} | |||
// TODO: mock sendNotification and test it once it is used. | |||
MOCK_METHOD7(sendNotification, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests?
gapir/cc/vulkan_gfx_api.inc
Outdated
@@ -36,11 +36,17 @@ IndirectMaps mIndirectMaps; | |||
|
|||
// Function for wrapping around the normal vkCreateInstance to inject | |||
// virtual swapchain as an additional enabled layer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update blurbs to take validation layer/debug report extension into account.
gapis/api/vulkan/find_issues.go
Outdated
debugReportExtension = "VK_EXT_debug_report" | ||
) | ||
|
||
func isValidationLayer(n string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blurbs
gapis/replay/batch.go
Outdated
@@ -164,8 +164,9 @@ func (m *Manager) execute( | |||
} | |||
|
|||
var payload gapir.Payload | |||
var decoder builder.ResponseDecoder | |||
builderBuildTimer.Time(func() { payload, decoder, err = b.Build(ctx) }) | |||
var postResp builder.PostDataResponsor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responsor? Responser? Receiver? Consumer?
gapis/replay/builder/builder.go
Outdated
// machine. | ||
type NotificationResponsor func(p *gapir.Notification) | ||
|
||
type ReadNotification func(p gapir.Notification) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blurb
gapis/replay/builder/builder.go
Outdated
@@ -566,10 +573,14 @@ func (b *Builder) Write(rng memory.Range, resourceID id.ID) { | |||
b.ReserveMemory(rng) | |||
} | |||
|
|||
func (b *Builder) RegisterNotificationReader(reader ReadNotification) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blurb
tools/build/rules/grpc_c++.bzl
Outdated
else: | ||
_generate_cc(**kwargs) | ||
|
||
def cc_grpc_library(name, proto, dep_cc_proto_libs, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc_grpc_library
is overwritten here (together with generate_cc to avoid dependency issue) because the gRPC original one depends on their in-house defined 'intermediate' targets when generating gRPC code for multiple proto files, makes the rule not good for external use, e.g. inferencing the original proto_library target names from proto-generated cpp file group target, but that rule of inference is only followed in gRPC repository.
@AWoloszyn This CL is ready for review |
@@ -86,12 +88,14 @@ message PostData { | |||
// build time of the replay instruction. | |||
message Notification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general rule for protobuf is that you never re-number fields. You only add to the end.
However since we ship the client&server in one package, this won't actually be a problem.
) | ||
|
||
var validationLayers = [...]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: [...]
gapis/api/vulkan/find_issues.go
Outdated
for _, d := range validationLayersData { | ||
newCmd.AddRead(d.Data()) | ||
} | ||
newCmd.AddRead(debugReportExtNameData.Data()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think AddRead() returns the cmd.
newCmd.AddRead().AddRead().AddRead()....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2b9a068
gapis/api/vulkan/find_issues.go
Outdated
layersData := s.AllocDataOrPanic(ctx, layers) | ||
info.SetEnabledLayerCount(uint32(len(layers))) | ||
info.SetPpEnabledLayerNames(NewCharᶜᵖᶜᵖ(layersData.Ptr())) | ||
infoData := s.AllocDataOrPanic(ctx, info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to clean this up somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2b9a068.
Similar to read_framebuffer.go, the memory is cleaned after at the return of find_issue's Transform call. It is one-command later then freeing just after the call to mutate() for vkCreateInstance
, because I inserted a vkCreateDebugReportCallbackEXT
after it. But I think it should be fine, using defer looks nicer than calling free() manually after each mutate().
gapis/api/vulkan/find_issues.go
Outdated
info.SetPpEnabledLayerNames(NewCharᶜᵖᶜᵖ(layersData.Ptr())) | ||
info.SetEnabledExtensionCount(uint32(len(exts))) | ||
info.SetPpEnabledExtensionNames(NewCharᶜᵖᶜᵖ(extsData.Ptr())) | ||
infoData := s.AllocDataOrPanic(ctx, info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to clean this up somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2b9a068
gapis/api/vulkan/find_issues.go
Outdated
} | ||
return false | ||
})) | ||
callbackHandleData := s.AllocDataOrPanic(ctx, callbackHandle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to clean these up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2b9a068
gapir/cc/replay_connection.cpp
Outdated
const void* data, uint32_t data_size) { | ||
using severity::Severity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Severity logLevels[] = {
Severity::FatalLevel,
Severity::ErrorLevel,
...
}
if (severity <= LOG_LEVEL_DEBUG) {
sev = logLevels[severity];
}
gapir/cc/replay_connection.h
Outdated
virtual bool sendNotification(uint64_t id, uint32_t api_index, uint64_t label, | ||
const std::string& msg, const void* data, | ||
uint32_t data_size); | ||
virtual bool sendNotification(uint64_t id, int severity, uint32_t api_index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make severity unsigned? Is there any reason it can ever be negative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to uint32_t in c0e75da
@@ -110,6 +110,156 @@ def grpc_cc_library(name, srcs = [], public_hdrs = [], hdrs = [], | |||
alwayslink = alwayslink, | |||
) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmuetschard Can you take a look at this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Only one minor comment, otherwise looks fine.
tools/build/rules/grpc_c++.bzl
Outdated
directly. | ||
""" | ||
|
||
def generate_cc_impl(ctx): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names that start with an underscore are considered private. So if you don't want functions to be used outside of this file, name them _foo
. This is typically always done for rule implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 4caa404
@@ -110,6 +110,156 @@ def grpc_cc_library(name, srcs = [], public_hdrs = [], hdrs = [], | |||
alwayslink = alwayslink, | |||
) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Only one minor comment, otherwise looks fine.
For initial commands, their IDs are assigned to 0, and linearized ID (the label) will be specified in the message. GAPIS and GAPIR use same Severity level, which is extracted from gapis/service/service.proto. Some validation errors relevant to the virtual swapchain are deferred to next CL to fix - [ ] vkGetPhysicalDeviceSurfaceFormatsKHR, 'count' arg not match - [ ] vkGetPhysicalDeviceSurfacePresentModesKHR, 'count' arg not match - [ ] vkCreateSwapChainKHR, 'imageFormat' does not match with 'colorSpace' member
4caa404
to
e638000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash before submitting.
This CL does not package Vulkan validation layers in GAPID. It only pipes the output validation layers to the report view, if the validation layers are present and can be used to create instance.
For initial commands, their IDs are assigned to 0, and linearized ID
(the label) will be specified in the message.
GAPIS and GAPIR use same Severity level, which is extracted from
gapis/service/service.proto.
Some validation errors relevant to the virtual swapchain are deferred to
next CL to fix