Pipe validation output to report view by qining · Pull Request #1931 · google/gapid

qining · 2018-05-30T11:56:47Z

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

vkGetPhysicalDeviceSurfaceFormatsKHR, 'count' arg not match
vkGetPhysicalDeviceSurfacePresentModesKHR, 'count' arg not match
vkCreateSwapChainKHR, 'imageFormat' does not match with 'colorSpace' member

qining · 2018-05-30T11:59:03Z

gapis/api/vulkan/find_issues.go

+				// 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()))


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.

qining · 2018-05-30T12:01:47Z

gapis/api/templates/vulkan_gfx_api_extras.tmpl

+    }
+    pInfo->enabledLayerCount = layer_count;
+    if (!has_debug_report && addValidationLayer) {
+      exts[ext_count++] = debugReportName;


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.

qining · 2018-06-04T14:50:29Z

core/log/log_pb/BUILD.bazel

    visibility = ["//visibility:public"],
    deps = [":log_pb_proto"],
 )
+


Not used, remove this.

qining · 2018-06-04T14:53:56Z

core/vulkan/vk_virtual_swapchain/cc/swapchain.cpp


  pSurfaceCapabilities->supportedUsageFlags =
-      VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
+      VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT|VK_IMAGE_USAGE_TRANSFER_SRC_BIT;


@AWoloszyn . Seems like we should have this usage bit set for the images created in our virtual swapchain.

Yep, that is fair, although PROBABLY should be another CL.

qining · 2018-06-04T14:57:14Z

gapir/cc/context.h

    std::unique_ptr<Interpreter> mInterpreter;
+
+    // total number of notifications issued by this context
+    uint64_t mNumNotifications;


Change to:

// The total number of debug messages sent to GAPIS by this context. uint64_t mNumSentDebugMessages

qining · 2018-06-04T14:57:39Z

gapir/cc/mock_replay_connection.h

    return mockedSendPostData(posts.get());
  }
  // TODO: mock sendNotification and test it once it is used.
+  MOCK_METHOD7(sendNotification,


qining · 2018-06-04T14:58:41Z

gapir/cc/vulkan_gfx_api.inc

@@ -36,11 +36,17 @@ IndirectMaps mIndirectMaps;

 // Function for wrapping around the normal vkCreateInstance to inject
 // virtual swapchain as an additional enabled layer.


Update blurbs to take validation layer/debug report extension into account.

qining · 2018-06-04T15:08:30Z

gapis/api/vulkan/find_issues.go

+	debugReportExtension = "VK_EXT_debug_report"
+)
+
+func isValidationLayer(n string) bool {


qining · 2018-06-04T15:09:25Z

gapis/replay/batch.go

 	var payload gapir.Payload
-	var decoder builder.ResponseDecoder
-	builderBuildTimer.Time(func() { payload, decoder, err = b.Build(ctx) })
+	var postResp builder.PostDataResponsor


Responsor? Responser? Receiver? Consumer?

qining · 2018-06-04T15:09:50Z

gapis/replay/builder/builder.go

+// machine.
+type NotificationResponsor func(p *gapir.Notification)
+
+type ReadNotification func(p gapir.Notification)


qining · 2018-06-04T15:10:01Z

gapis/replay/builder/builder.go

 	b.ReserveMemory(rng)
 }

+func (b *Builder) RegisterNotificationReader(reader ReadNotification) {


qining · 2018-06-04T15:14:06Z

tools/build/rules/grpc_c++.bzl

+  else:
+    _generate_cc(**kwargs)
+
+def cc_grpc_library(name, proto, dep_cc_proto_libs, **kwargs):


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.

qining · 2018-06-04T16:00:04Z

@AWoloszyn This CL is ready for review

AWoloszyn · 2018-06-04T19:39:57Z

gapir/replay_service/service.proto

@@ -86,12 +88,14 @@ message PostData {
 // build time of the replay instruction.
 message Notification {


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.

AWoloszyn · 2018-06-04T19:40:58Z

gapis/api/vulkan/find_issues.go

+	"github.com/google/gapid/gapis/service"
 )

+var validationLayers = [...]string{


AWoloszyn · 2018-06-04T19:43:47Z

gapis/api/vulkan/find_issues.go

+		for _, d := range validationLayersData {
+			newCmd.AddRead(d.Data())
+		}
+		newCmd.AddRead(debugReportExtNameData.Data())


I think AddRead() returns the cmd.

newCmd.AddRead().AddRead().AddRead()....

Fixed in 2b9a068

AWoloszyn · 2018-06-04T19:44:27Z

gapis/api/vulkan/find_issues.go

+		layersData := s.AllocDataOrPanic(ctx, layers)
+		info.SetEnabledLayerCount(uint32(len(layers)))
+		info.SetPpEnabledLayerNames(NewCharᶜᵖᶜᵖ(layersData.Ptr()))
+		infoData := s.AllocDataOrPanic(ctx, info)


We need to clean this up somewhere.

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().

AWoloszyn · 2018-06-04T19:44:46Z

gapis/api/vulkan/find_issues.go

+		info.SetPpEnabledLayerNames(NewCharᶜᵖᶜᵖ(layersData.Ptr()))
+		info.SetEnabledExtensionCount(uint32(len(exts)))
+		info.SetPpEnabledExtensionNames(NewCharᶜᵖᶜᵖ(extsData.Ptr()))
+		infoData := s.AllocDataOrPanic(ctx, info)


We have to clean this up somewhere

Fixed in 2b9a068

AWoloszyn · 2018-06-04T19:45:28Z

gapis/api/vulkan/find_issues.go

+			}
+			return false
+		}))
+		callbackHandleData := s.AllocDataOrPanic(ctx, callbackHandle)


