Bug 233941 – Add support to attribute ownership of ANGLE Metal's resources

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[John Cunningham]

Add support to attribute ownership of ANGLE Metal's resources

[John Cunningham]

Created attachment 446209 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[John Cunningham]

Created attachment 446269 [details]
Patch

[John Cunningham]

Created attachment 446276 [details]
Patch

[John Cunningham]

Created attachment 446295 [details]
Patch

[John Cunningham]

Created attachment 446308 [details]
Patch

[John Cunningham]

Created attachment 446433 [details]
Patch

[John Cunningham]

Created attachment 446438 [details]
Patch

[John Cunningham]

Created attachment 446442 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 446442 [details]
Patch

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

> Source/ThirdParty/ANGLE/include/EGL/eglext_angle.h:181
> +#define EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE 0x33B0

Let's put 0x34A7 for now (reserved for ANGLE)

> Source/ThirdParty/ANGLE/src/libANGLE/Caps.h:647
> +    bool createContextOwnershipIdentity = false;

createContextOwnershipIdentity*Metal* ?

> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:235
> +EGLint GetOwnershipIdentity(const egl::AttributeMap &attribs)

This file is for *all* implementations.
Ownership identity is a property of the Metal backend.
You need to move this to ContextMtl

> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:237
> +    return static_cast<EGLint>(attribs.getAsInt(EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE, EGL_FALSE));

getAsInt already returns int.
The default value should be 0, not EGL_FALSE

> Source/ThirdParty/ANGLE/src/libANGLE/State.h:105
> +          EGLint owwnershipIdentity);

Typo, but anyway needs to move away from the State.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:198
> +      mContextDevice(

here you would add  as the attribs: ContextMtl(.., AttributeMap& attribs) 
mContextDevice(GetOwenershipIdentity(attribs))

and in GetOwenershipIdentity you'd #ifdef

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:2449
> +#endif

Maybe remove, read the token off from attributes in the constructor and pass it to ContextDevice

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.h:28
> +    ContextDevice(task_id_token_t ownershipIdentity) { mOwnershipIdentity = ownershipIdentity; }

Maybe:
1) remove ifdef
2) set the parameter as GLint
3)  move the implementation to .mm file.

In implementation, do following

ContextDevice(GLint ownerIdentity)
{
#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
    auto ownerIdentity = static_cast<task_id_token_t>(ownerIdentity);
    if (ownerIdentity != TASK_ID_TOKEN_NULL)
    {
         kr = mach_port_mod_refs(mach_task_self(), ownerIdentity, MACH_PORT_RIGHT_SEND, 1);
        .... check kr, etc. 
       mOwnerIdentity = ownerIdentity;
    }
#endif
}
in ~ContextDevice, unref the task token.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:18
> +#pragma mark - Context Device implementation

I don't think you need a pragma mark categorisation for a 50 line implementation?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:36
> +#if USE_OWNERSHIP_IDENTITY_PRIVATE

so remove the extra ifdefs
the code should be 

auto texture = [get() newTextureWithDescriptor:descriptor];
setOwnerWithIdentity(texture);

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:124
> +}

Here you would #if:

void ContextDevice::setOwnerWithIdentity(id<MTLResource> resource)
{
#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
    setOwnerWithIdentity(resource, mOwnerIdentity);
#endif
}

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_format_table_autogen.mm:304
> +    id<MTLDevice> device = display->getMetalDevice();

these probably are now somehow wrong.
Anyway, we shouldn't modify the autopen tables unless the generator is modified.
I don't see the python files modified.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_ownership_identity.h:21
> +#import "libANGLE/renderer/metal/mtl_ownership_identity_private.h"

Is it useful to have the thing implemented as:
 - if MTLResource_Private.h is missing, don't use
 - if mtl_ownership_identity_private.h is missing, don't use

Can we just remove this file and use the define: ANGLE_USE_METAL_OWNERSHIP_IDENTITY.
Then we make sure by default, ANGLE_USE_METAL_OWNERSHIP_IDENTITY is only defined if
 1) os version is correct
 2) internal headers are visible
 3) user has requested the feature
 (e.g. this define is what common/apple/apple_platform.h would set, see below)

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_ownership_identity_private.h:36
> +

This file could be called mtl_resource_spi.h

Contents would be:

#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY

#import <Metal/Metal.h>
#import <Metal/MTLResource_Private.h>
#import <mach/mach_types.h>

namespace rx
{

....

}

#endif

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_task_identity_token.h:21
> +#define ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY 0

I think this could be moved to common/apple/apple_platform.h
I think additionally you could have the __has_include(<Metal/MTLResource_Private.h>) here

There you could have additionally

  #define ANGLE_USE_METAL_OWNERSHIP_IDENTITY (ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY && defined(ANGLE_ENABLE_METAL_OWNERSHIP_IDENTITY))

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:224
> +                if ([[metalDevice name] rangeOfString:it.trademark].location != NSNotFound)

