[PATCH 0/4] MR11343: opengl32: Create pinned or virtual buffer storage for large glBufferData.
From: Rémi Bernon <rbernon@codeweavers.com> --- dlls/opengl32/unix_wgl.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index 0ba0282401d..98ee40a6343 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -2294,6 +2294,17 @@ static BOOL wow64_unmap_buffer( TEB *teb, struct buffer *buffer ) unmap_vk_buffer( buffer ); } + if (buffer->pinned) + { + if (!buffer->map_ptr) + { + set_gl_error( teb, GL_INVALID_OPERATION ); + return FALSE; + } + buffer->map_ptr = NULL; + return TRUE; + } + if (buffer->copy_length) { TRACE( "Copying %#zx from wow64 buffer %p to buffer %p\n", buffer->copy_length, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11343
From: Rémi Bernon <rbernon@codeweavers.com> --- dlls/opengl32/unix_wgl.c | 125 +++++++++++++++++++-------------------- 1 file changed, 61 insertions(+), 64 deletions(-) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index 98ee40a6343..22e09352b0d 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -1925,7 +1925,7 @@ static int find_vk_memory_type( struct vk_device *vk_device, uint32_t flags, uin return -1; } -static struct buffer *create_buffer_storage( TEB *teb, GLenum target, GLuint name, size_t size, const void *data, GLbitfield flags ) +static BOOL init_buffer_storage_vulkan( struct buffer *buffer, TEB *teb, GLenum target, GLuint name, size_t size, const void *data, GLbitfield flags ) { VkExportMemoryAllocateInfo export_alloc = { @@ -1944,103 +1944,100 @@ static struct buffer *create_buffer_storage( TEB *teb, GLenum target, GLuint nam .handleType = VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT, }; struct opengl_funcs *funcs = teb->glTable; - GLuint buffer_name = name ? name : get_target_name( teb, target ); uint32_t type_mask = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; uint32_t desired_type = type_mask; - struct context *ctx = get_current_context( teb, NULL, NULL ); struct vk_device *vk_device; - struct buffer *buffer; + VkDeviceMemory vk_memory; int fd, memory_type; VkResult vr; - if (!(flags & (GL_MAP_READ_BIT | GL_MAP_WRITE_BIT))) return NULL; - if ((!(vk_device = buffers.vk_device) || !vk_device->vk_device) && !ctx->use_pinned_memory) return NULL; - - if (!(buffer = calloc( 1, sizeof(*buffer) ))) return NULL; - buffer->name = buffer_name; - buffer->size = size; - buffer->vk_device = vk_device; - - if (ctx->use_pinned_memory) - { - if (!buffer_vm_alloc( teb, buffer, size )) return NULL; - if (data) memcpy( buffer->vm_ptr, data, size ); - buffer->pinned = TRUE; - - /* FIXME: we may interfere with GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD if the - * application uses it as well. Unlike other targets, there’s no way to query - * the currently bound target, so we’d need to track it ourselves if we want - * to support it. */ - funcs->p_glBindBuffer( GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD, buffer_name ); - funcs->p_glBufferData( GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD, size, buffer->vm_ptr, GL_DYNAMIC_COPY ); - TRACE( "created buffer %p with pinned memory %p\n", buffer, buffer->vm_ptr ); - return buffer; - } + if ((!(vk_device = buffers.vk_device) || !vk_device->vk_device)) return FALSE; if (flags & GL_CLIENT_STORAGE_BIT) desired_type &= ~VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; memory_type = find_vk_memory_type( vk_device, desired_type, type_mask ); if (memory_type == -1) /* if we can’t find a matching type, try ignoring GL_CLIENT_STORAGE_BIT */ memory_type = find_vk_memory_type( vk_device, desired_type, type_mask & ~VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT ); - if (memory_type == -1) - { - WARN( "Could not find memory type\n" ); - free_buffer( funcs, buffer ); - return NULL; - } + if (memory_type == -1) return FALSE; alloc_info.memoryTypeIndex = memory_type; - vr = vk_device->p_vkAllocateMemory( vk_device->vk_device, &alloc_info, NULL, &buffer->vk_memory ); - if (vr) - { - ERR( "vkAllocateMemory failed: %d\n", vr ); - free_buffer( funcs, buffer ); - return NULL; - } + if (vk_device->p_vkAllocateMemory( vk_device->vk_device, &alloc_info, NULL, &vk_memory )) return FALSE; if (data) { VkMemoryMapInfoKHR map_info = { .sType = VK_STRUCTURE_TYPE_MEMORY_MAP_INFO_KHR, - .memory = buffer->vk_memory, + .memory = vk_memory, .size = VK_WHOLE_SIZE, }; VkMemoryUnmapInfoKHR unmap_info = { .sType = VK_STRUCTURE_TYPE_MEMORY_UNMAP_INFO_KHR, - .memory = buffer->vk_memory, + .memory = vk_memory, }; void *ptr; - vr = vk_device->p_vkMapMemory2KHR( vk_device->vk_device, &map_info, &ptr ); - if (vr) - { - ERR( "vkMapMemory2KHR failed: %d\n", vr ); - free_buffer( funcs, buffer ); - return NULL; - } - + if ((vr = vk_device->p_vkMapMemory2KHR( vk_device->vk_device, &map_info, &ptr ))) goto failed; memcpy( ptr, data, size ); vk_device->p_vkUnmapMemory2KHR( vk_device->vk_device, &unmap_info ); } - fd_info.memory = buffer->vk_memory; - vr = vk_device->p_vkGetMemoryFdKHR( vk_device->vk_device, &fd_info, &fd ); - if (vr) - { - ERR( "vkGetMemoryFdKHR failed: %d\n", vr ); - free_buffer( funcs, buffer ); - return NULL; - } + fd_info.memory = vk_memory; + if ((vr = vk_device->p_vkGetMemoryFdKHR( vk_device->vk_device, &fd_info, &fd ))) goto failed; + + buffer->vk_device = vk_device; + buffer->vk_memory = vk_memory; funcs->p_glCreateMemoryObjectsEXT( 1, &buffer->gl_memory ); funcs->p_glImportMemoryFdEXT( buffer->gl_memory, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd ); - if (name) - funcs->p_glNamedBufferStorageMemEXT( buffer->name, size, buffer->gl_memory, 0 ); - else - funcs->p_glBufferStorageMemEXT( target, size, buffer->gl_memory, 0 ); - TRACE( "created buffer_storage %p\n", buffer ); - return buffer; + if (name) funcs->p_glNamedBufferStorageMemEXT( buffer->name, size, buffer->gl_memory, 0 ); + else funcs->p_glBufferStorageMemEXT( target, size, buffer->gl_memory, 0 ); + + TRACE( "created buffer %p with vulkan memory\n", buffer ); + return TRUE; + +failed: + vk_device->p_vkFreeMemory( vk_device->vk_device, vk_memory, NULL ); + return FALSE; +} + +static BOOL init_buffer_storage_pinned( struct buffer *buffer, TEB *teb, GLenum target, GLuint name, size_t size, const void *data, GLbitfield flags ) +{ + struct context *ctx = get_current_context( teb, NULL, NULL ); + struct opengl_funcs *funcs = teb->glTable; + + if (!ctx->use_pinned_memory) return FALSE; + if (!buffer_vm_alloc( teb, buffer, size )) return FALSE; + if (data) memcpy( buffer->vm_ptr, data, size ); + buffer->pinned = TRUE; + + /* FIXME: we may interfere with GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD if the + * application uses it as well. Unlike other targets, there’s no way to query + * the currently bound target, so we’d need to track it ourselves if we want + * to support it. */ + funcs->p_glBindBuffer( GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD, name ? name : get_target_name( teb, target ) ); + funcs->p_glBufferData( GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD, size, buffer->vm_ptr, GL_DYNAMIC_COPY ); + + TRACE( "created buffer %p with pinned memory %p\n", buffer, buffer->vm_ptr ); + return TRUE; +} + +static struct buffer *create_buffer_storage( TEB *teb, GLenum target, GLuint name, GLint size, const void *data, GLbitfield flags ) +{ + GLuint buffer_name = name ? name : get_target_name( teb, target ); + struct buffer *buffer; + + if (!(flags & (GL_MAP_READ_BIT | GL_MAP_WRITE_BIT))) return NULL; + if (!(buffer = calloc( 1, sizeof(*buffer) ))) return NULL; + buffer->name = buffer_name; + buffer->size = size; + + if (init_buffer_storage_vulkan( buffer, teb, target, name, size, data, flags )) return buffer; + if (init_buffer_storage_pinned( buffer, teb, target, name, size, data, flags )) return buffer; + + if (buffer->vm_ptr) NtFreeVirtualMemory( GetCurrentProcess(), &buffer->vm_ptr, &buffer->vm_size, MEM_RELEASE ); + free( buffer ); + return NULL; } static void *wow64_map_buffer( TEB *teb, struct buffer *buffer, GLenum target, GLuint name, GLintptr offset, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11343
From: Rémi Bernon <rbernon@codeweavers.com> --- dlls/opengl32/unix_wgl.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index 22e09352b0d..c333bea7e06 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -122,7 +122,6 @@ struct context UINT64 debug_user; /* client pointer */ GLubyte *extensions; /* extension string */ char *wow64_version; /* wow64 GL version override */ - BOOL use_pinned_memory; /* use GL_AMD_pinned_memory to emulate persistent maps */ /* semi-stub state tracker for wglCopyContext */ GLbitfield used; /* context state used bits */ @@ -1047,8 +1046,7 @@ static void make_context_current( TEB *teb, const struct opengl_funcs *funcs, HD TRACE( "-- %s (disabled by config)\n", all_extensions[i].name ); } - if (is_win64 && is_wow64() && !initialize_vk_device( teb, ctx ) - && !(ctx->use_pinned_memory = client->extensions[GL_AMD_pinned_memory])) + if (is_win64 && is_wow64() && !initialize_vk_device( teb, ctx ) && !client->extensions[GL_AMD_pinned_memory]) { if (client->major_version > 4 || (client->major_version == 4 && client->minor_version > 3)) { @@ -1951,6 +1949,7 @@ static BOOL init_buffer_storage_vulkan( struct buffer *buffer, TEB *teb, GLenum int fd, memory_type; VkResult vr; + if (!(flags & GL_MAP_PERSISTENT_BIT)) return FALSE; if ((!(vk_device = buffers.vk_device) || !vk_device->vk_device)) return FALSE; if (flags & GL_CLIENT_STORAGE_BIT) desired_type &= ~VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; @@ -2004,9 +2003,10 @@ failed: static BOOL init_buffer_storage_pinned( struct buffer *buffer, TEB *teb, GLenum target, GLuint name, size_t size, const void *data, GLbitfield flags ) { struct context *ctx = get_current_context( teb, NULL, NULL ); + struct opengl_client_context *client = opengl_client_context_from_client( ctx->base.client_context ); struct opengl_funcs *funcs = teb->glTable; - if (!ctx->use_pinned_memory) return FALSE; + if (!client->extensions[GL_AMD_pinned_memory]) return FALSE; if (!buffer_vm_alloc( teb, buffer, size )) return FALSE; if (data) memcpy( buffer->vm_ptr, data, size ); buffer->pinned = TRUE; @@ -2022,6 +2022,15 @@ static BOOL init_buffer_storage_pinned( struct buffer *buffer, TEB *teb, GLenum return TRUE; } +static BOOL init_buffer_storage( struct buffer *buffer, TEB *teb, GLenum target, GLuint name, size_t size, const void *data, GLbitfield flags ) +{ + if (!buffer_vm_alloc( teb, buffer, size )) return FALSE; + if (data) memcpy( buffer->vm_ptr, data, size ); + + TRACE( "created buffer %p with virtual memory %p\n", buffer, buffer->vm_ptr ); + return TRUE; +} + static struct buffer *create_buffer_storage( TEB *teb, GLenum target, GLuint name, GLint size, const void *data, GLbitfield flags ) { GLuint buffer_name = name ? name : get_target_name( teb, target ); @@ -2034,6 +2043,7 @@ static struct buffer *create_buffer_storage( TEB *teb, GLenum target, GLuint nam if (init_buffer_storage_vulkan( buffer, teb, target, name, size, data, flags )) return buffer; if (init_buffer_storage_pinned( buffer, teb, target, name, size, data, flags )) return buffer; + if (init_buffer_storage( buffer, teb, target, name, size, data, flags )) return buffer; if (buffer->vm_ptr) NtFreeVirtualMemory( GetCurrentProcess(), &buffer->vm_ptr, &buffer->vm_size, MEM_RELEASE ); free( buffer ); @@ -2180,9 +2190,9 @@ void wow64_glBufferStorage( TEB *teb, GLenum target, GLsizeiptr size, const void GLbitfield flags, PFN_glBufferStorage p_glBufferStorage ) { const struct opengl_funcs *funcs = teb->glTable; - struct buffer *buffer = NULL, *previous; + struct buffer *buffer, *previous; - if (flags & GL_MAP_PERSISTENT_BIT) buffer = create_buffer_storage( teb, target, 0, size, data, flags ); + buffer = create_buffer_storage( teb, target, 0, size, data, flags ); previous = set_target_buffer_storage( teb, target, buffer ); if (!buffer) p_glBufferStorage( target, size, data, flags ); if (previous) free_buffer( funcs, previous ); @@ -2192,9 +2202,9 @@ void wow64_glNamedBufferStorage( TEB *teb, GLuint name, GLsizeiptr size, const v GLbitfield flags, PFN_glNamedBufferStorage p_glNamedBufferStorage ) { const struct opengl_funcs *funcs = teb->glTable; - struct buffer *buffer = NULL, *previous; + struct buffer *buffer, *previous; - if (flags & GL_MAP_PERSISTENT_BIT) buffer = create_buffer_storage( teb, 0, name, size, data, flags ); + buffer = create_buffer_storage( teb, 0, name, size, data, flags ); previous = set_named_buffer_storage( teb, name, buffer ); if (!buffer) p_glNamedBufferStorage( name, size, data, flags ); if (previous) free_buffer( funcs, previous ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11343
From: Rémi Bernon <rbernon@codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58834 --- dlls/opengl32/unix_wgl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index c333bea7e06..e63c81a92e7 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -2374,10 +2374,12 @@ void wow64_glBufferAttachMemoryNV( TEB *teb, GLenum target, GLuint memory, GLuin void wow64_glBufferData( TEB *teb, GLenum target, GLsizeiptr size, const void *data, GLenum usage, PFN_glBufferData p_glBufferData ) { const struct opengl_funcs *funcs = teb->glTable; - struct buffer *buffer; + struct buffer *buffer, *previous; - if ((buffer = set_target_buffer_storage( teb, target, NULL ))) free_buffer( funcs, buffer ); - p_glBufferData( target, size, data, usage ); + buffer = size >= 0x1000 ? create_buffer_storage( teb, target, 0, size, data, GL_MAP_READ_BIT | GL_MAP_WRITE_BIT ) : NULL; + previous = set_target_buffer_storage( teb, target, buffer ); + if (use_driver_buffer_map( buffer )) p_glBufferData( target, size, data, usage ); + if (previous) free_buffer( funcs, previous ); } void wow64_glBufferStorageMemEXT( TEB *teb, GLenum target, GLsizeiptr size, GLuint memory, GLuint64 offset, PFN_glBufferStorageMemEXT p_glBufferStorageMemEXT ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11343
Fwiw the application does glBufferData with NULL data, maps the buffer with GL_WRITE_ONLY, unmaps it, renders, and loops over. The performance regression comes from the map, which (I think?) cannot assume the buffer data is uninitialized, and needs to copy it from GL first, even if it's requested for write only. Allowing pinned memory helps here because it usually allows the mapping to succeed in low address space. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11343#note_145145
Memory mapping functions are supposed to guarantee that all operations on the buffer are complete before returning (technically, the same is true for persistent mappings, but it's much less likely to matter there). We could perhaps change use_driver_buffer_map to return true for pinned buffers and deal with the consequences, but I'm not entirely sure. CC @dlesho who looked at this in more details. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11343#note_145290
Right, the problem here is the synchronization, under the fiction of syncronous execution, applications can set buffer data, submit work using the data in the state they left it in, and then change the data before the first commands are executed on the GPU. In my experience, GL implementations will create staging buffers on subdata/map if the buffer is still being used somewhere the pipeline. If we wanted this sort of solution, we'd have to implement buffer tracking in wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11343#note_145372
Looked into this again and yeah as Jacek mentioned, with pinned buffers this would work if we actually called glMapBuffer. (But would also come with the pitfall of forcing gpu synchronization on the backend). For the Vulkan storage path, this can't work without buffer usage tracking in Wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11343#note_148427
participants (4)
-
Derek Lesho (＠dlesho) -
Jacek Caban (＠jacek)
-
Rémi Bernon
-
Rémi Bernon (＠rbernon)