KhronosGroup / OpenGL-Registry

OpenGL, OpenGL ES, and OpenGL ES-SC API and Extension Registry
677 stars 274 forks source link

Document memory object <size> parameter requirements #581

Closed cubanismo closed 12 months ago

cubanismo commented 1 year ago

For certain external handle types representing objects from non-Khronos APIs, it's difficult or impossible for an application to know the actual size of the associated memory object. The Vulkan external memory specifications acknowledge this in section 11.2.3. "Device Memory Allocation," with this language:

"If the parameters define an import operation and the external handle type is VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT, VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_KMT_BIT, or VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D12_RESOURCE_BIT, allocationSize is ignored. The implementation must query the size of these allocations from the OS."

But the OpenGL external object specs lacked equivalent language. This change adds such language.

Additionally, while investigating this spec issue internally, it was found current NVIDIA drivers actually expect a of zero when these handle types are used. This is documented in a footnote to help applications achieve better compatibility with drivers in the field.

Fixes #575

DominikWitczakAMD commented 1 year ago

Just a copy-paste from https://github.com/cubanismo/OpenGL-Registry/commit/3b39d964b02aaaed147f006d2cf38650aeedb1d4:

Agreed about new language for size now being required to be set to zero for VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT, VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_KMT_BIT.

For D3D12_RESOURCE, II'm not super-happy about adding a new requirement for UMD to check the allocation size internally, because it's a backward compat-breaking change. Can we drop the requirement in this case?

"Must" implies we should now be generating an error if size is not zero which is going to break existing apps. Can we change "the implementation must query" to "the implementation should query" ?

rpatlega commented 1 year ago

Setting size to 0 seems to be unsaid implicit specification here. looks to be no direct mechanism for the application to query actual size ( aligned width x aligned height x 4), D3DKMT way to query might not be way to find the size by application, application are baseline on size as "width x height x bpp".

Also as per the requirement, https://registry.khronos.org/OpenGL/extensions/EXT/EXT_external_objects.txt , This size is getting used for the comparision with memory offset for TexStorageMem*EXT /BufferStorageMemEXT and

"An INVALID_VALUE error is generated if is not a valid value for or the texture." "An INVALID_VALUE error is generated by BufferStorageMemEXT and NamedBufferStorageMemEXT if "memory" is 0, or if "offset" + "size" is greater than the size of the specified memory object". Are we expecting the size as well as offset to be zero in this case. If app is not aware of size, how could they get/decide on offset to plan for. We dont have interop test coverage with CTS to check on the expected behaviour here.

Agreed with @DominikWitczakAMD , Can we change "the implementation must query" to "the implementation should query" ? to avoid forcing error for this non-zero size case to keep backword compatibility.

pdaniell-nv commented 1 year ago

Are you okay with the part of the change that says "size is ignored"? So I think this is what you would like to see:

        If <handleType> is one of:

            HANDLE_TYPE_D3D12_RESOURCE_EXT
            HANDLE_TYPE_D3D11_IMAGE_EXT
            HANDLE_TYPE_D3D11_KMT_IMAGE_EXT

        <size> is ignored[fn1].

        [fn1] Some implementations incorrectly require <size> be set to '0'
        when importing such handles, so applications should set <size> to
        '0' for broader compatibility.
DominikWitczakAMD commented 1 year ago

That would work for us if we restricted the change to D3D11 handle types.

For D3D12, we currently rely on size being non-zero. Changing the spec language means breaking backward compatibility which we want to avoid at all cost.

pdaniell-nv commented 1 year ago

I assume your Vulkan implementation must handle VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D12_RESOURCE_BIT differently then since for Vulkan the spec is explicit that allocationSize is ignored.

rpatlega commented 1 year ago

Are you okay with the part of the change that says "size is ignored"? So I think this is what you would like to see:

        If <handleType> is one of:

            HANDLE_TYPE_D3D12_RESOURCE_EXT
            HANDLE_TYPE_D3D11_IMAGE_EXT
            HANDLE_TYPE_D3D11_KMT_IMAGE_EXT

        <size> is ignored[fn1].

        [fn1] Some implementations incorrectly require <size> be set to '0'
        when importing such handles, so applications should set <size> to
        '0' for broader compatibility.

This looks good to me for the approval

pdaniell-nv commented 1 year ago

@cubanismo are you okay with making this tweak to your MR?

pdaniell-nv commented 1 year ago

@oddhack this is approved to merge. Thanks.