might be unneeded change?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:238
> +    return angle::GetVendorIDFromMetalDeviceRegistryID([device registryID]);

might be unneeded change?

> Source/ThirdParty/ANGLE/src/libANGLE/validationEGL.cpp:1634
>  

Here you would validate that the passed identity is a task_id_token_t (e.g. at least that it's a send right? I'm not 100% sure) or null token.
I don't know yet how to do that, but we want to be relatively sure that there's no error when we ref it

[Radar WebKit Bug Importer]

<rdar://problem/86287434>

[John Cunningham]

Created attachment 446598 [details]
Patch

[John Cunningham]

Comment on attachment 446442 [details]
Patch

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

>> Source/ThirdParty/ANGLE/include/EGL/eglext_angle.h:181
>> +#define EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE 0x33B0
> 
> Let's put 0x34A7 for now (reserved for ANGLE)

Ok, I think it's already taken by EGL_METAL_TEXTURE_ANGLE but I'll use that for now

>> Source/ThirdParty/ANGLE/src/libANGLE/Caps.h:647
>> +    bool createContextOwnershipIdentity = false;
> 
> createContextOwnershipIdentity*Metal* ?

I went ahead and matched the style of ones such as "EGL_ANGLE_vulkan_image/vulkanImageANGLE" and "EGL_ANGLE_d3d_share_handle_client_buffer/d3dShareHandleClientBuffer"

>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:235
>> +EGLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
> 
> This file is for *all* implementations.
> Ownership identity is a property of the Metal backend.
> You need to move this to ContextMtl

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:237
>> +    return static_cast<EGLint>(attribs.getAsInt(EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE, EGL_FALSE));
> 
> getAsInt already returns int.
> The default value should be 0, not EGL_FALSE

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/State.h:105
>> +          EGLint owwnershipIdentity);
> 
> Typo, but anyway needs to move away from the State.

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:198
>> +      mContextDevice(
> 
> here you would add  as the attribs: ContextMtl(.., AttributeMap& attribs) 
> mContextDevice(GetOwenershipIdentity(attribs))
> 
> and in GetOwenershipIdentity you'd #ifdef

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:2449
>> +#endif
> 
> Maybe remove, read the token off from attributes in the constructor and pass it to ContextDevice

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.h:28
>> +    ContextDevice(task_id_token_t ownershipIdentity) { mOwnershipIdentity = ownershipIdentity; }
> 
> Maybe:
> 1) remove ifdef
> 2) set the parameter as GLint
> 3)  move the implementation to .mm file.
> 
> In implementation, do following
> 
> ContextDevice(GLint ownerIdentity)
> {
> #if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
>     auto ownerIdentity = static_cast<task_id_token_t>(ownerIdentity);
>     if (ownerIdentity != TASK_ID_TOKEN_NULL)
>     {
>          kr = mach_port_mod_refs(mach_task_self(), ownerIdentity, MACH_PORT_RIGHT_SEND, 1);
>         .... check kr, etc. 
>        mOwnerIdentity = ownerIdentity;
>     }
> #endif
> }
> in ~ContextDevice, unref the task token.

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:18
>> +#pragma mark - Context Device implementation
> 
> I don't think you need a pragma mark categorisation for a 50 line implementation?

True, originally added as I thought there would be more uses of MTLDevice, but there aren't that many, at least not many used by the context.

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:36
>> +#if USE_OWNERSHIP_IDENTITY_PRIVATE
> 
> so remove the extra ifdefs
> the code should be 
> 
> auto texture = [get() newTextureWithDescriptor:descriptor];
> setOwnerWithIdentity(texture);

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_format_table_autogen.mm:304
>> +    id<MTLDevice> device = display->getMetalDevice();
> 
> these probably are now somehow wrong.
> Anyway, we shouldn't modify the autopen tables unless the generator is modified.
> I don't see the python files modified.

Yeah accidental rename.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_ownership_identity_private.h:36
>> +
> 
> This file could be called mtl_resource_spi.h
> 
> Contents would be:
> 
> #if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
> 
> #import <Metal/Metal.h>
> #import <Metal/MTLResource_Private.h>
> #import <mach/mach_types.h>
> 
> namespace rx
> {
> 
> ....
> 
> }
> 
> #endif

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_task_identity_token.h:21
>> +#define ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY 0
> 
> I think this could be moved to common/apple/apple_platform.h
> I think additionally you could have the __has_include(<Metal/MTLResource_Private.h>) here
> 
> There you could have additionally
> 
>   #define ANGLE_USE_METAL_OWNERSHIP_IDENTITY (ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY && defined(ANGLE_ENABLE_METAL_OWNERSHIP_IDENTITY))

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:224
>> +                if ([[metalDevice name] rangeOfString:it.trademark].location != NSNotFound)
> 
> might be unneeded change?

