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

[Derek Lesho]

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.

>
>>
>>> There may be any number of foreseen and unforeseen pitfalls along the
>>> way, which could prevent this approach from working at all, but my 
>>> point
>>> is, it's worth testing—and I mean actually testing, not just theorizing
>>> about what we can and can't do. And even if it doesn't work on Windows,
>>> it still bears asking whether we should use these APIs for Wine.
>>>
>>
>> Sorry, I just fail to see what's the point of using D3DKMT is. Unless I
>> miss some detail, this won't make any of this work on windows, and if
>> you somehow found a way to do make it work (which I think does not
>> exists), you would violate the vulkan specification and rely on
>> undefined behavior.
>>
>> And then that makes me ask the question: Why should we use this API in
>> Wine, what's the benefit?
>>
>>> And, if the answer ends up being yes, we should start there when
>>> implementing them. It's far better to plan out the final architecture
>>> and implement things accordingly, when possible, rather than to start
>>> implementing things one way and then have to reorganize them later.
>>>
>>
>> This patch set is 99% setting up winevulkan for translation of the Win32
>> external memory extension to the Unix fd external memory extension. You
>> have to do that either way, and it won't change regardless of how
>> ultimately get your NT handle for a Unix fd. It's also just vulkan <->
>> vulkan sharing (and I guess OpenGl if someone wants to implement the
>> import extension in Wine), it does *not* relate to Direct3D at all.
>
> I was hoping this would be evident by now, but Vulkan does not exist 
> in a vacuum, as far as Wine is concerned. Neither does 
> VK_KHR_external_memory_win32. Vulkan can at the very least import 
> resources from Direct3D, and export resources to OpenGL. Those in turn 
> have more interop and intra-op (as it were) capabilities. And even if 
> there were no interop, it's not unlikely that we'd want to consider 
> how to make intra-op work in a generic way. Even the initial version 
> of this patch set was submitted with that in mind.
The shared resource primitives implemented in this patchset do exist in 
vacuum, together with the GL extension.
>
> Yes, it's a hard problem, and a lot of the hard things about it come 
> from making *all* of the interop and intra-op work at once. It would 
> be much easier if we could just submit this patch series and claim 
> we're done, if only for now. But Wine doesn't really follow this 
> development model: we much prefer to get the design right from the 
> beginning, no matter how long it takes, so that we don't have to fix 
> it much later, when it'll be much more painful.

>
>>>>>> Also, if there are APIs which allow registering resources retrieved
>>>>>> from
>>>>>> Vulkan with certain metadata, winevulkan would still have roughly 
>>>>>> the
>>>>>> same role.  The only difference would be in the translation layers,
>>>>>> where instead of using IOCTLs on the HANDLEs to shared resources 
>>>>>> they
>>>>>> have to get/set metadata, they would use some D3DKMT API you've 
>>>>>> found.
>>>>>
>>>>> Well, broadly, yes, but it changes the way that this patch series
>>>>> should be structured and sent.
>>>>
>>>> This patch's main focus is just setting up the winevulkan 
>>>> infrastructure
>>>> for shared resources.  If we decide to still use D3DKMT for one reason
>>>> or another, the only change to the current patchset would be to the 
>>>> tiny
>>>> function create_gpu_resource to get the HANDLE through KMT APIs 
>>>> instead
>>>> of wine_server_fd_to_handle.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> I note your tests also don't seem very interesting; they only 
>>>>>>>>> share
>>>>>>>>> objects within the same device. Sharing objects across multiple
>>>>>>>>> devices, and across multiple processes, strikes me as more
>>>>>>>>> interesting.
>>>>>>>>
>>>>>>>> In my opinion, given that the semantics for 
>>>>>>>> VK_KHR_external_memory_fd
>>>>>>>> and VK_KHR_external_memory_win32 are so similar, I wonder how
>>>>>>>> relevant
>>>>>>>> such tests would be, it seems like the kind of thing the Vulkan 
>>>>>>>> CTS
>>>>>>>> should add for testing drivers.
>>>>>>>>
>>>>>>>> I think the tests should be more for testing the properties of the
>>>>>>>> HANDLEs we get, as this is what we are implementing ourselves.
>>>>>>>> Maybe I
>>>>>>>> should add more tests looking at the locations of the named
>>>>>>>> objects in
>>>>>>>> the NT directory, or tests relating to types of the objects and 
>>>>>>>> their
>>>>>>>> access permissions.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> The point is more of a smoke test, because it's easy to get it 
>>>>>>> wrong,
>>>>>>> and not too hard to verify that we got it right.
>>>>>>>
>>>>>>> It'd also be nice to demonstrate what the behaviour of KMT 
>>>>>>> handles is
>>>>>>> across processes.
>>>>>>>
>>>>>> An in-process intra-device test tests the same amount of winevulkan
>>>>>> behavior as a cross-process inter-device test.  All the 
>>>>>> implementation
>>>>>> details of cross-process and cross-device sharing are handled on the
>>>>>> host and through wineserver's well established HANDLE<->FD mapping.
>>>>>>
>>>>>
>>>>> Is it really that hard just to add a test?
>>>>
>>>>
>>>> No, but I just think it would be a waste of time and lines of code,
>>>> especially for testing cross-process when it makes no difference which
>>>> processes the two devices are made on.  Given that you said "a 
>>>> test", do
>>>> you mean you think it would be fine if I were to just add a test 
>>>> testing
>>>> shared resources across devices in the same process?
>>>>
>>>>
>>>
>>> It's not that hard to write a cross-process test; see kernel32:pipe for
>>> an example. We have infrastructure for it.
>>>
>>> For a rationale: even if the code path isn't significantly different
>>> now, it might be in the future, especially when this patch series seems
>>> far from maturity. And, like I said, we have no tests for KMT handles
>>> across processes.
>>>
>>
>> FWIW since this patch set only implements NT handles, it might be better
>> to just not add any KMT handle tests in this patch set at all.
>>
>