From 66fdba6e09d70ad184280b92872b8adfb2cc9eae Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Fri, 22 Aug 2025 10:51:40 +0200 Subject: [PATCH] panvk: Fix disjoint image memory binding Right now, we store the last VkDeviceMemory object bound to an image in panvk_image::mem, but this doesn't work for disjoint images where the mem object can differ on each plane. Move panvk_image::mem to panvk_image_plane::mem and prefix the offset field with mem_, so it's clear the offset refers to the mem object. Note this should only fix host copy on disjoint images, since the GPU address was already properly set at bind time. Fixes: 1cd61ee94875 ("panvk: implement VK_EXT_host_image_copy for linear color images") Signed-off-by: Boris Brezillon Reviewed-by: Olivia Lee Part-of: (cherry picked from commit d0126f5ced922ba5516f789173d482644cbf024f) --- .pick_status.json | 2 +- src/panfrost/vulkan/panvk_cmd_draw.h | 2 +- src/panfrost/vulkan/panvk_host_copy.c | 157 ++++++++++++++++-------- src/panfrost/vulkan/panvk_image.c | 20 ++- src/panfrost/vulkan/panvk_image.h | 8 +- src/panfrost/vulkan/panvk_vX_cmd_draw.c | 20 ++- 6 files changed, 135 insertions(+), 74 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 0a752e81acf..1b02cc4abb7 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -5134,7 +5134,7 @@ "description": "panvk: Fix disjoint image memory binding", "nominated": true, "nomination_type": 2, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "1cd61ee94875efbf546455212e67ccc6bac111c6", "notes": null diff --git a/src/panfrost/vulkan/panvk_cmd_draw.h b/src/panfrost/vulkan/panvk_cmd_draw.h index 2437766c2ac..807ccfe9995 100644 --- a/src/panfrost/vulkan/panvk_cmd_draw.h +++ b/src/panfrost/vulkan/panvk_cmd_draw.h @@ -72,7 +72,7 @@ struct panvk_rendering_state { #if PAN_ARCH < 9 uint32_t bo_count; - struct pan_kmod_bo *bos[MAX_RTS + 2]; + struct pan_kmod_bo *bos[(MAX_RTS * PANVK_MAX_PLANES) + 2]; #endif } fb; diff --git a/src/panfrost/vulkan/panvk_host_copy.c b/src/panfrost/vulkan/panvk_host_copy.c index 03310db8344..4ae114ec28f 100644 --- a/src/panfrost/vulkan/panvk_host_copy.c +++ b/src/panfrost/vulkan/panvk_host_copy.c @@ -115,7 +115,7 @@ panvk_copy_image_to_from_memory(struct image_params img, unsigned layer_count = vk_image_subresource_layer_count(&img.img->vk, &img.subres); - void *img_base_ptr = img.ptr + plane->offset + slice_layout->offset_B; + void *img_base_ptr = img.ptr + plane->mem_offset + slice_layout->offset_B; for (unsigned layer = 0; layer < layer_count; layer++) { unsigned img_layer = layer + img.subres.baseArrayLayer; void *img_layer_ptr = img_base_ptr + @@ -197,28 +197,82 @@ panvk_copy_memory_to_image(struct panvk_image *dst, void *dst_cpu, dst_params, src_params, region->imageExtent, flags, true); } +static VkResult +mmap_plane(struct panvk_image *img, uint8_t p, int prot, + void *plane_ptrs[static const PANVK_MAX_PLANES]) +{ + assert(p < PANVK_MAX_PLANES); + + if (plane_ptrs[p]) + return VK_SUCCESS; + + plane_ptrs[p] = pan_kmod_bo_mmap(img->planes[p].mem->bo, 0, + pan_kmod_bo_size(img->planes[p].mem->bo), + prot, MAP_SHARED, NULL); + + if (plane_ptrs[p] == MAP_FAILED) { + plane_ptrs[p] = NULL; + return panvk_errorf(img->vk.base.device, VK_ERROR_MEMORY_MAP_FAILED, + "Failed to CPU map image"); + } + + /* In case of a multi-planar and !disjoint image (or disjoint but with some + * planes pointing to the same memory object), we propagate the BO mapping to + * all relevant entries, so we don't have to mmap the same BO to different + * location if another plane is copied. */ + for (uint8_t i = 0; i < PANVK_MAX_PLANES; i++) { + if (p != i && img->planes[p].mem == img->planes[i].mem) + plane_ptrs[i] = plane_ptrs[p]; + } + + return VK_SUCCESS; +} + +static void +munmap_planes(struct panvk_image *img, + void *plane_ptrs[static const PANVK_MAX_PLANES]) +{ + for (uint8_t i = 0; i < PANVK_MAX_PLANES; i++) { + if (!plane_ptrs[i]) + continue; + + ASSERTED int ret = + os_munmap(plane_ptrs[i], pan_kmod_bo_size(img->planes[i].mem->bo)); + assert(!ret); + + /* Make sure we reset all mapping entries pointing to the same virtual + * address so we don't end up with double-munmap() cases. */ + for (uint8_t j = i; j < PANVK_MAX_PLANES; j++) { + if (plane_ptrs[i] == plane_ptrs[j]) + plane_ptrs[j] = NULL; + } + + plane_ptrs[i] = NULL; + } +} + VKAPI_ATTR VkResult VKAPI_CALL panvk_CopyMemoryToImage(VkDevice device, const VkCopyMemoryToImageInfo *info) { - VK_FROM_HANDLE(panvk_device, dev, device); VK_FROM_HANDLE(panvk_image, dst, info->dstImage); - - void *dst_cpu = pan_kmod_bo_mmap( - dst->mem->bo, 0, pan_kmod_bo_size(dst->mem->bo), PROT_WRITE, MAP_SHARED, - NULL); - if (dst_cpu == MAP_FAILED) - return panvk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to CPU map image"); + void *dst_cpu[PANVK_MAX_PLANES] = {NULL}; + VkResult result = VK_SUCCESS; for (unsigned i = 0; i < info->regionCount; i++) { - panvk_copy_memory_to_image(dst, dst_cpu, &info->pRegions[i], + uint8_t p = panvk_plane_index( + dst->vk.format, info->pRegions[i].imageSubresource.aspectMask); + + result = mmap_plane(dst, p, PROT_WRITE, dst_cpu); + if (result != VK_SUCCESS) + goto out_unmap; + + panvk_copy_memory_to_image(dst, dst_cpu[p], &info->pRegions[i], info->flags); } - ASSERTED int ret = os_munmap(dst_cpu, pan_kmod_bo_size(dst->mem->bo)); - assert(!ret); - - return VK_SUCCESS; +out_unmap: + munmap_planes(dst, dst_cpu); + return result; } static void @@ -244,25 +298,25 @@ panvk_copy_image_to_memory(struct panvk_image *src, void *src_cpu, VKAPI_ATTR VkResult VKAPI_CALL panvk_CopyImageToMemory(VkDevice device, const VkCopyImageToMemoryInfo *info) { - VK_FROM_HANDLE(panvk_device, dev, device); VK_FROM_HANDLE(panvk_image, src, info->srcImage); - - void *src_cpu = pan_kmod_bo_mmap( - src->mem->bo, 0, pan_kmod_bo_size(src->mem->bo), PROT_READ, MAP_SHARED, - NULL); - if (src_cpu == MAP_FAILED) - return panvk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to CPU map image"); + void *src_cpu[PANVK_MAX_PLANES] = {NULL}; + VkResult result = VK_SUCCESS; for (unsigned i = 0; i < info->regionCount; i++) { - panvk_copy_image_to_memory(src, src_cpu, &info->pRegions[i], + uint8_t p = panvk_plane_index( + src->vk.format, info->pRegions[i].imageSubresource.aspectMask); + + result = mmap_plane(src, p, PROT_READ, src_cpu); + if (result != VK_SUCCESS) + goto out_unmap; + + panvk_copy_image_to_memory(src, src_cpu[p], &info->pRegions[i], info->flags); } - ASSERTED int ret = os_munmap(src_cpu, pan_kmod_bo_size(src->mem->bo)); - assert(!ret); - - return VK_SUCCESS; +out_unmap: + munmap_planes(src, src_cpu); + return result; } static void @@ -337,9 +391,9 @@ panvk_copy_image_to_image(struct panvk_image *dst, void *dst_cpu, unsigned depth = sample_count > 1 ? sample_count : region->extent.depth; void *src_base_ptr = - src_cpu + src_plane->offset + src_slice_layout->offset_B; + src_cpu + src_plane->mem_offset + src_slice_layout->offset_B; void *dst_base_ptr = - dst_cpu + dst_plane->offset + dst_slice_layout->offset_B; + dst_cpu + dst_plane->mem_offset + dst_slice_layout->offset_B; for (unsigned layer = 0; layer < layer_count; layer++) { unsigned src_layer = layer + src_subres.baseArrayLayer; unsigned dst_layer = layer + dst_subres.baseArrayLayer; @@ -421,37 +475,32 @@ panvk_CopyImageToImage(VkDevice device, const VkCopyImageToImageInfo *info) { VkResult result = VK_SUCCESS; - VK_FROM_HANDLE(panvk_device, dev, device); VK_FROM_HANDLE(panvk_image, dst, info->dstImage); VK_FROM_HANDLE(panvk_image, src, info->srcImage); - - void *dst_cpu = pan_kmod_bo_mmap( - dst->mem->bo, 0, pan_kmod_bo_size(dst->mem->bo), PROT_WRITE, MAP_SHARED, - NULL); - if (dst_cpu == MAP_FAILED) - return panvk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to CPU map image"); - - void *src_cpu = pan_kmod_bo_mmap( - src->mem->bo, 0, pan_kmod_bo_size(src->mem->bo), PROT_READ, MAP_SHARED, - NULL); - if (src_cpu == MAP_FAILED) { - result = panvk_errorf(dev, VK_ERROR_MEMORY_MAP_FAILED, - "Failed to CPU map image"); - goto unmap_dst; - } + void *src_cpu[PANVK_MAX_PLANES] = {NULL}; + void *dst_cpu[PANVK_MAX_PLANES] = {NULL}; for (unsigned i = 0; i < info->regionCount; i++) { - panvk_copy_image_to_image(dst, dst_cpu, src, src_cpu, &info->pRegions[i], - info->flags); + uint8_t src_p = panvk_plane_index( + src->vk.format, info->pRegions[i].srcSubresource.aspectMask); + uint8_t dst_p = panvk_plane_index( + dst->vk.format, info->pRegions[i].dstSubresource.aspectMask); + + result = mmap_plane(dst, dst_p, PROT_WRITE, dst_cpu); + if (result != VK_SUCCESS) + goto out_unmap; + + result = mmap_plane(src, src_p, PROT_READ, src_cpu); + if (result != VK_SUCCESS) + goto out_unmap; + + panvk_copy_image_to_image(dst, dst_cpu[dst_p], src, src_cpu[src_p], + &info->pRegions[i], info->flags); } - ASSERTED int ret = os_munmap(src_cpu, pan_kmod_bo_size(src->mem->bo)); - assert(!ret); -unmap_dst: - ret = os_munmap(dst_cpu, pan_kmod_bo_size(dst->mem->bo)); - assert(!ret); - +out_unmap: + munmap_planes(src, src_cpu); + munmap_planes(dst, dst_cpu); return result; } diff --git a/src/panfrost/vulkan/panvk_image.c b/src/panfrost/vulkan/panvk_image.c index a6515afbd9a..be9cd5b2e21 100644 --- a/src/panfrost/vulkan/panvk_image.c +++ b/src/panfrost/vulkan/panvk_image.c @@ -425,15 +425,16 @@ panvk_image_init(struct panvk_image *image, static VkResult panvk_image_plane_bind(struct panvk_device *dev, - struct panvk_image_plane *plane, struct pan_kmod_bo *bo, - uint64_t base, uint64_t offset) + struct panvk_image_plane *plane, + struct panvk_device_memory *mem, uint64_t offset) { - plane->plane.base = base + offset; - plane->offset = offset; + plane->plane.base = mem->addr.dev + offset; + plane->mem = mem; + plane->mem_offset = offset; /* Reset the AFBC headers */ if (drm_is_afbc(plane->image.props.modifier)) { /* Transient CPU mapping */ - void *bo_base = pan_kmod_bo_mmap(bo, 0, pan_kmod_bo_size(bo), + void *bo_base = pan_kmod_bo_mmap(mem->bo, 0, pan_kmod_bo_size(mem->bo), PROT_WRITE, MAP_SHARED, NULL); if (bo_base == MAP_FAILED) @@ -459,7 +460,7 @@ panvk_image_plane_bind(struct panvk_device *dev, } } - ASSERTED int ret = os_munmap(bo_base, pan_kmod_bo_size(bo)); + ASSERTED int ret = os_munmap(bo_base, pan_kmod_bo_size(mem->bo)); assert(!ret); } @@ -700,19 +701,16 @@ panvk_image_bind(struct panvk_device *dev, } assert(mem); - image->mem = mem; if (is_disjoint(image)) { const VkBindImagePlaneMemoryInfo *plane_info = vk_find_struct_const(bind_info->pNext, BIND_IMAGE_PLANE_MEMORY_INFO); const uint8_t plane = panvk_plane_index(image->vk.format, plane_info->planeAspect); - return panvk_image_plane_bind(dev, &image->planes[plane], mem->bo, - mem->addr.dev, offset); + return panvk_image_plane_bind(dev, &image->planes[plane], mem, offset); } else { for (unsigned plane = 0; plane < image->plane_count; plane++) { VkResult result = - panvk_image_plane_bind(dev, &image->planes[plane], mem->bo, - mem->addr.dev, offset); + panvk_image_plane_bind(dev, &image->planes[plane], mem, offset); if (result != VK_SUCCESS) return result; } diff --git a/src/panfrost/vulkan/panvk_image.h b/src/panfrost/vulkan/panvk_image.h index b0fc5105db3..305d8f93c8c 100644 --- a/src/panfrost/vulkan/panvk_image.h +++ b/src/panfrost/vulkan/panvk_image.h @@ -22,15 +22,15 @@ struct panvk_image_plane { struct pan_image image; struct pan_image_plane plane; - /* Plane offset inside the image BO */ - uint64_t offset; + struct panvk_device_memory *mem; + + /* Plane offset inside the memory object. */ + uint64_t mem_offset; }; struct panvk_image { struct vk_image vk; - struct panvk_device_memory *mem; - uint8_t plane_count; struct panvk_image_plane planes[PANVK_MAX_PLANES]; }; diff --git a/src/panfrost/vulkan/panvk_vX_cmd_draw.c b/src/panfrost/vulkan/panvk_vX_cmd_draw.c index 6ca96dc2e16..50bd9ee4b6f 100644 --- a/src/panfrost/vulkan/panvk_vX_cmd_draw.c +++ b/src/panfrost/vulkan/panvk_vX_cmd_draw.c @@ -67,7 +67,18 @@ render_state_set_color_attachment(struct panvk_cmd_buffer *cmdbuf, state->render.color_attachments.samples[index] = img->vk.samples; #if PAN_ARCH < 9 - state->render.fb.bos[state->render.fb.bo_count++] = img->mem->bo; + for (uint8_t p = 0; p < ARRAY_SIZE(iview->pview.planes); p++) { + struct pan_image_plane_ref pref = + pan_image_view_get_plane(&iview->pview, p); + + if (!pref.image) + continue; + + assert(pref.plane_idx < ARRAY_SIZE(img->planes)); + assert(img->planes[pref.plane_idx].mem->bo != NULL); + state->render.fb.bos[state->render.fb.bo_count++] = + img->planes[pref.plane_idx].mem->bo; + } #endif fbinfo->rts[index].view = &iview->pview; @@ -108,7 +119,8 @@ render_state_set_z_attachment(struct panvk_cmd_buffer *cmdbuf, container_of(iview->vk.image, struct panvk_image, vk); #if PAN_ARCH < 9 - state->render.fb.bos[state->render.fb.bo_count++] = img->mem->bo; + /* Depth plane always comes first. */ + state->render.fb.bos[state->render.fb.bo_count++] = img->planes[0].mem->bo; #endif state->render.z_attachment.fmt = iview->vk.format; @@ -172,7 +184,9 @@ render_state_set_s_attachment(struct panvk_cmd_buffer *cmdbuf, container_of(iview->vk.image, struct panvk_image, vk); #if PAN_ARCH < 9 - state->render.fb.bos[state->render.fb.bo_count++] = img->mem->bo; + /* The stencil plane is always last. */ + state->render.fb.bos[state->render.fb.bo_count++] = + img->planes[img->plane_count - 1].mem->bo; #endif state->render.s_attachment.fmt = iview->vk.format;