[PATCH 1/4] winevulkan: Use standard CRT memory allocators.
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- In preparation for split PE/.so parts. dlls/winevulkan/make_vulkan | 8 +-- dlls/winevulkan/vulkan.c | 111 ++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 59 deletions(-)
On 13.04.21 00:53, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- In preparation for split PE/.so parts.
dlls/winevulkan/make_vulkan | 8 +-- dlls/winevulkan/vulkan.c | 111 ++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 59 deletions(-)
Hi, could you give an overview of all the changes that you are planning to make for the .so/PE split? winevulkan had quite a few contributors over the past years, and I think those who aren't part of codeweavers aren't super familiar with wine internals. I would certainly like to know more about the implications of this rework. Thanks, Georg Lehmann
Hi Georg, On 13.04.2021 09:52, Georg Lehmann wrote:
On 13.04.21 00:53, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- In preparation for split PE/.so parts.
dlls/winevulkan/make_vulkan | 8 +-- dlls/winevulkan/vulkan.c | 111 ++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 59 deletions(-)
Hi,
could you give an overview of all the changes that you are planning to make for the .so/PE split? winevulkan had quite a few contributors over the past years, and I think those who aren't part of codeweavers aren't super familiar with wine internals. I would certainly like to know more about the implications of this rework.
Sure. Generally, Wine is moving towards stricter separation between PE/Windows side and Unix side. This already has a great effect on compatibility with stuff like API hooking, DRMs, debuggers etc. In the future, this will be also needed for stuff like 32on64. In practice, in its current shape, it means that winevulkan should be built as a PE file. As such, it may do Windows calls, but not direct Unix calls. For Unix calls, it will have a separated .so library. .so library exports its internal interface to PE side and can do any Unix calls. However, .so part should avoid calling PE side, so it's very limited to what win32 APIs it can use. #pragma makedep unix controls which side given source file lives on. In case of winevulkan, the transition is quite straightforward. Most of the code can live in .so part (namely, vulkan.c and vulkan_thunks.c). There are a few Windows calls that we need to be moved out of there and this series moves most of win32 calls out of vulkan.c. We will also need dispatch tables and exported functions on PE part, so that will need another thin layer of thunking. Using wine_vkResetEvent as an example, in my WIP tree, there is a new PE function for PE side: VkResult WINAPI wine_vkResetEvent(VkDevice device, VkEvent event) { return unix_funcs->p_vkResetEvent(device, event); } That's what applications can see, while unix_funcs->p_vkResetEvent points to the existing vkResetEvent thunk living in .so part: VkResult WINAPI unix_vkResetEvent(VkDevice device, VkEvent event) { TRACE("%p, 0x%s\n", device, wine_dbgstr_longlong(event)); return device->funcs.p_vkResetEvent(device->device, event); } Thanks, Jacek
On 4/13/21 1:45 PM, Jacek Caban wrote:
Hi Georg,
On 13.04.2021 09:52, Georg Lehmann wrote:
On 13.04.21 00:53, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- In preparation for split PE/.so parts.
dlls/winevulkan/make_vulkan | 8 +-- dlls/winevulkan/vulkan.c | 111 ++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 59 deletions(-)
Hi,
could you give an overview of all the changes that you are planning to make for the .so/PE split? winevulkan had quite a few contributors over the past years, and I think those who aren't part of codeweavers aren't super familiar with wine internals. I would certainly like to know more about the implications of this rework.
Sure. Generally, Wine is moving towards stricter separation between PE/Windows side and Unix side. This already has a great effect on compatibility with stuff like API hooking, DRMs, debuggers etc. In the future, this will be also needed for stuff like 32on64.
How do you plan to deal with vkMapMemory with 32on64? You can't simply mremap that into 32-bit space as the pointer gets stored to be deduplicated and may also be stored and used by the Vulkan driver: ( https://gitlab.freedesktop.org/mesa/drm/-/blob/master/amdgpu/amdgpu_bo.c#L47... [not amdgpu specific, they all do this, just an example] ) There is also no guarantee that it isn't from a suballocation also so the pointer you get may be offset from some other mmap. There may need to be extensions needed to vkMapMemory with a base address + flags, but that still doesn't fully solve the other problems unless we forced dedicated allocations for everything and did something about the deduping... This stuff also applies to mapped pointers returned by GL fwiw. Am I missing something obvious that makes this much simpler than I am thinking? Thanks! - Joshie 🐸✨
In practice, in its current shape, it means that winevulkan should be built as a PE file. As such, it may do Windows calls, but not direct Unix calls. For Unix calls, it will have a separated .so library. .so library exports its internal interface to PE side and can do any Unix calls. However, .so part should avoid calling PE side, so it's very limited to what win32 APIs it can use. #pragma makedep unix controls which side given source file lives on.
In case of winevulkan, the transition is quite straightforward. Most of the code can live in .so part (namely, vulkan.c and vulkan_thunks.c). There are a few Windows calls that we need to be moved out of there and this series moves most of win32 calls out of vulkan.c. We will also need dispatch tables and exported functions on PE part, so that will need another thin layer of thunking. Using wine_vkResetEvent as an example, in my WIP tree, there is a new PE function for PE side:
VkResult WINAPI wine_vkResetEvent(VkDevice device, VkEvent event) { return unix_funcs->p_vkResetEvent(device, event); }
That's what applications can see, while unix_funcs->p_vkResetEvent points to the existing vkResetEvent thunk living in .so part:
VkResult WINAPI unix_vkResetEvent(VkDevice device, VkEvent event) { TRACE("%p, 0x%s\n", device, wine_dbgstr_longlong(event)); return device->funcs.p_vkResetEvent(device->device, event); }
Thanks,
Jacek
On 13.04.2021 14:58, Joshua Ashton wrote:
How do you plan to deal with vkMapMemory with 32on64?
It's not yet a concern and not really related to module split itself, so I wasn't planning to deal with it (at least not yet).
You can't simply mremap that into 32-bit space as the pointer gets stored to be deduplicated and may also be stored and used by the Vulkan driver: ( https://gitlab.freedesktop.org/mesa/drm/-/blob/master/amdgpu/amdgpu_bo.c#L47... [not amdgpu specific, they all do this, just an example] )
There is also no guarantee that it isn't from a suballocation also so the pointer you get may be offset from some other mmap.
There may need to be extensions needed to vkMapMemory with a base address + flags, but that still doesn't fully solve the other problems unless we forced dedicated allocations for everything and did something about the deduping...
This stuff also applies to mapped pointers returned by GL fwiw.
Am I missing something obvious that makes this much simpler than I am thinking?
I'm not very familiar with Vulkan yet, but is there something fundamentally wrong with vkGetMemoryFdKHR and mmaping ourselves? Thanks, Jacek
On 4/13/21 3:00 PM, Jacek Caban wrote:
On 13.04.2021 14:58, Joshua Ashton wrote:
How do you plan to deal with vkMapMemory with 32on64?
It's not yet a concern and not really related to module split itself, so I wasn't planning to deal with it (at least not yet).
You can't simply mremap that into 32-bit space as the pointer gets stored to be deduplicated and may also be stored and used by the Vulkan driver: ( https://gitlab.freedesktop.org/mesa/drm/-/blob/master/amdgpu/amdgpu_bo.c#L47... [not amdgpu specific, they all do this, just an example] )
There is also no guarantee that it isn't from a suballocation also so the pointer you get may be offset from some other mmap.
There may need to be extensions needed to vkMapMemory with a base address + flags, but that still doesn't fully solve the other problems unless we forced dedicated allocations for everything and did something about the deduping...
This stuff also applies to mapped pointers returned by GL fwiw.
Am I missing something obvious that makes this much simpler than I am thinking?
I'm not very familiar with Vulkan yet, but is there something fundamentally wrong with vkGetMemoryFdKHR and mmaping ourselves?
Several things: There may be certain usage restrictions on memory that is exportable this way. It'll require a dedicated allocation for everything and we don't know ahead of the time buffer/image to associate it with (the app may allocate the memory first, then bind it to the image/buffer) so we are kinda SOL there without stupid tracking. (!) Sparse binding won't work due to requiring a dedicated allocation. (!) It also can also change how residency is dealt with (ie. local bo list vs global bo list). There may also be performance implications wrt residency and image compression given the assumption seems to be "this is for interprocess or interdevice sharing." There may be video memory fragmentation from disallowing suballocations and requiring dedicated allocations for every resource also. - Joshie 🐸✨
Thanks,
Jacek
On 13.04.21 16:00, Jacek Caban wrote:
On 13.04.2021 14:58, Joshua Ashton wrote:
How do you plan to deal with vkMapMemory with 32on64?
It's not yet a concern and not really related to module split itself, so I wasn't planning to deal with it (at least not yet).
You can't simply mremap that into 32-bit space as the pointer gets stored to be deduplicated and may also be stored and used by the Vulkan driver: ( https://gitlab.freedesktop.org/mesa/drm/-/blob/master/amdgpu/amdgpu_bo.c#L47... [not amdgpu specific, they all do this, just an example] )
There is also no guarantee that it isn't from a suballocation also so the pointer you get may be offset from some other mmap.
There may need to be extensions needed to vkMapMemory with a base address + flags, but that still doesn't fully solve the other problems unless we forced dedicated allocations for everything and did something about the deduping...
This stuff also applies to mapped pointers returned by GL fwiw.
Am I missing something obvious that makes this much simpler than I am thinking?
I'm not very familiar with Vulkan yet, but is there something fundamentally wrong with vkGetMemoryFdKHR and mmaping ourselves?
No, the fd return by vkGetMemoryFdKHR is only usable for imports via the driver. We could potentially propose an extension which gives us a mappable fd or one where we give the driver an address to map the memory to. Both options probably take a decent amount of work for the driver stacks. So if we need an extension it's better to start early on that. (I have no idea how far away 32on64 is)
On 4/13/21 3:41 PM, Georg Lehmann wrote:
On 13.04.21 16:00, Jacek Caban wrote:
On 13.04.2021 14:58, Joshua Ashton wrote:
How do you plan to deal with vkMapMemory with 32on64?
It's not yet a concern and not really related to module split itself, so I wasn't planning to deal with it (at least not yet).
You can't simply mremap that into 32-bit space as the pointer gets stored to be deduplicated and may also be stored and used by the Vulkan driver: ( https://gitlab.freedesktop.org/mesa/drm/-/blob/master/amdgpu/amdgpu_bo.c#L47... [not amdgpu specific, they all do this, just an example] )
There is also no guarantee that it isn't from a suballocation also so the pointer you get may be offset from some other mmap.
There may need to be extensions needed to vkMapMemory with a base address + flags, but that still doesn't fully solve the other problems unless we forced dedicated allocations for everything and did something about the deduping...
This stuff also applies to mapped pointers returned by GL fwiw.
Am I missing something obvious that makes this much simpler than I am thinking?
I'm not very familiar with Vulkan yet, but is there something fundamentally wrong with vkGetMemoryFdKHR and mmaping ourselves?
No, the fd return by vkGetMemoryFdKHR is only usable for imports via the driver.
I had not considered that the memory FD returned may not actually be mappable. I guess the spec doesn't provide any guarantees other than "you can import this into another VkDeviceMemory" on closer look. I guess that's another concern... 🐸 - Joshie 🐸✨
We could potentially propose an extension which gives us a mappable fd or one where we give the driver an address to map the memory to. Both options probably take a decent amount of work for the driver stacks. So if we need an extension it's better to start early on that. (I have no idea how far away 32on64 is)
Thanks for the explanation! On 13.04.21 14:45, Jacek Caban wrote:
Hi Georg,
On 13.04.2021 09:52, Georg Lehmann wrote:
On 13.04.21 00:53, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> --- In preparation for split PE/.so parts.
dlls/winevulkan/make_vulkan | 8 +-- dlls/winevulkan/vulkan.c | 111 ++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 59 deletions(-)
Hi,
could you give an overview of all the changes that you are planning to make for the .so/PE split? winevulkan had quite a few contributors over the past years, and I think those who aren't part of codeweavers aren't super familiar with wine internals. I would certainly like to know more about the implications of this rework.
Sure. Generally, Wine is moving towards stricter separation between PE/Windows side and Unix side. This already has a great effect on compatibility with stuff like API hooking, DRMs, debuggers etc. In the future, this will be also needed for stuff like 32on64.
In practice, in its current shape, it means that winevulkan should be built as a PE file. As such, it may do Windows calls, but not direct Unix calls. For Unix calls, it will have a separated .so library. .so library exports its internal interface to PE side and can do any Unix calls. However, .so part should avoid calling PE side, so it's very limited to what win32 APIs it can use. #pragma makedep unix controls which side given source file lives on.
In case of winevulkan, the transition is quite straightforward. Most of the code can live in .so part (namely, vulkan.c and vulkan_thunks.c). There are a few Windows calls that we need to be moved out of there and this series moves most of win32 calls out of vulkan.c. We will also need dispatch tables and exported functions on PE part, so that will need another thin layer of thunking. Using wine_vkResetEvent as an example, in my WIP tree, there is a new PE function for PE side:
VkResult WINAPI wine_vkResetEvent(VkDevice device, VkEvent event) { return unix_funcs->p_vkResetEvent(device, event); }
That's what applications can see, while unix_funcs->p_vkResetEvent points to the existing vkResetEvent thunk living in .so part:
VkResult WINAPI unix_vkResetEvent(VkDevice device, VkEvent event) { TRACE("%p, 0x%s\n", device, wine_dbgstr_longlong(event)); return device->funcs.p_vkResetEvent(device->device, event); }
This sounds like it could potentially negatively affect performance up to a measurable degree due to another indirection. But I guess there's no way to have a function in the unix library (with the correct calling convention and everything) and to return that from the PE in vkGet*ProcAddr? Thanks, Georg
On 13.04.2021 17:16, Georg Lehmann wrote:
This sounds like it could potentially negatively affect performance up to a measurable degree due to another indirection. But I guess there's no way to have a function in the unix library (with the correct calling convention and everything) and to return that from the PE in vkGet*ProcAddr?
That's not impossible for now, but it would be more problematic as we move forward with the separation. Unix part will be 'hidden' from applications in a way that's similar in a lot of aspects to Windows kernel calls. With that analogy, we can't just return a 'kernel' pointer. Thanks, Jacek
On 4/13/21 18:33, Jacek Caban wrote:
On 13.04.2021 17:16, Georg Lehmann wrote:
This sounds like it could potentially negatively affect performance up to a measurable degree due to another indirection. But I guess there's no way to have a function in the unix library (with the correct calling convention and everything) and to return that from the PE in vkGet*ProcAddr?
That's not impossible for now, but it would be more problematic as we move forward with the separation. Unix part will be 'hidden' from applications in a way that's similar in a lot of aspects to Windows kernel calls. With that analogy, we can't just return a 'kernel' pointer.
Maybe we can link most of the native Vulkan functions directly to the unix_table, avoiding unix side wrapper and thus an extra call?
On 13.04.2021 17:35, Paul Gofman wrote:
On 4/13/21 18:33, Jacek Caban wrote:
On 13.04.2021 17:16, Georg Lehmann wrote:
This sounds like it could potentially negatively affect performance up to a measurable degree due to another indirection. But I guess there's no way to have a function in the unix library (with the correct calling convention and everything) and to return that from the PE in vkGet*ProcAddr?
That's not impossible for now, but it would be more problematic as we move forward with the separation. Unix part will be 'hidden' from applications in a way that's similar in a lot of aspects to Windows kernel calls. With that analogy, we can't just return a 'kernel' pointer.
Maybe we can link most of the native Vulkan functions directly to the unix_table, avoiding unix side wrapper and thus an extra call?
Unix calling conventions and Unix structure layouts are known only to Unix part. Jacek
participants (4)
-
Georg Lehmann -
Jacek Caban -
Joshua Ashton -
Paul Gofman