-
Notifications
You must be signed in to change notification settings - Fork 0
Fixing command sizing and changing the test to issue the maximum number of packets the queue supports. #23
Conversation
…maximum number of packets the queue supports.
constexpr int FIRST_CMD_COUNT_SIZE_INCREASE = 5; | ||
// The driver places a structure before each command in a command chain. | ||
// Need to increase the size of the command by the size of this structure. | ||
constexpr int CMD_COUNT_SIZE_INCREASE = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get this size from the driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be really nice but not sure the best way to do that. The reason it is 3
is because the driver sets the size of the command in the chain here https://github.com/amd/xdna-driver/blob/main/src/driver/amdxdna/aie2_message.c#L632 which uses the size of this struct https://github.com/amd/xdna-driver/blob/main/src/driver/amdxdna/aie2_msg_priv.h#L388-L392. We don't have a need to have this cmd_chain_slot_execbuf_cf
struct in the runtime but can copy it over if we want to get the size. We could be more programmatic with an ioctl but that might be overkill. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the comment there:
/* Accurate buf size to hint firmware to do necessary copy */
What happens in the FW doesn't take the hint I wonder 🤔
Anyway my thought is it's just a matter of API - xdna should have a more visible API but it doesn't so the only thing we can do is document and leave breadcrumbs. So can you please leave these exact lines in the comment (with the hash, i.e. go to the link and hit y
on your keyboard).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works with me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…er of packets the queue supports. (#23) * Fixed workarounds in the AIE soft queue regarding the size of the command chain and the individual commands. * Added the functionality to aie_hsa_dispatch_test.cc to query the size of the AIE queue and issue the maximum number of packets it supports. * Some additional small fixes on aie_hsa_dispatch_test.cc test. * Adding links to driver source
…er of packets the queue supports. (#23) * Fixed workarounds in the AIE soft queue regarding the size of the command chain and the individual commands. * Added the functionality to aie_hsa_dispatch_test.cc to query the size of the AIE queue and issue the maximum number of packets it supports. * Some additional small fixes on aie_hsa_dispatch_test.cc test. * Adding links to driver source
…er of packets the queue supports. (#23) * Fixed workarounds in the AIE soft queue regarding the size of the command chain and the individual commands. * Added the functionality to aie_hsa_dispatch_test.cc to query the size of the AIE queue and issue the maximum number of packets it supports. * Some additional small fixes on aie_hsa_dispatch_test.cc test. * Adding links to driver source
Made the following changes:
aie_hsa_dispatch_test.cc
to query the size of the AIE queue and issue the maximum number of packets it supports.aie_hsa_dispatch_test.cc
test.