Skip to content

Added stream_id to Perfetto annotations #274

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

Open
wants to merge 19 commits into
base: amd-staging
Choose a base branch
from

Conversation

ajanicijamd
Copy link
Contributor

rocprofiler-systems Pull Request

Related Issue

  • Closes #

What type of PR is this? (check all that apply)

  • Bug Fix
  • Cherry Pick
  • Continuous Integration
  • Documentation Update
  • Feature
  • Optimization
  • Refactor
  • Other (please specify)

Technical Details

Have you added or updated tests to validate functionality?

  • Yes
  • No - does not apply to this PR

Added / Updated documentation?

  • Yes
  • No - does not apply to this PR

Have you updated CHANGELOG?

  • Yes
  • No - does not apply to this PR

@ajanicijamd ajanicijamd marked this pull request as ready for review June 27, 2025 21:01
@ajanicijamd ajanicijamd requested review from jrmadsen and a team as code owners June 27, 2025 21:01
@dgaliffiAMD
Copy link
Collaborator

dgaliffiAMD commented Jul 9, 2025

I see the stream_id for the rocm_kernel_dispatch events. That looks good. I used our transpose example to validate.

  • stream_id seems to be missing from the rocm_memory_copy events. I'm not sure why it's missing there yet.
  • In tool_tracing_buffered, we'll still have to update the track names for the two categories to correspond to the HIP Streams, so they are grouped together. For example, "HIP Activity [0] Stream 1" to group together rocm_memory_copy and rocm_kernel_dispatch events for stream_id 1. I have some sample code on my local branch to show you what I mean.
  • We still need a new configuration option to allow the user to toggle between the two grouping styles.
  • Lastly, we'll need to add some compile-time and runtime-time guards to preserve backwards compatibility. The HIP_STREAM callback was only introduced in ROCPROFILER_VERSIONS >= 700.

@dgaliffiAMD
Copy link
Collaborator

dgaliffiAMD commented Jul 16, 2025

I just merged with amd-staging to resolve a conflict there.
You can also take a look at https://github.com/dgaliffiAMD/rocprofiler-systems/tree/group-by-stream. I added some code to support a ROCPROFSYS_ROCM_GROUP_BY_QUEUE, so we can toggle between grouping by HSA queue or HIP stream. We still need add some documentation and testing around this, though

ajanicijamd and others added 9 commits July 16, 2025 19:06
Based on the `ROCPROFSYS_ROCM_GROUP_BY_QUEUE` setting, group these
traces accordingly in the Perfetto trace.

Signed-off-by: David Galiffi <[email protected]>
…ption.

If it is not supported, we cannot group by HIP stream and must default to grouping by HSA queue
Comment on lines 1314 to 1316
ROCPROFILER_CALL(rocprofiler_configure_callback_tracing_service(
_data->primary_ctx, ROCPROFILER_CALLBACK_TRACING_HIP_STREAM, nullptr, 0,
tool_hip_stream_callback, nullptr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

rocprofiler_configure_callback_tracing_service will return an error in this case if both "kernel-dispatch" and "memory-copy" are enabled.

Instead, you can change the logic to something like this:

if ROCPROFILER_BUFFER_TRACING_KERNEL_DISPATCH
   setup specific to kernel-dispatch

if ROCPROFILER_BUFFER_TRACING_MEMORY_COPY 
   setup specific to memory-copy

if ROCPROFILER_BUFFER_TRACING_MEMORY_COPY or ROCPROFILER_BUFFER_TRACING_KERNEL_DISPATCH
   rocprofiler_configure_callback_tracing_service(ROCPROFILER_CALLBACK_TRACING_HIP_STREAM)

dgaliffiAMD and others added 4 commits July 17, 2025 19:44
The HIP_STREAM callback was new in ROCPROFILER v0.7.0

Signed-off-by: David Galiffi <[email protected]>
Signed-off-by: David Galiffi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants