[RFC PATCH 3/4] winevulkan: Add support for named video resources.

[Derek Lesho]

On 5/4/21 6:50 PM, Zebediah Figura (she/her) wrote:
> On 5/4/21 2:39 PM, Derek Lesho wrote:
>>
>> On 5/3/21 7:54 PM, Zebediah Figura (she/her) wrote:
>>> On 5/3/21 4:08 PM, Derek Lesho wrote:
>>>> On 5/3/21 4:14 PM, Zebediah Figura (she/her) wrote:
>>>>
>>>>> Hello Derek, it looks like you've put a lot of thought into this
>>>>> problem. I assume you've already discussed this privately in detail
>>>>> with our Direct3D maintainers, but for the benefit of the rest of us,
>>>>> I have a few questions:
>>>>>
>>>>>
>>>>> 1. How do you plan to accomodate the following (whether in the 
>>>>> near or
>>>>> distant future):
>>>>>
>>>>> * CL_KHR_d3d11_sharing
>>>>>
>>>>> * CL_KHR_gl_sharing
>>>>>
>>>>> * Shared resources in d3d9-11, when using the OpenGL backend
>>>>>
>>>>> * Shared resources in d3d12
>>>> My plan is to add an optional map inside the resource objects in which
>>>> graphics API implementations (DXVK, vkd3d-proton, wined3d ...) and 
>>>> wine
>>>> layers (openGL, openCL) can store extra data as needed. There would be
>>>> an interface in the driver which takes a key (probably a GUID) and
>>>> either accept or return an arbitrary blob of data.  The benefit to 
>>>> this
>>>> approach, in my view, is that it would allow some flexibility as
>>>> requirements are understood and evolve for sharing between all these
>>>> APIs.  For example, at first, DXVK might roll its own data structure /
>>>> interface for sharing between its D3D9 and D3D11 implementations, and
>>>> maybe later on as the problem is better understood, an interface could
>>>> come into being for sharing between DXVK's d3d11 and wined3d's d3d9.
>>>
>>> I was kind of hoping for some more specific answers, i.e. rough
>>> descriptions of what the code does. Nevertheless, according to my
>>> understanding, this really doesn't account for all of the specifics of
>>> each API I've listed. Put simply, I'm worried that this patch set fits
>>> Vulkan and nothing else.
>> I admit that I haven't thought about openCL or D3D12 much, but for the
>> other use-cases I'm really not sure what problems you are thinking of.
>> Could you perhaps explain what concerns you have with them? From my
>> perspective, all I've constructed is a generic way to associate data
>> with objects, and a way to look the objects up by name and KMT handle.
>> The only way I could see this not working would be if the FDs output by
>> hosts APIs weren't compatible.
>
> Ideally this sort of research would be done by the patch submitter, 
> but anyway, one problem is that unlike Vulkan, OpenGL has no API to 
> export a resource. As far as I can see, from just a bit of research, 
> in order to share OpenGL resources one must create the destination 
> context as "shared" with the source. It's not clear at all that "a 
> generic way to associate data with objects" is enough to accomodate this.

 From what I've seen, openGL has accumulated many different ways to 
share resources over the years.  GL_EXT_memory_object_win32 seems to be 
the extension analogous to the VK_EXT_external_memory_win32, and it 
seems to correspond quite nicely, with HANDLEs being compatible between 
openGL and Vulkan users, as well as fds being compatible on the host 
side.  The only mention of gl context related shared resources that I 
see is from a stack overflow post where egl is used, and I think that is 
a system unto itself (not related to the broader windows environment).

https://stackoverflow.com/questions/31954439/can-i-share-a-external-texture-between-2-opengl-contexts-android

>
>>>
>>>>>
>>>>> * Shared resources between d3d9-12 devices, when using wined3d or
>>>>> libvkd3d on Windows
>>>>>
>>>>> * Shared resources between d3d9-12 devices and OpenGL or Vulkan, when
>>>>> using wined3d or libvkd3d on Windows
>>>> I decided not to worry about the situation on windows, as it seems 
>>>> much
>>>> more complex to me.  There seems to be a number of sparsely documented
>>>> D3DKMT APIs for tracking information about shared resources, and it
>>>> seemed to me that it would be improper to try to implement these
>>>> interfaces given how different the stack is on wine.  As for a custom
>>>> interface unrelated to the native one working on Windows, I'm 
>>>> really not
>>>> sure how feasible that would be, and what benefits it would 
>>>> provide.  We
>>>> need to be able to associate metadata to the objects underlying the
>>>> HANDLEs, as well as provide a global lookup for these objects, all in
>>>> usermode.  You could try implementing a global memory section that
>>>> stores this metadata associated to KMT (global) handles, but you would
>>>> have trouble doing the same for regular NT handles, since as far as I
>>>> can see it is impossible to fetch a KMT handle from a regular NT 
>>>> handle
>>>> on the windows implementation.  If a solution does come about for this
>>>> problem, I doubt it would be a good fit for wine.
>>>
>>> I would strongly advise not throwing these possibilities out the
>>> window immediately.
>> I just get the impression that if there were a good way to implement
>> this stuff without support from wine, the authors of DXVK would have
>> figured it out by now given how it's one of the last major hurdles for
>> the project.  It has been discussed quite a bit.
>
> This is a good example of why such discussion should not happen in a 
> private, secluded space (and, worse, without including any of the 
> several Wine developers actually involved in Direct3D)
The VKx Discord is public.  https://discord.gg/mjWm8DK .  Anyone is 
welcome to join, the discussions around stuff like this would probably 
happen in the channels with the -dev suffix.
> , or, if it must, why the results and thought process should be fully 
> explained. I don't avow a better knowledge of these APIs than said 
> authors, but even if I were to limit myself to Vulkan, and forget 
> about OpenGL, it's not obvious to me at all why this can't work.
  I'm not sure what you're referring to by `this`, do you mean the idea 
