Add support to attribute ownership of ANGLE Metal's resources
Created attachment 446209 [details] Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Created attachment 446269 [details] Patch
Created attachment 446276 [details] Patch
Created attachment 446295 [details] Patch
Created attachment 446308 [details] Patch
Created attachment 446433 [details] Patch
Created attachment 446438 [details] Patch
Created attachment 446442 [details] Patch
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
<rdar://problem/86287434>
Created attachment 446598 [details] Patch
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.
Created attachment 446670 [details] Patch
I ran clang-format using the ANGLE style on the changed files.
Created attachment 446691 [details] Patch
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
Created attachment 446703 [details] Patch
Created attachment 446769 [details] Patch
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
Created attachment 446844 [details] Patch for landing
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.
Created attachment 446970 [details] Patch for landing
(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.
Created attachment 447021 [details] Patch for landing
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.
Created attachment 447114 [details] Patch for landing
Created attachment 447237 [details] Patch for landing
Created attachment 447246 [details] Patch for landing
Comment on attachment 447246 [details] Patch for landing This patch is pleasingly small now! Great work John doing the major upstreaming first!
johncunningham@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json. Rejecting attachment 447246 [details] from commit queue.
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].