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

Vulkan Command Tree Grouping Returns Wrong Command paths. #1214

Open
pmuetschard opened this issue Oct 17, 2017 · 10 comments · Fixed by #1223
Open

Vulkan Command Tree Grouping Returns Wrong Command paths. #1214

pmuetschard opened this issue Oct 17, 2017 · 10 comments · Fixed by #1223

Comments

@pmuetschard
Copy link
Member

Given a trace with the following tree:

- Frame 1 (0-363)
- Frame 2 (364-372)
  - Draw 1 (364-371)
    - 364: vkWaitForFences
    - ...
    - 371: vkQueueSubmit
      - [371 0]
        - [371 0 0]
          - RenderPass
            - 371.0.0.0: vkCmdBeginRenderPass
            - ...
            - 371.0.0.6: vkCmdEndRenderPass
  - vkQueuePresnetKHR

When fetching the nodes from the server the following values are returned:

The node at [1, 0, 7] (vkQueueSubmit) is for command: [371] - Correct.
The node at [1, 0, 7, 0] is for command: [371, 0, 0] - Should be [371, 0].
The node at [1, 0, 7, 0, 0] is for command [371, 0, 0, 0] - Should be [371, 0, 0].
The node at [1, 0, 7, 0, 0, 0] (RenderPass) is for commands [0]-[6]- Should be [371, 0, 0].
The nodes at [1, 0, 7, 0, 0, 0, x] are for commands [371, 0, 0, x], which is correct again.

This results in strange behavior, especially when selecting the RenderPass node.

@Qining Qining self-assigned this Oct 18, 2017
Qining added a commit to Qining/gapid that referenced this issue Oct 18, 2017
Fix google#1214

Turns out we just need to remove two historical lines of code
Qining added a commit to Qining/gapid that referenced this issue Oct 18, 2017
Fix google#1214

Turns out we just need to remove two historical lines of code
Qining added a commit that referenced this issue Oct 18, 2017
…1223)

* Fix indices for subcommand in Vulkan when resolving command groups

Fix #1214

Turns out we just need to remove two historical lines of code

* remove unused variables
@Qining
Copy link
Contributor

Qining commented Oct 18, 2017

It seems like this is closed automatically. Hopefully this is fixed now @pmuetschard

@pmuetschard
Copy link
Member Author

It fixed most cases, however, the RenderPass group still is broken:
The node at [1, 0, 7, 0, 0, 0] (RenderPass) is for commands [0]-[6]- Should be [371, 0, 0]

@pmuetschard pmuetschard reopened this Oct 20, 2017
@Qining
Copy link
Contributor

Qining commented Oct 20, 2017

@pmuetschard, this is because renderpass group is actually an api.CmdIDGroup, not a api.Command or api.SubCmdRoot or api.SubCmdIdx.

Do you think [371, 0, 0, 0]-[371, 0, 0, 6] is a valid range for this group? I'm thinking of making the Commands field of the returned service.CommandTreeNode something like cmdTree.path.Capture.SubCommandRange(....). Currently, it is cmdTree.path.Capture.CommandRange(...).

@Qining
Copy link
Contributor

Qining commented Oct 20, 2017

@pmuetschard, and in terms of framebuffer request, the subcommand for [1, 0, 7, 0, 0, 0] (RenderPass) should be [371, 0, 0, 6]

@pmuetschard
Copy link
Member Author

I am making these observations from the perspective of the API the server exposes, and not from the implementation:

  • The currently returned value of [0]-[6] is certainly incorrect, as it incorrectly says that the group consists of the first 7 commands of the trace.
  • [371, 0, 0, 0]-[371, 0, 0, 6] would certainly be valid, and just differ in where the new layer starts from what I was pointing out above:
- 364: Frame 2                          layer 0
  - 364: Draw 1                         layer 0
    - 364: vkWaitForFences              layer 0
    - ...
    - 371: vkQueueSubmit                layer 0
      - 371.0: [371 0]                  layer 1
        - 371.0.0: [371 0 0]            layer 2
          - 371.0.0.0: RenderPass       layer 3   -   vs layer 2, in my example
            - 371.0.0.0: vkCmd..        layer 3

@pmuetschard
Copy link
Member Author

in terms of framebuffer request, the subcommand for [1, 0, 7, 0, 0, 0] (RenderPass) should be [371, 0, 0, 6]

