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.

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

feature: add support for querying simd16 eu per dss #745

Closed
wants to merge 1 commit into from

Conversation

JablonskiMateusz
Copy link
Contributor

Related-To: NEO-12012

Related-To: NEO-12012
Signed-off-by: Mateusz Jablonski <[email protected]>
@JablonskiMateusz
Copy link
Contributor Author

@@ -504,9 +511,10 @@ bool IoctlHelperXe::getTopologyDataAndMap(const HardwareInfo &hwInfo, DrmQueryTo
topologySize -= itemSize;
dataPtr = ptrOffset(dataPtr, itemSize);
}

receivedEuPerDssInfo |= receivedSimd16EuPerDssInfo;

Choose a reason for hiding this comment

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

why don't you simply add DRM_XE_TOPO_SIMD16_EU_PER_DSS to operate in the same var as DRM_XE_TOPO_SIMD16_EU_PER_DSS? you are not doing anything differently and kernel isn't going to report both. If the kernel reports both in future it actually means it has 2 different types of EUs in that platform, which would be very odd.

Choose a reason for hiding this comment

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

@@ -835,6 +835,61 @@ TEST(IoctlHelperXeTest, givenComputeDssWhenGetTopologyDataAndMapThenResultsAreCo
}
}

TEST(IoctlHelperXeTest, givenSimd16EuPerDssAndEuPerDssWhenGetTopologyDataAndMapThenPreferSimd16EuPerDss) {

Choose a reason for hiding this comment

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

it's not meant to prefer one vs the other as we don't return both for any platform. For a non-mock test, an assert that this is true would be ok.

@@ -513,6 +520,7 @@ struct drm_xe_query_topology_mask {
#define DRM_XE_TOPO_DSS_GEOMETRY (1 << 0)
#define DRM_XE_TOPO_DSS_COMPUTE (1 << 1)
#define DRM_XE_TOPO_EU_PER_DSS (1 << 2)

Choose a reason for hiding this comment

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

There are other changes in this header that are already available in drm-next. It's probably easier if you just sync the header with drm-next and add on top this change. This way it won't conflict with future updates.
This is what we have in drm-next:

#define DRM_XE_TOPO_DSS_GEOMETRY        1
#define DRM_XE_TOPO_DSS_COMPUTE         2
#define DRM_XE_TOPO_L3_BANK             3
#define DRM_XE_TOPO_EU_PER_DSS          4

Choose a reason for hiding this comment

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

Also, feel free to commit this once DRM_XE_TOPO_SIMD16_EU_PER_DSS reaches drm-next as per rules in https://docs.kernel.org/gpu/drm-uapi.html:

The kernel patch can only be merged after all the above requirements are met, but it must be merged to either drm-next or drm-misc-next before the userspace patches land. uAPI always flows from the kernel, doing things the other way round risks divergence of the uAPI definitions and header files.

@JablonskiMateusz JablonskiMateusz added the merged change was merged label Sep 11, 2024
@JablonskiMateusz
Copy link
Contributor Author

covered in dfbad80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged change was merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants