-
Notifications
You must be signed in to change notification settings - Fork 174
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
levelzero: only use Sysman queries instead of similar Core API queries #595
base: master
Are you sure you want to change the base?
Conversation
f04d462
to
ecf31fb
Compare
d978732
to
980e829
Compare
This should be rebased/simplified on top of #695 |
@saik-intel Now that we use zesInit(), I guess there's no reason to query both zeDevicePciGetPropertiesExt() and zesDevicePciGetProperties() in case one fails but no the other. The later should always be supported now, right? |
Now that zesInit() is mandatory, don't bother falling back to the core API, Sysman shouldn't fail. Signed-off-by: Brice Goglin <[email protected]>
Now that zesInit() is mandatory, don't bother trying the core API extension first in case sysman wouldn't be available. Hence remove the zeDevicePciGetPropertiesExt() optional detection Signed-off-by: Brice Goglin <[email protected]>
We don't need it anymore. Signed-off-by: Brice Goglin <[email protected]>
Signed-off-by: Brice Goglin <[email protected]>
980e829
to
c2b2cfd
Compare
@TApplencourt Could you please run |
Of course! Here it's: At first glance, Everything looks good to me.
But Make check reported an error:
Composite or FLat doesn't change anything:
Using:
|
Thanks. There's at least one bug in the test file, I'll try to debug more your report. Aside of that it looks like ZES fails to report a valid PCI link speed (it shows 0.25GB/s instead of 63 in your case, and nothing on my machines). I'll revert that part and use ZE for this for now. |
Could you comment out the assert on line 188 of tests/hwloc/levelzero.c, make -C tests/hwloc levelzero && tests/hwloc/levelzero? I'd like to confirm that devices are reported by ZES and ZE in different orders. That'd be funny, but easy to fix. |
This goes on top of #594 which might be merged once a compute-runtime release brings a working zesInit(). This PR requires zesInit() to be more widely available, especially the last commit which completely remove the old setting of ZES_ENABLE_SYSMAN=1 in the env.Now that we require zesInit(), just use ZES queries instead of switching from/to the Core API depending on ZES being available or not.