This will, of course, all depend on how we resolve #1134.

@Qining
Copy link
Contributor

Qining commented Oct 20, 2017

Hmm, I will put up an CL with minimized changes to make the CommandRange a SubCommandRange. Probably not align with how we will fix #1134, but at least makes sense for now.

@pmuetschard
Copy link
Member Author

Sorry to be spammy, I want to make sure we also consider multiple render passes and render pass parents:

Yours:

...
- 371: vkQueueSubmit
  - 371.0: [371 0]
    - 371.0.0: [371 0 0] (range 371.0.0)
      - 371.0.0.0: RenderPass (range: 371.0.0.0 - 371.0.0.6)
      - 371.0.0.7: RenderPass (range: 371.0.0.7 - 371.0.0.13)
    - 371.0.1: [371 0 1] (range 371.0.1)
      - 371.0.1.0: RenderPass (range: 371.0.1.0 - 371.0.1.6)
      - 371.0.1.7: RenderPass (range: 371.0.1.7 - 371.0.1.13)

vs mine:

...
- 371: vkQueueSubmit
  - 371.0: [371 0]
    - 371.0.0: [371 0 0] (range 371.0.0 - 371.0.1)
      - 371.0.0: RenderPass (range: 371.0.0.0 - 371.0.0.6)
      - 371.0.1: RenderPass (range: 371.0.1.0 - 371.0.1.6)
    - 371.0.2: [371 0 2] (range 371.0.2 - 371.0.3)
      - 371.0.2: RenderPass (range: 371.0.2.0 - 371.0.2.6)
      - 371.0.3: RenderPass (range: 371.0.3.0 - 371.0.3.6)

The choice is really yours and depends on what you think makes the most sense in numbering things from the Vulkan perspective. I like mine slightly better, because the render pass grouping makes sense to me, however, I'm not as familiar with the API as you, so maybe the grouping of the render pass's parent is more important.

@Qining
Copy link
Contributor

Qining commented Oct 20, 2017

Here is a situation why I think mine is better:

- 371: vkQueueSubmit
  - 371.0: [371 0]                                                   [indicate the SubmitInfo index]
    - 371.0.0: [371 0 0] (range 371.0.0)                 [indicate the Commandbuffer index in the SubmitInfo]
      - 371.0.0.0 vkCmdCopyImage
      - 371.0.0.1 vkCmdPipelineBarrier
      - (some other subcommands)
      - 371.0.0.10: RenderPass (range: 371.0.0.10 - 371.0.0.16)
       - 371.0.0.10: vkCmdBeginRenderpass
         - 371.0.0.11: Draw object A (range: 371.0.0.11-371.0.0.13)         [This is a debug marker group]
          - 371.0.0.11: vkCmdBeginMarkerGroup
          - 371.0.0.12: vkCmdDraw...
          - 371.0.0.13: vkCmdEndMarkerGroup
       - 371.0.0.14: vkCmdBind... 
       - 371.0.0.16: vkCmdEndRenderpass
      - 371.0.0.7: RenderPass (range: 371.0.0.7 - 371.0.0.13)
    - 371.0.1: [371 0 1] (range 371.0.1)             [second commandbuffer in the same SubmitInfo]
      - 371.0.1.0: RenderPass (range: 371.0.1.0 - 371.0.1.6)
      - 371.0.1.7: RenderPass (range: 371.0.1.7 - 371.0.1.13)

So, generally, each layer corresponds to a concrete thing in Vulkan API, in total there are: submit info index, command buffer index, command index, secondary command buffer index, secondary command index. Adding another layer of index does not quite match with the concepts in Vulkan API.

So, in the example above, the commands: [371.0.0.1] and [371.0.0.11] are at the same level, adding, showing them are in the same submit info, same command buffer. But adding another layer for the renderpass grouping introduces a new index and breaks the same level thing.

Besides, users are allowed to create as many as 'debug marker groups' as they want. So in extreme cases, it is possible to have tens of layers of debug marker groups.

@pmuetschard
Copy link
Member Author

SGTM. Thanks for the clarification. I agree, it makes more sense to prioritize the grouping at the buffers.

@Qining Qining assigned AWoloszyn and unassigned Qining Oct 24, 2019
@AWoloszyn AWoloszyn removed their assignment May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants