-
Notifications
You must be signed in to change notification settings - Fork 210
NPU plugin support #2066
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
NPU plugin support #2066
Conversation
Not tested on real hardware. Development done based on the kernel API and example listings from a real system. |
I dropped the preferred allocation policy as it would require more than one NPU to exist and that's not expected as the NPUs are tied to CPUs. |
Tested the plugin on a MeteorLake hardware. Plugin works ok: it registers the resource, container requesting the resources is started, and container gets the I'll add a test Dockerfile into this PR and move this to "ready to review" state. |
Added a workload dockerfile and a job for it. The workload has a selection of tests that I've tested to work. There are also additional tests, but they require the NPU compiler & openvino runtime, which can be included, but they take such a long time to compile. |
915ead2
to
a7ccdbd
Compare
f84a009
to
347fd7b
Compare
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.
Minor nit-picks.
Thanks for the review @eero-t. I fixed all but two that I objected to. |
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.
Approved, assuming GPU->NPU typos will be fixed.
(Did not review the controller part, I'll leave that to somebody whole knows that functionality better.)
setDefaultImageIfNeeded(&v.Spec.Image) | ||
case *NpuDevicePlugin: | ||
setDefaultImageIfNeeded(&v.Spec.Image) |
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.
These calls in the switch are all identical, so there could as well be just a single call after the switch?
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.
It could be written like so:
// type switches can have only one type in a case so the same repeats for
// all xDevicePlugin types.
// TODO: implement receivers if more complex logic is needed.
var image *string
switch v := obj.(type) {
case *DlbDevicePlugin:
image = &v.Spec.Image
case *DsaDevicePlugin:
image = &v.Spec.Image
case *FpgaDevicePlugin:
image = &v.Spec.Image
case *GpuDevicePlugin:
image = &v.Spec.Image
case *IaaDevicePlugin:
image = &v.Spec.Image
case *QatDevicePlugin:
image = &v.Spec.Image
case *SgxDevicePlugin:
image = &v.Spec.Image
case *NpuDevicePlugin:
image = &v.Spec.Image
default:
return fmt.Errorf("%w: expected an xDevicePlugin object but got %T", errObjType, obj)
}
if len(*image) == 0 {
*image = r.defaultImage
}
But I'm not sure if it's any better.
Supports Core Ultra 1, 2 and 200V series NPUs. Signed-off-by: Tuomas Katila <[email protected]>
Co-authored-by: Eero Tamminen <[email protected]> Signed-off-by: Tuomas Katila <[email protected]>
Signed-off-by: Tuomas Katila <[email protected]>
Signed-off-by: Tuomas Katila <[email protected]>
Rebased against current main. I also verified that everything still works against a real hw. |
No description provided.