[Webkit-unassigned] [Bug 233941] Add support to attribute ownership of ANGLE Metal's resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=233941

John Cunningham <johncunningham at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #446442|1                           |0
        is obsolete|                            |

--- Comment #13 from John Cunningham <johncunningham at apple.com> ---
Comment on attachment 446442
  --> https://bugs.webkit.org/attachment.cgi?id=446442
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211209/c975940c/attachment-0001.htm>