I attempted to make it very explicit when we were using the objective c object rather than C++, but it's okay to be reverted

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:238
>> +    return angle::GetVendorIDFromMetalDeviceRegistryID([device registryID]);
> 
> might be unneeded change?

Done

>> Source/ThirdParty/ANGLE/src/libANGLE/validationEGL.cpp:1634
>>  
> 
> Here you would validate that the passed identity is a task_id_token_t (e.g. at least that it's a send right? I'm not 100% sure) or null token.
> I don't know yet how to do that, but we want to be relatively sure that there's no error when we ref it

I'll add a TODO for now.

[John Cunningham]

Created attachment 446670 [details]
Patch

[John Cunningham]

I ran clang-format using the ANGLE style on the changed files.

[John Cunningham]

Created attachment 446691 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 446691 [details]
Patch

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

> Source/ThirdParty/ANGLE/src/libANGLE/Caps.cpp:1297
>      // clang-format on

strangely the clang-format off / on doesn't apply here?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:2450
> +GLint ContextMtl::getOwnershipIdentity(const egl::AttributeMap &attribs) const

This function should be just a compile-unit local function in the anonymous namespace at the top of the file, GetOwnershipIdentity...

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:326
> +#if HAVE(MTLRESOURCE_SET_OWNERSHIP_IDENTITY) && HAVE(TASK_IDENTITY_TOKEN)

move this to new bug and some other feedback in slack

[John Cunningham]

Created attachment 446703 [details]
Patch

[John Cunningham]

Created attachment 446769 [details]
Patch

[Jonah RD]

Comment on attachment 446769 [details]
Patch

Looks good to me. Two things that should be added:
- Entry to egl_angle_ext.xml (for an enum like so: https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/scripts/egl_angle_ext.xml;l=175?q=EGL_ANGLE_display_semaphore_share_group&ss=chromium&start=1)
- Entry in registry_xml.py for supported_egl_extensions: https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/scripts/registry_xml.py;l=276

[John Cunningham]

Created attachment 446844 [details]
Patch for landing

[Kenneth Russell]

Comment on attachment 446844 [details]
Patch for landing

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

Couple of quick points:

1) If at all possible let's land this upstream in ANGLE first and then roll it into WebKit. This touches a lot of files, and given that we're still upstreaming WebKit's changes, has the potential to introduce a lot of conflicts during the next roll.

2) I've allocated an enum value in ANGLE's internal doc for the new extension. Please follow Jonah's recommendations about making sure all the bookkeeping files are updated in the repo.

Thanks.

> Source/ThirdParty/ANGLE/include/EGL/eglext_angle.h:181
> +#define EGL_CONTEXT_METAL_OWNERSHIP_IDENTITY_ANGLE 0x34A7

The ANGLE team maintains an internal document allocating these enums. I've reserved 0x34D2 for this purpose.

[John Cunningham]

Created attachment 446970 [details]
Patch for landing

[John Cunningham]

(In reply to Kenneth Russell from comment #22)
> Comment on attachment 446844 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446844&action=review
> 
> Couple of quick points:
> 
> 1) If at all possible let's land this upstream in ANGLE first and then roll
> it into WebKit. This touches a lot of files, and given that we're still
> upstreaming WebKit's changes, has the potential to introduce a lot of
> conflicts during the next roll.
> 
> 2) I've allocated an enum value in ANGLE's internal doc for the new
> extension. Please follow Jonah's recommendations about making sure all the
> bookkeeping files are updated in the repo.
> 
> Thanks.
> 
> > Source/ThirdParty/ANGLE/include/EGL/eglext_angle.h:181
> > +#define EGL_CONTEXT_METAL_OWNERSHIP_IDENTITY_ANGLE 0x34A7
> 
> The ANGLE team maintains an internal document allocating these enums. I've
> reserved 0x34D2 for this purpose.

Updated to 0x34D2.

[John Cunningham]

Created attachment 447021 [details]
Patch for landing

[Kenneth Russell]

Comment on attachment 447021 [details]
Patch for landing

The latest patch looks good to me though I'd again suggest that this first be landed upstream in ANGLE and then rolled forward into WebKit to reduce merge conflicts and reduce the chance of accidents.

[John Cunningham]

Created attachment 447114 [details]
Patch for landing

[John Cunningham]

Created attachment 447237 [details]
Patch for landing

[John Cunningham]

Created attachment 447246 [details]
Patch for landing

[Kenneth Russell]

Comment on attachment 447246 [details]
Patch for landing

This patch is pleasingly small now! Great work John doing the major upstreaming first!

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 447246 [details] from commit queue.

[EWS]

Committed r287143 (245327@main): <https://commits.webkit.org/245327@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447246 [details].