From c05104c71fccf64a9e6d49725f0a2bf3da16a0a1 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Tue, 12 Dec 2023 19:00:50 +0100 Subject: [PATCH] panvk: Keep a ref to a pan_kmod_bo in panvk_buffer We don't need the panfrost_bo object which contains both the BO and its CPU/GPU mappings. We store the GPU address at bind time. We also have a hack for index buffers which are currently walked by the CPU to extract the min/max index. Add a comment to make sure this field goes away when indirect draw is fixed. While at it, keep a ref to the buffer object so we don't end up with a invalid deref (UAF) if the vulkan user does something silly like freeing the VkDeviceMemory object while the VkBuffer is still active. Flag this with a TODO to make sure we don't forget about it. Signed-off-by: Boris Brezillon Reviewed-by: Constantine Shablya Reviewed-by: Erik Faye-Lund Part-of: --- src/panfrost/vulkan/panvk_device.c | 12 ++++++++++-- src/panfrost/vulkan/panvk_private.h | 19 ++++++++++++++++--- src/panfrost/vulkan/panvk_vX_cmd_buffer.c | 5 ++--- src/panfrost/vulkan/panvk_vX_meta_copy.c | 12 ++++++------ 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/panfrost/vulkan/panvk_device.c b/src/panfrost/vulkan/panvk_device.c index 7c7ac4ccdc4..e4db78a397c 100644 --- a/src/panfrost/vulkan/panvk_device.c +++ b/src/panfrost/vulkan/panvk_device.c @@ -1304,13 +1304,20 @@ panvk_BindBufferMemory2(VkDevice device, uint32_t bindInfoCount, for (uint32_t i = 0; i < bindInfoCount; ++i) { VK_FROM_HANDLE(panvk_device_memory, mem, pBindInfos[i].memory); VK_FROM_HANDLE(panvk_buffer, buffer, pBindInfos[i].buffer); + struct pan_kmod_bo *old_bo = buffer->bo; if (mem) { - buffer->bo = mem->bo; - buffer->bo_offset = pBindInfos[i].memoryOffset; + buffer->bo = pan_kmod_bo_get(mem->bo->kmod_bo); + buffer->dev_addr = mem->bo->ptr.gpu + pBindInfos[i].memoryOffset; + if (buffer->vk.usage & VK_BUFFER_USAGE_INDEX_BUFFER_BIT) + buffer->host_ptr = mem->bo->ptr.cpu + pBindInfos[i].memoryOffset; } else { buffer->bo = NULL; + buffer->dev_addr = 0; + buffer->host_ptr = NULL; } + + pan_kmod_bo_put(old_bo); } return VK_SUCCESS; } @@ -1490,6 +1497,7 @@ panvk_DestroyBuffer(VkDevice _device, VkBuffer _buffer, if (!buffer) return; + pan_kmod_bo_put(buffer->bo); vk_buffer_destroy(&device->vk, pAllocator, &buffer->vk); } diff --git a/src/panfrost/vulkan/panvk_private.h b/src/panfrost/vulkan/panvk_private.h index 82560b2e8a4..0aeff5bd5a4 100644 --- a/src/panfrost/vulkan/panvk_private.h +++ b/src/panfrost/vulkan/panvk_private.h @@ -526,8 +526,21 @@ struct panvk_descriptor_pool { struct panvk_buffer { struct vk_buffer vk; - struct panfrost_bo *bo; - VkDeviceSize bo_offset; + mali_ptr dev_addr; + + /* TODO: See if we can rework the synchronization logic so we don't need to + * pass BOs around. + */ + struct pan_kmod_bo *bo; + + /* FIXME: Only used for index buffers to do the min/max index retrieval on + * the CPU side. This is all broken anyway and the min/max search should be + * done with a compute shader that also patches the job descriptor + * accordingly (basically an indirect draw). + * + * Make sure this field goes away as soon as we fixed indirect draws. + */ + void *host_ptr; }; static inline mali_ptr @@ -536,7 +549,7 @@ panvk_buffer_gpu_ptr(const struct panvk_buffer *buffer, uint64_t offset) if (buffer->bo == NULL) return 0; - return buffer->bo->ptr.gpu + buffer->bo_offset + offset; + return buffer->dev_addr + offset; } static inline uint64_t diff --git a/src/panfrost/vulkan/panvk_vX_cmd_buffer.c b/src/panfrost/vulkan/panvk_vX_cmd_buffer.c index 92c9a215e2b..4fb0756c57d 100644 --- a/src/panfrost/vulkan/panvk_vX_cmd_buffer.c +++ b/src/panfrost/vulkan/panvk_vX_cmd_buffer.c @@ -811,12 +811,11 @@ panvk_index_minmax_search(struct panvk_cmd_buffer *cmdbuf, uint32_t start, uint32_t count, bool restart, uint32_t *min, uint32_t *max) { - void *ptr = cmdbuf->state.ib.buffer->bo->ptr.cpu + - cmdbuf->state.ib.buffer->bo_offset + cmdbuf->state.ib.offset; + void *ptr = cmdbuf->state.ib.buffer->host_ptr + cmdbuf->state.ib.offset; assert(cmdbuf->state.ib.buffer); assert(cmdbuf->state.ib.buffer->bo); - assert(cmdbuf->state.ib.buffer->bo->ptr.cpu); + assert(cmdbuf->state.ib.buffer->host_ptr); uint32_t debug_flags = cmdbuf->device->physical_device->instance->debug_flags; diff --git a/src/panfrost/vulkan/panvk_vX_meta_copy.c b/src/panfrost/vulkan/panvk_vX_meta_copy.c index 59bc1a71b13..47c390095be 100644 --- a/src/panfrost/vulkan/panvk_vX_meta_copy.c +++ b/src/panfrost/vulkan/panvk_vX_meta_copy.c @@ -1106,7 +1106,7 @@ panvk_meta_copy_buf2img(struct panvk_cmd_buffer *cmdbuf, struct panvk_batch *batch = panvk_cmd_open_batch(cmdbuf); view.first_layer = view.last_layer = l + first_layer; - batch->blit.src = buf->bo->kmod_bo; + batch->blit.src = buf->bo; batch->blit.dst = img->bo; panvk_per_arch(cmd_alloc_tls_desc)(cmdbuf, true); panvk_per_arch(cmd_alloc_fb_desc)(cmdbuf); @@ -1523,7 +1523,7 @@ panvk_meta_copy_img2buf(struct panvk_cmd_buffer *cmdbuf, struct pan_tls_info tlsinfo = {0}; batch->blit.src = img->bo; - batch->blit.dst = buf->bo->kmod_bo; + batch->blit.dst = buf->bo; batch->tls = pan_pool_alloc_desc(&cmdbuf->desc_pool.base, LOCAL_STORAGE); GENX(pan_emit_tls)(&tlsinfo, batch->tls.cpu); @@ -1718,8 +1718,8 @@ panvk_meta_copy_buf2buf(struct panvk_cmd_buffer *cmdbuf, util_dynarray_append(&batch->jobs, void *, job.cpu); - batch->blit.src = src->bo->kmod_bo; - batch->blit.dst = dst->bo->kmod_bo; + batch->blit.src = src->bo; + batch->blit.dst = dst->bo; panvk_per_arch(cmd_close_batch)(cmdbuf); } @@ -1863,7 +1863,7 @@ panvk_meta_fill_buf(struct panvk_cmd_buffer *cmdbuf, util_dynarray_append(&batch->jobs, void *, job.cpu); - batch->blit.dst = dst->bo->kmod_bo; + batch->blit.dst = dst->bo; panvk_per_arch(cmd_close_batch)(cmdbuf); } @@ -1913,7 +1913,7 @@ panvk_meta_update_buf(struct panvk_cmd_buffer *cmdbuf, util_dynarray_append(&batch->jobs, void *, job.cpu); - batch->blit.dst = dst->bo->kmod_bo; + batch->blit.dst = dst->bo; panvk_per_arch(cmd_close_batch)(cmdbuf); }