Need to clean these up.

Fixed in 2b9a068

AWoloszyn · 2018-06-06T16:09:06Z

gapir/cc/replay_connection.cpp

+                                        uint32_t api_index, uint64_t label,
+                                        const std::string& msg,
                                        const void* data, uint32_t data_size) {
+  using severity::Severity;


const Severity logLevels[] = { Severity::FatalLevel, Severity::ErrorLevel, ... } if (severity <= LOG_LEVEL_DEBUG) { sev = logLevels[severity]; }

AWoloszyn · 2018-06-06T16:09:36Z

gapir/cc/replay_connection.h

-  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,


Can we make severity unsigned? Is there any reason it can ever be negative?

Changed to uint32_t in c0e75da

AWoloszyn · 2018-06-06T16:12:38Z

tools/build/rules/grpc_c++.bzl

    alwayslink = alwayslink,
  )

+


@pmuetschard Can you take a look at this part?

Done. Only one minor comment, otherwise looks fine.

pmuetschard · 2018-06-07T17:25:22Z

tools/build/rules/grpc_c++.bzl

+directly.
+"""
+
+def generate_cc_impl(ctx):


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.

Done in 4caa404

pmuetschard · 2018-06-07T17:28:13Z

tools/build/rules/grpc_c++.bzl

    alwayslink = alwayslink,
  )

+


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

AWoloszyn

Please squash before submitting.

qining requested review from AWoloszyn and ben-clayton May 30, 2018 11:56

qining commented May 30, 2018

View reviewed changes

qining force-pushed the pipe-vk-validation-report branch 2 times, most recently from 7df78e1 to 86d1a89 Compare June 2, 2018 03:31

qining changed the title ~~WIP: Pipe validation output to report view~~ Pipe validation output to report view Jun 2, 2018

qining requested a review from pmuetschard June 4, 2018 14:50

qining commented Jun 4, 2018

View reviewed changes

AWoloszyn reviewed Jun 4, 2018

View reviewed changes

qining mentioned this pull request Jun 5, 2018

Add Vulkan Validation to the Report View #832

Closed

AWoloszyn reviewed Jun 6, 2018

View reviewed changes

pmuetschard reviewed Jun 7, 2018

View reviewed changes

qining added 6 commits June 11, 2018 10:26

Hide the debug report for initial commands to be shown in report view

56e47af

Blurbs and some minor fixes

92e2606

Cleanup allocations for customized struts used in vulkan find_issue.go

f899da5

Add test for sending notification and some minor fixes

e7bdc77

fix comments

e638000

qining force-pushed the pipe-vk-validation-report branch from 4caa404 to e638000 Compare June 11, 2018 14:26

AWoloszyn approved these changes Jun 12, 2018

View reviewed changes

qining merged commit a154851 into google:master Jun 12, 2018

qining deleted the pipe-vk-validation-report branch October 23, 2018 17:33

		@@ -36,11 +36,17 @@ IndirectMaps mIndirectMaps;

		// Function for wrapping around the normal vkCreateInstance to inject
		// virtual swapchain as an additional enabled layer.

		@@ -86,12 +88,14 @@ message PostData {
		// build time of the replay instruction.
		message Notification {

Comments

Conversation

qining commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qining commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWoloszyn Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWoloszyn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

qining commented May 30, 2018 •

edited

Loading

AWoloszyn Jun 4, 2018 •

edited

Loading