[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

--- Comment #10 from Kimmo Kinnunen <kkinnunen 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)

> 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

-- 
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/fbedf155/attachment-0001.htm>