Add support for ext dispatch packet 4. - #8401
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
Only reviewing the rocr-runtime file (hsa_ext_amd.h). The change is ABI-safe: amd_launch_descriptor_t resolves via the existing #include "amd_launch_descriptor.h", both new unions preserve the original field sizes/offsets (1 byte and 8 bytes on LP64), the 64-byte packet size is unchanged, and existing .setup/.dep_signal accesses still compile through the anonymous unions. Two non-blocking notes inline.
| uint8_t setup; // Format 3: full 8-bit setup field | ||
| struct { // Format 4: bitfield with dispatch_ahead | ||
| uint8_t setup_dimensions : 2; | ||
| uint8_t dispatch_ahead_programmatic : 1; |
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
[Optional] This setup byte is read by the CP/FW, so its bit layout is part of the hardware contract — but C bitfield allocation order is implementation-defined. It matches the documented layout (dimensions bits[1:0], dispatch_ahead_programmatic bit[2]) only on little-endian GCC/Clang, which is all AMD ships, yet the rest of HSA deliberately encodes such fields with explicit shift/mask constants (e.g. HSA_KERNEL_DISPATCH_PACKET_SETUP_DIMENSIONS) rather than bitfields. Consider a mask/shift accessor or a static_assert locking the layout. Note too that the anonymous struct/union requires C11+ from every consumer of this public header.
db1dd4c to
8009b98
Compare
274a13c to
c0fb5a0
Compare
| @@ -1539,7 +1567,8 @@ bool VirtualGPU::dispatchAqlPacket(AqlPacket* packet, uint16_t header, | |||
| } | |||
| return true; | |||
| } else { | |||
| dispatchBlockingWait(reinterpret_cast<hsa_kernel_dispatch_packet_t*>(packet)); | |||
| hsa_amd_packet_type8_t amd_format = static_cast<hsa_amd_packet_type8_t>(rest & 0xFF); | |||
There was a problem hiding this comment.
This is a bug, you cannot do this for a regular packet., this needs a ext packet detect guard. rest && 0xff will give you dimensions for a regular packet. for a dim3 kenrel it means we think now its type3
c0fb5a0 to
4d9adf1
Compare
saleelk
left a comment
saleelk
left a comment
There was a problem hiding this comment.
its mising the change in queue.h
bool __forceinline isExtDispatch() const {
return (type(packet.header) == HSA_PACKET_TYPE_VENDOR_SPECIFIC &&
amd_vendor.format == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH);
}
| @@ -1419,6 +1424,24 @@ bool VirtualGPU::dispatchGenericAqlPacket(AqlPacket* packet, uint16_t header, ui | |||
| *aql_loc = *packet; | |||
|
|
|||
| metadata_preloader_.Set(packet, header, index & queueMask); | |||
|
|
|||
| // For Format 4 (DISPATCH1), set the launch_descriptor pointer in the AQL packet | |||
There was a problem hiding this comment.
this could be done inside the Set function btw . Also fix the comment
bfd67c5 to
45bc161
Compare
45bc161 to
afbd91b
Compare
3 participants
Motivation
Technical Details
JIRA ID
Test Plan
Test Result
Submission Checklist