I mentioned of using a global memory section, or trying to work with the 
Windows KMT APIs?
>
>>>
>>>>>
>>>>>
>>>>> 2. What is the reason for the "weak" reference counting introduced in
>>>>> this patch?
>>>> In wine's ntoskrnl's object manager code, object structure data isn't
>>>> freed until there are no more references to the object in the server.
>>>> This is because a driver could reasonably expect that even though they
>>>> don't hold any references to an object, they can expect for their
>>>> pointer to valid if they or anyone else holds a handle to the object.
>>>> The weak references allow a driver to safely store their own data
>>>> alongside a weakly-referenced object, as with a weak reference, the
>>>> destruction of the object structure will be deferred until there 
>>>> are no
>>>> more weak references.  The reason why the driver can't just hold a 
>>>> full
>>>> reference is that it has no indication on when to free it, an
>>>> application can request a HANDLE for a graphics resource, then
>>>> completely de-initialize the graphics API, keeping the HANDLE. That is
>>>> to say, there's no single point in the winevulkan code where we can 
>>>> make
>>>> a call to the driver to free that object.  On the other hand, if the
>>>> driver were to keep no reference to the object, but just the 
>>>> pointer, it
>>>> would have no way of knowing whether the object pointers are still 
>>>> valid
>>>> when the client asks for information about an object.
>>>
>>> The code, as it stands, looks all sorts of wrong. The right approach
>>> would be to let the server notify ntoskrnl of destruction, the way it
>>> already does.
>> In a sense, this is what the current approach does.  It's just that
>> instead of executing code straight from the thread that received the
>> DISPATCH_FREE irp, it deduces whether or not that DISPATCH_FREE irp has
>> come yet by finding out whether the object has any real references
>> left.  I'm not claiming that it's the most idiomatic approach, but it's
>> a quite unintrusive and small helper that gives us the functionality we
>> need.
>
> Well, yes, that's exactly what I'm saying looks wrong. It's the sort 
> of thing that looks like a hack bolted on to the existing code rather 
> than fitting into its design (and, if necessary, changing that design 
> to accomodate). Lack of intrusiveness is rarely, in my experience, 
> worthwhile.
Upon closer inspection, I found an edge case where using device objects 
as a front-end for the file objects as you suggested would prevent 
problems, so it looks like we won't have to discuss which approach we 
find more desirable after all 😁.  When a Vulkan object is created to 
export KMT handles, the Vulkan resource always remains the sole 
reference to the underlying payload, so that once the initial object is 
destroyed, the only valid operations on the Vulkan objects that have 
imported the KMT handle is to destroy them.  In my initial 
implementation, I accepted that I didn't yet have a solution for this 
problem, as by virtue of holding the fd handles, the payload would be 
preserved by the driver.  If we add an object of indirection between the 
application visible handle and the fd handle, this should sort it self out.
>
>>>
>>> That's easier said than done, of course, and it takes some thought.
>>> One possible approach that comes to mind is to have shared resource
>>> handles be (NT) handles to device file objects. These files expose a
>>> pair of ioctls IOCTL_RESOURCE_SET_INFO and IOCTL_RESOURCE_GET_INFO,
>>> which store and retrieve a structure like:
>> Yeah, that could also work, I guess we would retain the global device
>> WineVideoResourceManager, and create the per-fd device objects via
>> IOCTLs to that global device.  Seems like it would accomplish
>> approximately the same thing with more code, with the advantage being
>> that it wouldn't include the weak reference code, which I assume is what
>> you consider all sorts of wrong.
>>>
>>> struct resource_info
>>> {
>>>     obj_handle_t fd_handle;
>>>     struct wined3d_resource_desc desc;
>>>     unsigned int layer_count;
>>>     unsigned int level_count;
>>> };
>> I still think it would be unwise to lock ourselves into a structure like
>> this.  Different APIs will have different requirements and this will
>> probably evolve some as we find more API surface to this.  It also makes
>> it a headache for out-of-tree projects to sync with whatever version of
>> the interface is in the wine they are running on top of.
>
> I'm not particularly advocating locking us into anything at the 
> moment, but quite frankly, that's the burden that said out-of-tree 
> projects must shoulder, by virtue of remaining out-of-tree (and, more 
> saliently, refusing to interact with upstream).
I don't think we need to discuss the politics and history behind the 
reasons why there exists out-of-tree projects to accept that, at the 
current moment, some of the most-used d3d implementations exist out of 
tree, and that if there's a way to accommodate them better at minimal 
cost to the wine project, it'd be preferable.
>
>>>
>>> "file" is itself a handle, which was created through
>>> wine_server_fd_to_handle(), and will be duplicated anew into the
>>> calling process through each IOCTL_RESOURCE_GET_INFO call. The
>>> remaining parameters are stored and returned transparently.
>>>
>>> When winevulkan needs to create a shared resource handle, it does
>>> something like this:
>>>
>>>     vkGetMemoryFdKHR(..., &fd);
>>>     file = CreateFile(...);
>> As I previously mentioned, I'm currently not aware of how we could
>> create a previously-unexisting, possibly unnamed object through
>> CreateFile.  If there's something I'm missing here, could you please
>> explain it?
>
> CreateFile internally creates a FILE_OBJECT, which can be accessed 
> from IRPs via the "FileObject" parameter. Private data can be 
> allocated and stored in the "FsContext" pointer. See http.sys for an 
> example.
I'm aware, my question is, what path would we provide to CreateFile?  
Even if there is a way, I think that using IOCTLs from a primary device 
would make more sense, maybe you can take a look at what I mean when I 
finish v2.
>
>>> resource_info.fd_handle = wine_server_fd_to_handle(fd);
>>>     /* fill resource_info.desc &c. */
>>>     DeviceIoControl(file, ..., &resource_info, ...);
>>>     CloseHandle(fd_handle);
>>>
>>> and eventually returns "file" from vkGetMemoryWin32HandleKHR().
>>>
>>> When wined3d, Vulkan, or OpenGL needs to open a shared resource
>>> handle, it uses IOCTL_RESOURCE_GET_INFO on that handle, followed by
>>> wine_server_handle_to_fd(). wined3d will subsequently use the other
>>> fields of struct resource_info; other APIs will ignore them.
>>>
>>> Extending this to objects which aren't resources (e.g. semaphores,
>>> fences) is left as an exercise to the reader; it could be done with a
>>> separate pair of ioctls or by extending the above structure.
>>>
>>> I will of course disclaim that I have thought this approach through
>>> completely, or that it is the best approach (in particular, I still am
>>> inclined to think that server-side management probably isn't that bad)
>> Yeah I'm not necessarily opposed to server-side management either, but
>> at the very least it seemed to me like it was off the table in 2019.
>
> I assume you're referring to the thread beginning with [2] (p.s. in 
> the future, it'd be nice to link to old mail conversations you're 
> referencing).
>
> Personally, I'd interpret statements like "you'll have to make a very 
> convincing argument that the existing primitives can't be used for 
> this" as exactly that: if it really is unreasonable to implement 
> something one way, explain why before trying to do it in a less 
> obvious way. Particularly unintuitive code can count as "unreasonable".
If you think the solution you laid out involving a device object layer 
of the file handle is intuitive, I guess we can consider this resolved?
>
>>> , but it seems like a reasonable way to use the existing ntoskrnl
>>> infrastructure and avoid any awful hacks.
>>>
>>> Note that this still doesn't handle OpenGL, and that's something that
>>> should really be resolved before anything is implemented.
>> Another advantage to the approach of letting the client APIs decide on
>> how to store metadata is that, as long as we know that OpenGL and Vulkan
>> (and any other potential host API) can export FDs which are compatible
>> with one another, we can be assured that our current implementation
>> isn't incompatible with the prospective implementation(/s) of the 
>> other API.
>>>
>>>>>
>>>>>
>>>>> 3. Why is winevideo.sys necessary at all, if
>>>>> wine_server_fd_to_handle() and wine_server_handle_to_fd() provide the
>>>>> necessary wrapping of an external FD object?
>>>> Because we need more information to be stored in the object behind the
>>>> HANDLE, and we need a way to lookup these objects globally. For
>>>> example, with D3D11 shared resources, we must be able to lookup and
>>>> object by its KMT handle and name, and we must be able to store extra
>>>> information about the object such as width, height, and layers, as
>>>> Vulkan doesn't do this for us.  Of course, we could figure add a 
>>>> way to
>>>> put these objects into the server's object namespace, but that still
>>>> leaves the KMT handles to worry about.
>>>>>
>>>>>
>>>>> 4. What kind of objects are Vulkan or Direct3D shared handles on
>>>>> Windows? E.g. what does NtQueryObject(ObjectTypeInformation) return?
>>>>> Are named Vulkan objects implemented using the NT namespace? How is
>>>>> this different between e.g. D3D11_RESOURCE_MISC_SHARED and
>>>>> D3D11_RESOURCE_MISC_SHARED_NTHANDLE, or between
>>>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT and
>>>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_KMT_BIT?
>>>> KMT handles are in their own namespace, completely separated from 
>>>> the NT
>>>> object system.  None of the usual object APIs work with them. The
>>>> objects behind the NT handles do have a custom type, and named Vulkan
>>>> objects are implemented using the NT namespace.  This is not reflected
>>>> in the implementation I sent.  I previously sent a patchset to 
>>>> implement
>>>> these resources in 2019, and back then, the concern was that
>>>> implementing these custom object types complicated wineserver code
>>>> unnecessarily since the fd<->HANDLE APIs were already present.  
>>>> This is
>>>> the reason why I had to come up with the new approach.
>>>
>>> That is an answer to one of the questions I asked, though not really
>>> the others.
>>>
>>> In particular, I suspect (especially given [1]) that the name
>>> parameter is really an NT name, or maps to one, which means that name
>>> assignment and lookup should be done in a way consistent with other NT
>>> objects.
>> In my previous email I did note that "named Vulkan objects are
>> implemented using the NT namespace".  In my 2019 version, I did
>> implement naming of these objects through the NT namespace, but since
>> this implementation doesn't have it's own wineserver type, it's not
>> possible without adding such functionality to the existing fd type used
>> for wine_server_fd_to_handle and wine_server_handle_to_fd.  One problem
>> with adding this functionality is that these ntdll APIs are public so
>> we'd probably have to add an additional wine_server_fd_to_named_handle
>> function for this.  While it would be nice to be more accurate and put
>> these objects into the NT namespace, in practice I don't think any
>> applications are looking up these objects by any other method than using
>> the graphics APIs.
>
> Accuracy is not so much a concern to me as is leveraging existing 
> lookup code rather than implementing our own.
This should be resolved with the device object layer approach I think.
>>>
>>>>>
>>>>>
>>>>> 5. Are D3D11_RESOURCE_MISC_SHARED / KMT handles cross-process? If 
>>>>> not,
>>>>> should they be implemented using a process-local handle table
>>>>> somewhere instead of using NT handles? (Could/should we use D3DKMT*
>>>>> APIs from gdi32?)
>>>> I'd always assumed that they were, and I must admit I wrote the 
>>>> rest of
>>>> this email under that assumption.  I'll write tests tomorrow to find
>>>> out.  If they aren't, that would indeed make things a lot simpler...
>>>
>>> On the flip side of this, is there any reason that we can't just
>>> pretend NT handles are KMT handles?
>>
>> I'm assuming you mean returning NT handles when the app requests a KMT
>> handle?  If so, that wouldn't work as I've confirmed that KMT handles
>> are indeed global.  Additionally, Vulkan specifies that KMT handles
>> don't hold a reference to the underlying object, as soon as the Vulkan
>> object is freed, the KMT handles become invalid.
>>
>>>
>>>>>
>>>>>
>>>>> 6. Would it make more sense to use wineserver to manage shared
>>>>> resources instead of a separate driver, and thereby avoid introducing
>>>>> hacks into ntoskrnl?
>>>> I think I addressed this in my response to question 4.
>>>>
>>>
>>>
>>> [1]
>>> https://docs.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12device-createsharedhandle 
>>>
>>>
>>
>
> [2] https://www.winehq.org/pipermail/wine-devel/2019-November/153787.html
>