[macos] Reland default metal and also check if the system supports metal before defaulting to it #24601
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
iskakaushik
force-pushed
the
reland-make-metal-default-macos
branch
from
a3b67ee
to
b91185b
Compare
iskakaushik
changed the title
| #import <QuartzCore/QuartzCore.h> | ||
|
|
||
| @implementation FlutterRenderingBackend | ||
|
|
||
| + (BOOL)renderUsingMetal { | ||
| if (@available(macOS 10.14, *)) { | ||
| return YES; | ||
| // Do not use MTLCreateSystemDefaultDevice() to check if not-null | ||
| // as that can force a mux switch to the discrete GPU. |
There was a problem hiding this comment.
I'm a little confused by this comment. If the only Metal-supporting GPU is the discrete GPU, then presumably this would happen anyway; you don't have any logic here to avoid using Metal if that GPU isn't already active.
Can you elaborate on what the intended behavior is for a multi-GPU device?
There was a problem hiding this comment.
My intention was to delay the switch from happening until absolutely needed. Which sounds a little premature in retrospect. I will remove change it to be MTLCreateSystemDefaultDevice(), this should be fine as that is (default device) indeed the device we use for rendering when there are multiple GPUs.
There was a problem hiding this comment.
I'm not sure if the following scenario can happen, 2 GPUs one metal capable and the other isn't and MTLCreateSystemDefaultDevice returns nil as default device isn't capable of metal. This isn't ideal in that scenario, but we will need to address that more broadly.
| #import <QuartzCore/QuartzCore.h> | ||
|
|
||
| @implementation FlutterRenderingBackend | ||
|
|
||
| + (BOOL)renderUsingMetal { | ||
| if (@available(macOS 10.14, *)) { | ||
| return YES; | ||
| BOOL systemSupportsMetal = MTLCreateSystemDefaultDevice() != nil; |
There was a problem hiding this comment.
Why the intermediate variable, vs just returning the RHS directly?
There was a problem hiding this comment.
It adds more clarity to the reader IMO.
…ports metal before defaulting to it (flutter/engine#24601)
…tal before defaulting to it (flutter#24601)
…tal before defaulting to it (flutter#24601)
Fixes: flutter/flutter#74058
Fixes: flutter/flutter#32569