Add support for processing accelerators hardware #385
Conversation
There was a problem hiding this comment.
@mlorenzofr I absolutely love your ambition :) And overall, the quality of your PR is outstanding, thank you!
That said, I'm going to request that you remove the filtering functionality from this PR and add the filtering functionality in a separate follow-up PR? That way I can evaluate that separately. The reason is because none of the other ghw functions have a filter parameter so adding one just for the accelerator package would be mis-aligned.
|
Of course I can, I'll start making the changes and submit them in 2 different PRs. 😉 |
mlorenzofr
force-pushed
the
accelerators
branch
2 times, most recently
from
50b8a40 to
55dc8f3
Compare
|
I was thinking... this PR will detect processing accelerators hardware, but there are hardware from some vendors that are not categorized as processing accelerators, but as 3D controllers. Should we add that hardware in the default detection until the custom filters feature is added? |
@mlorenzofr Yes, absolutely. :) |
There was a problem hiding this comment.
@mlorenzofr thank you sir! a few suggested changes inline :)
mlorenzofr
force-pushed
the
accelerators
branch
from
55dc8f3 to
abffb83
Compare
|
I've added all the suggestions, I loved them ❤️ . Thanks a lot for the contributions. 🙏 Additionally, since we've added discovery for more hardware, I've added an additional test for Nvidia cards. |
pkg/accelerator/accelerator.go
Outdated
|
|
||
| type Info struct { | ||
| ctx *context.Context | ||
| AcceleratorDevices []*AcceleratorDevice `json:"devices"` |
There was a problem hiding this comment.
One tiny request... let's call the field Devices instead of AcceleratorDevices since the "Accelerator" part is assumed.
mlorenzofr
force-pushed
the
accelerators
branch
2 times, most recently
from
c036151 to
58df8e5
Compare
|
I forgot to update the documentation, I fixed it the commit 58df8e5 |
Signed-off-by: Manuel Lorenzo <mlorenzofr@redhat.com>
mlorenzofr
force-pushed
the
accelerators
branch
from
58df8e5 to
25c02a9
Compare
|
@mlorenzofr welcome to the |
|
Nice work indeed! |
LOL, probably because I made a comment in Italian earlier and that triggered your notifications :P |
| ) | ||
|
|
||
| func main() { | ||
| accel, err := ghw.Accelerator() |
There was a problem hiding this comment.
do we want to extend (maybe later?) this API to accept additive user-provided filters?
I'm referring to the wide variety of devices out there and how they present themselves on PCI bus.
By "additive" filter I mean
- users cannot rule out a device
ghwis really sure it's an accelerator (this meansghwmust be conservative in the core logic, which I think it's good anyway) - users can opt-in devices if they know better than
ghwfor whatever reasons (maybe oldghwversion, maybe rare/custom devices)
There was a problem hiding this comment.
Yep. @mlorenzofr was going to submit a PR adding that filtering functionality. He originally added it in this PR but I asked him to separate it out into a new one.
There was a problem hiding this comment.
perfect! I'll be on the lookout this time!
There was a problem hiding this comment.
Yes, we'll extend it, but in a different PR. @jaypipes asked to separate it from the original development so he could analyze it step by step.
Welcome back, btw 😉
This PR adds support for detection of processing accelerators to the library. Please take a look.
This fixes #383