[PATCH v7 4/4] winevulkan: Implement VK_KHR_external_memory_win32 for images.

[Georg Lehmann]

On 10.06.21 21:18, Derek Lesho wrote:
> 
> On 6/9/21 9:56 PM, Zebediah Figura (she/her) wrote:
>> On 6/5/21 10:53 AM, Georg Lehmann wrote:
>>>
>>>
>>> On 05.06.21 01:23, Zebediah Figura (she/her) wrote:
>>>> On 6/1/21 10:17 AM, Derek Lesho wrote:
>>>>>
>>>>> On 5/31/21 10:43 AM, Zebediah Figura (she/her) wrote:
>>>>>> On 5/26/21 9:21 AM, Derek Lesho wrote:
>>>>>>> On 5/25/21 10:07 PM, Zebediah Figura (she/her) wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> * Shared resources in d3d9-11, when using the OpenGL backend
>>>>>>>>>
>>>>>>>>> As mentioned, the EXT_external_objects extension exists, (
>>>>>>>>> https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_external_objects.txt 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ), which accepts and produces file descriptors compatible with the
>>>>>>>>> ones
>>>>>>>>> Vulkan produces and accepts.  The interactions here seem to be 
>>>>>>>>> well
>>>>>>>>> defined.
>>>>>>>>
>>>>>>>> According to my reading, neither GL_EXT_external_objects nor
>>>>>>>> GL_EXT_external_objects_fd provide for export of an object from an
>>>>>>>> OpenGL context; they only allow importing objects created by 
>>>>>>>> Vulkan.
>>>>>>>
>>>>>>> Yeah, from what I can see, openGL has no standard way to share 
>>>>>>> memory
>>>>>>> across processes without the use of this extension and Vulkan. I
>>>>>>> suspect this would be difficult due to all the user mode driver code
>>>>>>> protecting against invalid use of resources.  I think the most
>>>>>>> reasonable path forward is to create a Vulkan dummy object in 
>>>>>>> wined3d,
>>>>>>> then import it into GL.  Of course, this would make shared resources
>>>>>>> unsupported for old GPUs and systems that don't support Vulkan, 
>>>>>>> but the
>>>>>>> API is half a decade old at this point, and only GPUs from almost a
>>>>>>> decade ago don't support it.
>>>>>>
>>>>>> There is no free software NVidia Vulkan driver, which is a rather
>>>>>> glaring hole. There also exist cards released as recently as 2014 
>>>>>> that
>>>>>> don't support Vulkan. And in general, although it's sometimes
>>>>>> convenient for the implementer to require the latest hardware, it's a
>>>>>> rather mean thing to do to users.
>>>>>>
>>>>>> Keep in mind that we need Direct3D shared resources for bug 44985,
>>>>>> which affects a very long list of applications.
>>>>>>
>>>>>> Now, with that in mind, two options I immediately see are:
>>>>>>
>>>>>> * Create all contexts as shared. It's not clear to me whether this 
>>>>>> has
>>>>>> negative consequences, however. It's also not clear to me whether 
>>>>>> this
>>>>>> can be done across processes.
>>>>>>
>>>>>> * Try to introduce an OpenGL extension that allows exporting file
>>>>>> descriptors.
>>>>>
>>>>> The second option definitely sounds better, as with the first you're
>>>>> still limited to in-process sharing, if my understanding is correct.
>>>>> Additionally, the case where you're using wined3d's d3d9 for example,
>>>>> create a shared resource, then try importing it into DXVK for 
>>>>> d3d10/11,
>>>>> would only work with the second solution from what I can see.  I 
>>>>> imagine
>>>>> that we could cover the vast majority of users by implementing the
>>>>> "vulkan helper" path that I suggested before, and quite easily
>>>>> transition into using an extension if/when needed.
>>>>>>
>>>>>>>
>>>>>>> Alternatively we could get try to use platform-specific buffer 
>>>>>>> sharing
>>>>>>> APIs like EGLStreams or GBM, or just support in-process shared
>>>>>>> resources
>>>>>>> through the eglCreateContext API, which allows the creation of 
>>>>>>> "shared
>>>>>>> contexts", in which the GPU resources are shared.
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> * Shared resources between d3d9-12 devices, when using wined3d or
>>>>>>>>>> libvkd3d on Windows
>>>>>>>>> Putting aside how rare and questionable of a use-case this must 
>>>>>>>>> be, I
>>>>>>>>> don't think this concerns any of my patches.  My future patches 
>>>>>>>>> would
>>>>>>>>> provide a way for translation layers on wine to store metadata
>>>>>>>>> alongside
>>>>>>>>> the host handlers, it doesn't have any correspondence to 
>>>>>>>>> Windows.  If
>>>>>>>>> you really wanted to (why?), you could probably create your own
>>>>>>>>> backing
>>>>>>>>> storage in a shared memory object and accept the caveats that come
>>>>>>>>> along
>>>>>>>>> with that.
>>>>>>>>
>>>>>>>> I suspect those of us who have worked with Direct3D for multiple
>>>>>>>> decades would take issue with "rare and questionable", but 
>>>>>>>> regardless,
>>>>>>>> part of the point here is to ask: *is* it possible to use a 
>>>>>>>> native set
>>>>>>>> of APIs, like perhaps the D3DKMT*() functions from gdi32, to share
>>>>>>>> resources? I.e. should the implementation live in gdi32 instead of
>>>>>>>> winevulkan/opengl32/wined3d? Just from a quick scan I see several
>>>>>>>> which look like the ones we want, and they're even documented.
>>>>>>>
>>>>>>> Could you link these APIs so I can look further into them?  I 
>>>>>>> conferred
>>>>>>> with Josh and he didn't find anything.
>>>>>>
>>>>>> Sure, here are some APIs that look promising, and are documented:
>>>>>>
>>>>>> * D3DKMTCreateAllocation()
>>>>>> * D3DKMTOpenResource()
>>>>>> * D3DKMTOpenResourceFromNtHandle()
>>>>>> * D3DKMTQueryResourceInfo()
>>>>>> * D3DKMTQueryResourceInfoFromNtHandle()
>>>>>
>>>>> So I discussed this with Josh and Georg, and the problems with these
>>>>> APIs for our usecases are
>>>>>
>>>>> 1) In order to interface with any of these APIs, you need a KMT device
>>>>> handle.  This roughly corresponds to a Vulkan device object or OpenGL
>>>>> context, but according to Josh they don't have to map 1:1.
>>>>
>>>> It's a bit unfortunate that this discussion still isn't happening in a
>>>> public place,
>>>
>>> I'm sorry, but I get very annoyed by comments like these when at the
>>> same time most of wine's development happens behind closed doors inside
>>> codeweavers. It's not like wine-devel is used for architecture
>>> discussion or anything like that, no it's just patches and sometimes
>>> reviews. And somehow you now criticize me (well, us, but I can only
>>> speak for myself) for not communicating only via wine-devel? I send
>>> patches via wine-devel and I review patches on wine-devel, just like
>>> everyone else.
>>
>> So for what it's worth—and I'm not speaking for CodeWeavers here—I 
>> don't think "most of wine's development" happens anywhere at all. 
>> Large-scale architectural decisions aren't really discussed often. But 
>> if they are, they're usually discussed on wine-devel, Bugzilla, 
>> #winehackers, WineConf, or privately via e-mail.
>>
>> The last is a bit unfortunate, but it's also not specific to 
>> CodeWeavers—I can think of two people with whom I'm currently engaged 
>> in architectural discussion, and only one of those is a CodeWeavers 
>> employee.
>>
>> And I get the impression that CodeWeavers, or at least its employees, 
>> try to encourage discussing things openly where possible (not 
>> constrained by NDA, etc.) And when it comes to patches, we *are* 
>> encouraged and even expected to explain ourselves from the point of 
>> view of an outsider; see [2] for example.
>>
>> By contrast, when I tried to get the VKx community to discuss things 
>> openly, it was made quickly clear to me that they were not interested 
>> in using official Wine channels or coöperating with upstream Wine 
>> developers, but rather wanted Wine developers to use their channels 
>> (and their development practices, and their forks, etc...)
>>
>> So I'm being more than a little passive-aggressive, and I'm sorry for 
>> that. But I also really do want to point out that the people most 
>> involved in Direct3D/GPU work in Wine have not been involved in this 
>> discussion at all, and before some point in this reply chain they/we 
>> had no idea it was even happening. And as a result when a submitter 
>> acts like some fact is obvious, to the point of not explaining it at 
>> all, just because they discussed it privately with someone else, it's 
>> more than a little frustrating.
> The only thing I have to say here is that my experiences working for 
> codeweavers give me a different impression.  We regularly get updated on 
> what everyone is working on, and have been shared much more information 
> about the ongoing PE/unix architectural rework than the outside community.
>>
>> The bottom line is: it's really not clear that this patch set is 
>> taking the right approach. Not "it's taking the wrong approach", but 
>> it's really not clear that it's taking the right one. And most of that 
>> unclarity comes from a lack of public discussion, or even private 
>> discussion with the Wine contributors involved in Direct3D/GPU APIs.
> I'd like to reiterate here that this current patchset is not opinionated 
> on the approach we end up taking for D3D resources.  It simply 
> implements 1:1 an exclusively Vulkan/GL feature with the almost 
> identical corresponding Unix API.  The discussions we are having about 
> what to do in those cases are important, but they don't affect whether 
> or not this current patch series should be merged. If you disagree with 
> that statement, please refer to the code that would have to be 
> substantially changed in any thus-far proposed approach to the greater 
> problem, so that we can address it separately from the broader 
> discussion of how we want to handle resources and interop with D3D.
>>
>> [2] https://www.winehq.org/pipermail/wine-devel/2019-June/147452.html
>>
>>>
>>> Anyway, back to the actual discussion.
>>>
>>>> but anyway, we already use KMT device handles; see e.g.
>>>> wined3d_output_init().
>>>>
>>>>>
>>>>> 2) There's no way to update the private runtime data once the resource
>>>>> is created.  In the current and simpler flow, the host graphics API 
>>>>> will
>>>>> create the object, and the translation layer will then attach the 
>>>>> needed
>>>>> private data.  For this to work on windows, the translation layers 
>>>>> would
>>>>> have to agree on some way to get Vulkan to set the desired private 
>>>>> data,
>>>>> and that's assuming that no Windows drivers already use this for their
>>>>> own purposes.
>>>>
>>>> Part of my point here is that, ideally, we shouldn't need to. 
>>>> Presumably
>>>> if a Win32 application creates a Vulkan resource and tries to import it
>>>> into Direct3D, the Microsoft Direct3D runtime will call such functions
>>>> to get information from it. It seems at least plausible that we 
>>>> could do
>>>> the same.
>>>>
>>>
>>> You can *not* import a Vulkan resource into Direct3D.
>>> The vulkan specification [1] says:
>>>
>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT specifies an NT handle
>>> that has only limited valid usage outside of Vulkan and other compatible
>>> APIs. It must be compatible with the functions DuplicateHandle,
>>> CloseHandle, CompareObjectHandles, GetHandleInformation, and
>>> SetHandleInformation.
>>>
>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT_BIT specifies a global
>>> share handle that has only limited valid usage outside of Vulkan and
>>> other compatible APIs. It is not compatible with any native APIs.
>>>
>>>
>>> Note the "outside of Vulkan and other compatible APIs" part, which to my
>>> knowledge means Vulkan and the OpenGL import extension.
>>>
>>> These handles also can *not* include the info needed for e.g. D3D11
>>> because they are memory objects, not image/buffer objects. If you bind
>>> e.g. a 2D image to the memory, you don't know the width, the height or
>>> the format on import because it's *just* memory.
>>>
>>>
>>> [1]
>>> https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#VkExternalMemoryHandleTypeFlagBits 
>>>
>>
>> That may be true for the OPAQUE bit, but that's not the only bit. E.g. 
>> as far as I can tell from my reading of the Vulkan specification, 
>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT isn't forbidden from 
>> being specified in VkExportMemoryAllocateInfo, and the language even 
>> seems to "explicitly imply", as it were, that it's allowed. See the 
>> Vulkan 1.2 specification § 11.2.4 in particular.
>>
>> Granted, the documentation for 
>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT itself (no section 
>> number, I guess?) says 
>> "VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT specifies an NT 
>> handle returned by IDXGIResource1::CreateSharedHandle referring to a 
>> Direct3D 10 or 11 texture resource," which is a bit unclear. I haven't 
>> actually tested whether it works in practice, either, although of 
>> course even if it doesn't now it might in the future.
> 
> Good catch, I think we overlooked this possibility before.  I wrote some 
> tests to see whether any drivers support this on windows, and it seems 
> they don't.  (When querying the properties of sharing an image through 
> VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT, both the NVIDIA and 
> AMD windows drivers report only 
> VK_EXTERNAL_MEMORY_FEATURE_DEDICATED_ONLY_BIT|VK_EXTERNAL_MEMORY_FEATURE_IMPORTABLE_BIT). 
> 
> 
> It seems that we could however expose this functionality in wine, and 
> then write the relevant translation layer code that would work on 
> windows if this were ever to be supported by a windows driver. 
> Nevertheless, I'm still leaning against using this approach, as
> 
> 1) It requires us to reverse engineer the undocumented runtime data 
> format, which has a good chance of changing from windows version to 
> windows version.  I doubt that any windows applications have any reason 
> to use these APIs when they can just go through D3D11 to get the 
> information they want in a well-defined manner, meaning Microsoft 
> probably doesn't have to worry about changing this.
> 
> 2) This approach would require us to wrap the VkImage structure in 
> winevulkan, since it is inferred that the runtime data we need is 
> derived from VkImage used in the dedicated+shared allocation. (The spec 
> doesn't specify this, but it's the only possible way this data is shared 
> with the call creating the shared resource). According the Georg, doing 
> this would slow down some critical hotpath functions, namely the 
> descriptor update functions.
> 

But I also think if we actually want to go down that path we could find 
other ways to store data per VkImage. VK_EXT_private_data is the most 
simplistic one at the cost of requiring that it's available if we want 
to expose VK_KHR_external_memory_win32.