Bug 167259 – [WebGL] Do not allow GPU muxing on some old Mac hardware

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

[WebGL] Do not allow GPU muxing on particular old Mac hardware

[Dean Jackson]

<rdar://problem/30060378>

[Dean Jackson]

Created attachment 299394 [details]
Patch

[Dean Jackson]

Created attachment 299396 [details]
Patch

[Darin Adler]

Comment on attachment 299394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299394&action=review

I suggest crunching the code down a little. The three functions with the AGC prefixes are doing more than they need to for our simple purposes. In fact, the code is simple enough that I’m not sure we need a separate GPUUtilsMac.mm source file, once we remove the unneeded parts.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:57
> +    agc->service = serviceObject;

This field seems to be write-only. We should not store a pointer to the serviceObject since we are releasing it in this function. Luckily no one ever looks at the contents of the field!

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:73
> +    kern_return_t kernResult;
> +
> +    kernResult = IOConnectCallScalarMethod(agc->dataPort, kAGCOpen, nullptr, 0, nullptr, nullptr);
> +    return kernResult == KERN_SUCCESS;

No need for the local variable.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:81
> +    kern_return_t kernResult;
> +
> +    kernResult = IOConnectCallScalarMethod(agc->dataPort, kAGCClose, nullptr, 0, nullptr, nullptr);
> +    return kernResult == KERN_SUCCESS;

No need for the local variable.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.h:37
> +bool AGCAttach(mach_port_t, AGCConnectRef_t, IONotificationPortRef, IOServiceInterestCallback, void *arg);

Seems like this should just return an io_connect_t rather than a boolean. No need for AGCConnect_t.

Also, since we aren’t passing anything other than null for any of the arguments, I suggest making a version of this function for our WebKit purposes that does not take any arguments.

    io_connect_t AGCAttach();

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.h:38
> +bool AGCOpen(AGCConnectRef_t);

Should just take io_connect_t.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.h:39
> +bool AGCClose(AGCConnectRef_t);

Should just take io_connect_t dataPort. No need for boolean result.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:83
>  #if !PLATFORM(IOS)

Should be #if PLATFORM(MAC)

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:86
> +AGCConnect_t agc_connect;
> +AGCConnectRef_t agc = &agc_connect;

I see no reason these should be global variables.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:121
> +    int names[2] = { CTL_HW, HW_NCPU };
> +    int cpuCount;
> +    size_t cpuCountLength = sizeof(cpuCount);
> +    sysctl(names, 2, &cpuCount, &cpuCountLength, nullptr, 0);
> +    return cpuCount <= 4;

It’s non-obvious why a cpuCount <= 4 check is the correct thing to do here. Would it be practical to add a comment about why this is correct?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:130
> +    static int canMux = -1;
> +    if (canMux == -1)
> +        canMux = hasMuxableGPU() && !hasGPUThatShouldNotMux();
> +
> +    ASSERT(!canMux || canMux == 1);

Here’s a way to write this in C++ without using the magic -1 int:

    static bool canMux = hasMuxableGPU() && !hasGPUThatShouldNotMux();

[Simon Fraser (smfr)]

Comment on attachment 299396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299396&action=review

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:45
> +    kern_return_t kernResult;
> +    io_service_t serviceObject;
> +    io_iterator_t iterator;

Declare these at first use.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:64
> +        (void) IOServiceAddInterestNotification(notifyPort, serviceObject, kIOGeneralInterest, callback, myarg, &ignored);

No space after )

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:74
> +    kern_return_t kernResult;
> +
> +    kernResult = IOConnectCallScalarMethod(agc->dataPort, kAGCOpen, nullptr, 0, nullptr, nullptr);

Put on one line.

> Source/WebCore/platform/graphics/mac/GPUUtilsMac.cpp:82
> +    kern_return_t kernResult;
> +
> +    kernResult = IOConnectCallScalarMethod(agc->dataPort, kAGCClose, nullptr, 0, nullptr, nullptr);

Ditto.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:95
> +    if (!AGCAttach(masterPort, agc, nullptr, nullptr, nullptr)) {

How expensive is this call?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:121
> +    return cpuCount <= 4;

Is there no more specific way to detect this hardware?

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:126
> +    static int canMux = -1;

The cool kids are using std::optional<bool>

[Dean Jackson]

Thanks for the feedback. The code came from the OpenGL team. You're right that we can do away with the separate file, as well as squish everything into smaller functions.

[Dean Jackson]

Created attachment 299410 [details]
Patch

[Darin Adler]

Comment on attachment 299410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299410&action=review

Thanks. Looks great the way you reworked it. One tiny additional suggestion for refinement:

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:120
> +    mach_port_t masterPort;
> +
> +    if (IOMasterPort(MACH_PORT_NULL, &masterPort) != KERN_SUCCESS)
> +        return false;

I suggest putting this inside the attachToAppleGraphicsControl function instead of passing an argument to the function.

[Dean Jackson]

Committed r211028: <http://trac.webkit.org/changeset/211028>