feat: improve GPU support when multiple GPUs #2238
Conversation
| for (let i = 0; i < gpus.length; i++) { | ||
| if (gpus[i].vendor !== GPUVendor.UNKNOWN) { | ||
| selectedGPU = i; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: use the Array.find method ?
There was a problem hiding this comment.
I think we would need to use findIndex versus find. With that we'd have to check for -1 as that is what's returned if there is no match. Not sure it would be much less code or clearer.
There was a problem hiding this comment.
If there is a single GPU we will take it but won't check if the vendor is known. Should we check it as well ?
There was a problem hiding this comment.
@jeffmaury I left it not checking since that was the way it was before and I was not sure if that would break anything. I can add the check if that is the right thing to do.
There was a problem hiding this comment.
Would be good because I understood your configuration have 2 GPUs returned one which is not to be selected so I wonder we could have a configuration where we could have this single GPU returned
| let selectedGPU = 0; | ||
| if (gpus.length > 1) { | ||
| // Look for a GPU that is of a known type, use the first one found. | ||
| // Fall back to the first one if no GPUs are of known type. | ||
| for (let i = 0; i < gpus.length; i++) { | ||
| if (gpus[i].vendor !== GPUVendor.UNKNOWN) { | ||
| selectedGPU = i; | ||
| break; | ||
| } | ||
| } | ||
| console.warn( | ||
| `found ${gpus.length} gpus: using multiple GPUs is not supported. Using ${gpus[selectedGPU].model}.`, | ||
| ); | ||
| } | ||
| gpu = gpus[selectedGPU]; |
There was a problem hiding this comment.
| let selectedGPU = 0; | |
| if (gpus.length > 1) { | |
| // Look for a GPU that is of a known type, use the first one found. | |
| // Fall back to the first one if no GPUs are of known type. | |
| for (let i = 0; i < gpus.length; i++) { | |
| if (gpus[i].vendor !== GPUVendor.UNKNOWN) { | |
| selectedGPU = i; | |
| break; | |
| } | |
| } | |
| console.warn( | |
| `found ${gpus.length} gpus: using multiple GPUs is not supported. Using ${gpus[selectedGPU].model}.`, | |
| ); | |
| } | |
| gpu = gpus[selectedGPU]; | |
| if (gpus.length > 1) { | |
| // Look for a GPU that is of a known type, use the first one found. | |
| // Fall back to the first one if no GPUs are of known type. | |
| gpu = gpus.find(({vendor}) => vendor !== GPUVendor.UNKNOWN) ?? gpus[0]; | |
| } |
We can simplify
There was a problem hiding this comment.
The change missed the case with 0, cpus. pulled it in minus the check for number of cpus and it seems to do the right thing. Also rebased.
There was a problem hiding this comment.
It is a lot more compact which is great.
If there are multiple GPUs use the first one that is of known type instead of first GPU. If no GPUs of are known type fall back to the first GPU as before. Add another string for vendor that is accepted as an NVIDIA GPU when doing GPU detection based on what was seen on linux with an NVIDIA 4070 Ti Super Signed-off-by: Michael Dawson <[email protected]>
mhdawson
force-pushed
the
improve-gpu-detection
branch
from
022cb15 to
9d09bf3
Compare
|
|
||
| // Look for a GPU that is of a known type, use the first one found. | ||
| // Fall back to the first one if no GPUs are of known type. | ||
| gpu = gpus.find(({ vendor }) => vendor !== GPUVendor.UNKNOWN) ?? gpus[0]; |
There was a problem hiding this comment.
If as @benoitf suggested we should only use recognized GPUs it should be as simple as removing the ?? gpus[0]
|
Merging as got two approval and tests are passing |
|
Thanks @mhdawson this is great ! |
What does this PR do?
If there are multiple GPUs use the first one that is of known type instead of first GPU. If no GPUs of are known type fall back to the first GPU as before.
Add another string for vendor that is accepted as an NVIDIA GPU when doing GPU detection based on what was seen on linux with an NVIDIA 4070 Ti Super
The VM in which I did testing for potential GPU support on linux (#2180) had 2 CPUs, one of which is NVIDIA but it was not gpu[0]. This should have no effect in people only have 1 gpu. With this change I could use the second GPU.
Screenshot / video of UI
N/A
What issues does this PR fix or reference?
#2214
How to test this PR?
Run unit tests as updated