pull in leegao's multi-fd AHB mapping fix - #7
Conversation
leegao
left a comment
leegao
left a comment
There was a problem hiding this comment.
LGTM, with one potential change on 1032 (restructure to iterate on the copied struct instead)
I'm very glad these got caught, the synchronization design is inverted from how you would normally design the mutex control (upstream vs downstream), so these are really easy to get wrong.
| * head of the pNext chain, the unlink only updates pCreateInfo->pNext, | ||
| * so a copy taken earlier would still pass the unknown struct to the | ||
| * driver. */ | ||
| create_info = *pCreateInfo; |
There was a problem hiding this comment.
ahh I see what it's trying to say, yeah that's a legitimate type, since VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_EMULATED_B8G8R8A8_CREATE_INFO_EXT is always at the head of the chain
fortunately, it'll just raise a validation warning (the WSI code already injects VK_STRUCTURE_TYPE_WSI_IMAGE_CREATE_INFO_MESA for the system driver), but good catch
An alternative structure is to leave the copy at the top, and change the iteration to
VkBaseInStructure *prev = &create_info;
for (const VkBaseInStructure *s = pCreateInfo->pNext; s; s = s->pNext) {
if (s->sType == VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_EMULATED_B8G8R8A8_CREATE_INFO_EXT) {
is_emulated_bgra8 = true;
prev->pNext = s->pNext; // unlink
break;
}
prev = (VkBaseInStructure *) s;
}which has the benefit of not mutating (directly) the upstream pCreateInfo, though we do still change the linked pNext link in place instead of deep-copying everything out so it probably doesn't really matter (we also know who the caller is when this sType extension is sent in, because it's an internal callsite)
| // not being able to mmap these. | ||
| WRAPPER_LOG(error, "EXT_map_memory_placed emulation failed for swapchain image, bypassing emulation"); | ||
| simple_mtx_lock(&device->resource_mutex); | ||
| simple_mtx_unlock(&device->resource_mutex); |
There was a problem hiding this comment.
oh yeah, that's a legitimate copy-paste type, good catch
|
|
||
| if (dedicated_allocate_info && dedicated_allocate_info->image != VK_NULL_HANDLE) { | ||
| struct wrapper_image *img = get_wrapper_image_from_handle(device, dedicated_allocate_info->image); | ||
| /* resource_mutex is already held here and simple_mtx is not |
There was a problem hiding this comment.
oh yeah this is a great catch, it's also a good reason for why getters should never be locked by an exclusive mutex since only upstream controls (and should orchestrate) synchronizaton
You should also change this comment to a TODO to refactor the get-handles to avoid this specific footgun in the future
| if (mem->alloc_size > 0) { | ||
| mem->map_size = mem->alloc_size; | ||
| } else { | ||
| off_t res = lseek(fd, 0, SEEK_END); |
There was a problem hiding this comment.
And a potential future TODO here to fortify this in the future is to just plumb the size from AllocateMemory down to here so we don't always have to guess, but that's for later
2 participants
No description provided.