Skip to content

Add changes to partial sync xrt::module buffers after they are patched #8890

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 1 commit into
base: master
Choose a base branch
from

Conversation

rbramand-xilinx
Copy link
Collaborator

Problem solved by the commit

This PR is improvement over PR - #8815
xrt::module buffers like instruction, control pkt are synced only once for the first time and in subsequent runs only parts of buffer where patching happens is synced to reduce the overhead of buffer sync. This improves performance.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

A boolean is used to indicate if its first run where entire buffer is synced and in following runs only part of buffer is synced

Risks (if any) associated the changes in the commit

Moderate and needs proper testing as all the ELF flows can be affected.

What has been tested and how, request additional testing if necessary

Testing is pending, so marking the PR with do not merge label

Documentation impact (if any)

NA

Copy link

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

I assume this doesn't change much over the previous PR. I am not excited about the conditional code in the templated function, but don't have a better suggestion right now.

Copy link
Collaborator

@larry9523 larry9523 left a comment

Choose a reason for hiding this comment

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

I think we can make this simpler by

  1. mark the buffer dirty right after we allocate and fill in data
  2. when set_arg is called, where we patch a single argument, if the buffer is dirty, don't do partial sync. if the buffer is clean, then do a partial sync and keep the buffer in clean state
  3. right before we send the command xrt::module_int::sync, if the buffer is dirty, sync the whole buffer. skipping sync if the buffer is clean.

@rbramand-xilinx
Copy link
Collaborator Author

I think we can make this simpler by

  1. mark the buffer dirty right after we allocate and fill in data
  2. when set_arg is called, where we patch a single argument, if the buffer is dirty, don't do partial sync. if the buffer is clean, then do a partial sync and keep the buffer in clean state
  3. right before we send the command xrt::module_int::sync, if the buffer is dirty, sync the whole buffer. skipping sync if the buffer is clean.

Hi @larry9523, in the existing design the dirty bit is for all the buffers and it is not specific to single buffer. So when set_arg is called if dirty bit is set we are syncing all the buffers. But with my changes it simplifies things and doesnt sync all the buffers and sync specific buffer in patcher patch calls in subsequent calls.
That said we can have wrapper bo class that can hold dirty bit to do it as you are suggesting. That can be done during refactoring of code.

@larry9523
Copy link
Collaborator

I think we can make this simpler by

  1. mark the buffer dirty right after we allocate and fill in data
  2. when set_arg is called, where we patch a single argument, if the buffer is dirty, don't do partial sync. if the buffer is clean, then do a partial sync and keep the buffer in clean state
  3. right before we send the command xrt::module_int::sync, if the buffer is dirty, sync the whole buffer. skipping sync if the buffer is clean.

Hi @larry9523, in the existing design the dirty bit is for all the buffers and it is not specific to single buffer. So when set_arg is called if dirty bit is set we are syncing all the buffers. But with my changes it simplifies things and doesnt sync all the buffers and sync specific buffer in patcher patch calls in subsequent calls. That said we can have wrapper bo class that can hold dirty bit to do it as you are suggesting. That can be done during refactoring of code.

OK. If that is the case, let's get this merged and refactor it in next PR.

Copy link

clang-tidy review says "All clean, LGTM! 👍"

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.

3 participants