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.

Sign up for GitHub

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

Jump to bottom

Corrected the logic in the npu preempt data function #8296

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Corrected the logic in the npu preempt data function #8296

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/runtime_src/core/include/ert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ get_ert_npu_data(struct ert_start_kernel_cmd* pkt)
static inline struct ert_npu_preempt_data*
get_ert_npu_preempt_data(struct ert_start_kernel_cmd* pkt)
{
if (pkt->opcode != ERT_START_NPU)
if (pkt->opcode != ERT_START_NPU_PREEMPT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my humble opinion, this check is not needed at all since this function is an internal function.
In another word, the person who call "get_ert_npu_preempt_data" should ensure that
pkt->opcode == ERT_START_NPU_PREEMPT
is true.

With that being said, we should add
assert(pkt->opcode == ERT_START_NPU_PREEMPT)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All functions in this file have similar checks, so maintaining symmetry is beneficial. Additionally, since xrt is a submodule in various code bases (e.g., driver, etc.), it's important to include this check and ensure that callers handle it properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all those similar checks are unnecessary because all those are internal functions of driver.
If we can use assert, we do not need the if-check for internal function call.
Personally, this should be put into xrt driver code review standard in order to get the best possible performance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @dkotipal-xilinx / @dezhiAmd ,
What do you want to do with this PR? Shall i merge this change as there are other functions using similar check and check in this function is incorrect? Or u remove checks in all functions.
For me, adding check is good foe sanity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Merging this change is better than waiting. I realize our pipelines does not have the capability to catch assert under debug mode anyway.

We could do assert in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @dezhiAmd for the confirmation. Merging this change.

return NULL;

// past extra cu_masks embedded in the packet data
Expand Down
Loading