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

Initial scaffolding for profiling replays. #2775

Closed
wants to merge 3 commits into from

Conversation

pmuetschard
Copy link
Member

No description provided.

@@ -234,6 +248,11 @@ func (a API) Replay(
transforms.Add(rf)
}

if profile != nil {
// Don't use DCE. TODO: should we?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using DCE for profiling. It would be way too easy for it to give completely wrong answers.

@@ -331,6 +352,25 @@ func (a API) QueryFramebufferAttachment(
return res.(*image.Data), nil
}

func (a API) SupportsPerfetto(ctx context.Context, i *device.Instance) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should be a function of the device, rather than a function of the API.

@@ -1086,3 +1122,22 @@ func (a API) QueryTimestamps(
}
return res.([]replay.Timestamp), nil
}

func (a API) SupportsPerfetto(ctx context.Context, i *device.Instance) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I think we should add this to the device.

@AWoloszyn
Copy link
Contributor

It's going to take me a bit longer to get through all of the transforms

).AddRead(bufferImageCopyData.Data())

out.MutateAndWrite(ctx, id, newCmd)
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also handle VkCmdBlitImage VkCmdCopyImage
If the image can/would be used as a framebuffer, this may cause problems as well. Likely we should check for VkImageUsageFlagBits_VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT and do nothing, since that would indicate this is a color attachment.

break
}

imageCreateInfo.SetExtent(NewVkExtent3D(s.Arena, newTexWidth, newTexHeight, newTexDepth))
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a mipmapped image, we have to adjust mip-levels appropriately.
mipLevels must be less than or equal to the number of levels in the complete mipmap chain based on extent.width, extent.height, and extent.depth.

That means any image-view that is created from this image must also be adjusted so only ever talk about the first mip-level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we want to do anything about array-layers, I expect we want to at least leave them alone,

}
out.MutateAndWrite(ctx, id, newCmd)
case *VkCmdCopyBufferToImage:
bufferImageCopy := cmd.PRegions().MustRead(ctx, cmd, s, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mip-levels also have to be handled here.

height := u32.Min(imageExtent.Height(), newTexHeight)
depth := u32.Min(imageExtent.Depth(), newTexDepth)

bufferImageCopy.SetImageExtent(NewVkExtent3D(s.Arena, width, height, depth))
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are setting the image depth to 1, we have to be careful with the 2-d image view into 3d image.

l := s.MemoryLayout
cb := CommandBuilder{Thread: cmd.Thread(), Arena: s.Arena}
switch cmd := cmd.(type) {
case *VkCmdSetViewport:
Copy link
Contributor

Choose a reason for hiding this comment

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

The viewport may also be specified in the pipeline creation as a non-dynamic piece of data. We should catch it there as well.
https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VkPipelineViewportStateCreateInfo

newCmd.AddWrite(w.Range, w.ID)
}
out.MutateAndWrite(ctx, id, newCmd)
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

You may have to also adjust scissor and renderpasscreateinfo structs for this to remain valid.

https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VkRenderPassBeginInfo
The application must ensure (using scissor if necessary) that all rendering is contained within the render area. The render area must be contained within the framebuffer dimensions.

isIndirectDraw := false
var cmdBuf VkCommandBuffer
switch cmd := cmd.(type) {
case *VkCmdDraw:
Copy link
Contributor

Choose a reason for hiding this comment

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

My only worry about this transform is that drawing partial primitives is not well-defined. As in the draw call has to not crash, but the driver is free to simply drop the entire call (if ALL primitives are partial).

l := s.MemoryLayout
cb := CommandBuilder{Thread: cmd.Thread(), Arena: s.Arena}
switch cmd := cmd.(type) {
case *VkCreateShaderModule:
Copy link
Contributor

Choose a reason for hiding this comment

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

You are allowed to specify multiple shaders in a single module. So we have to be more clever about the re-writing that we will be doing here.

It might be easier to ignore all of the VkCreateShaderModule calls, and intercept at the VkCreateGraphicsPipeline Calls (we can create shaders as necessary there), that way we can only override the fragment shader.

out.MutateAndWrite(ctx, id, cmd)
}

func (t *ProfilePostBack) Flush(ctx context.Context, out transform.Writer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may make sense sometime to unify all of these PostBack finalizing calls, so that we don't have to duplicate this all over the place.

@@ -491,6 +491,14 @@ message Thumbnail {
bool disable_optimization = 7;
}

message OverrideConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to make this more generic. Something more similar to having transforms be able to register them as "profiling overrides" with a name.

That way we don't have to change the public interface every time we add a what-if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants