From 1b4a9c7d450739350516b91c9ff913932ed6a9e9 Mon Sep 17 00:00:00 2001 From: Alejandro Piñeiro Date: Wed, 29 Apr 2020 15:58:10 +0200 Subject: v3dv: properly return OOM error during pipeline creation So far we were just asserting or aborting if any of the internal method used during the pipeline creation failed. We needed to change the return value of several methods, in order to bubble up the proper memory allocation error. Note that as the pipeline creation is complex and splitted in several methods, if an error happens in the middle, it returns back, and rely on the higher level to call PipelineDestroy. This method needs to take into account that some of the resources could have not been allocated when freeing it. Also note that v3dv_get_shader_variant is used during the pipeline bind, as with the new resources bound, we need to check if we need to recompile a new variant. At that moment we are not creating a new vulkan object so we can really return a OOM error. For now we just assert on that case. Part-of: --- src/broadcom/vulkan/v3dv_cmd_buffer.c | 28 +++++++++-- src/broadcom/vulkan/v3dv_pipeline.c | 91 +++++++++++++++++++++++++++-------- src/broadcom/vulkan/v3dv_private.h | 4 +- 3 files changed, 99 insertions(+), 24 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c index afc2b1832ff..821aff2af09 100644 --- a/src/broadcom/vulkan/v3dv_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c @@ -2118,8 +2118,15 @@ update_fs_variant(struct v3dv_cmd_buffer *cmd_buffer) memcpy(&local_key, &p_stage->key.fs, sizeof(struct v3d_fs_key)); if (cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer)) { + VkResult vk_result; variant = v3dv_get_shader_variant(p_stage, &local_key.base, - sizeof(struct v3d_fs_key)); + sizeof(struct v3d_fs_key), + &cmd_buffer->device->alloc, + &vk_result); + /* At this point we are not creating a vulkan object to return to the + * API user, so we can't really return back a OOM error + */ + assert(variant); if (p_stage->current_variant != variant) { p_stage->current_variant = variant; @@ -2142,8 +2149,15 @@ update_vs_variant(struct v3dv_cmd_buffer *cmd_buffer) memcpy(&local_key, &p_stage->key.vs, sizeof(struct v3d_vs_key)); if (cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer)) { + VkResult vk_result; variant = v3dv_get_shader_variant(p_stage, &local_key.base, - sizeof(struct v3d_vs_key)); + sizeof(struct v3d_vs_key), + &cmd_buffer->device->alloc, + &vk_result); + /* At this point we are not creating a vulkan object to return to the + * API user, so we can't really return back a OOM error + */ + assert(variant); if (p_stage->current_variant != variant) { p_stage->current_variant = variant; @@ -2156,8 +2170,16 @@ update_vs_variant(struct v3dv_cmd_buffer *cmd_buffer) memcpy(&local_key, &p_stage->key.vs, sizeof(struct v3d_vs_key)); if (cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer)) { + VkResult vk_result; variant = v3dv_get_shader_variant(p_stage, &local_key.base, - sizeof(struct v3d_vs_key)); + sizeof(struct v3d_vs_key), + &cmd_buffer->device->alloc, + &vk_result); + + /* At this point we are not creating a vulkan object to return to the + * API user, so we can't really return back a OOM error + */ + assert(variant); if (p_stage->current_variant != variant) { p_stage->current_variant = variant; diff --git a/src/broadcom/vulkan/v3dv_pipeline.c b/src/broadcom/vulkan/v3dv_pipeline.c index f2ad55033ae..facc3da7f74 100644 --- a/src/broadcom/vulkan/v3dv_pipeline.c +++ b/src/broadcom/vulkan/v3dv_pipeline.c @@ -111,14 +111,11 @@ destroy_pipeline_stage(struct v3dv_device *device, vk_free2(&device->alloc, pAllocator, p_stage); } -void -v3dv_DestroyPipeline(VkDevice _device, - VkPipeline _pipeline, - const VkAllocationCallbacks *pAllocator) +static void +v3dv_destroy_pipeline(struct v3dv_pipeline *pipeline, + struct v3dv_device *device, + const VkAllocationCallbacks *pAllocator) { - V3DV_FROM_HANDLE(v3dv_device, device, _device); - V3DV_FROM_HANDLE(v3dv_pipeline, pipeline, _pipeline); - if (!pipeline) return; @@ -143,6 +140,20 @@ v3dv_DestroyPipeline(VkDevice _device, vk_free2(&device->alloc, pAllocator, pipeline); } +void +v3dv_DestroyPipeline(VkDevice _device, + VkPipeline _pipeline, + const VkAllocationCallbacks *pAllocator) +{ + V3DV_FROM_HANDLE(v3dv_device, device, _device); + V3DV_FROM_HANDLE(v3dv_pipeline, pipeline, _pipeline); + + if (!pipeline) + return; + + v3dv_destroy_pipeline(pipeline, device, pAllocator); +} + static const struct spirv_to_nir_options default_spirv_options = { .caps = { false }, .ubo_addr_format = nir_address_format_32bit_index_offset, @@ -1077,8 +1088,10 @@ pipeline_stage_create_vs_bin(const struct v3dv_pipeline_stage *src, * 4096, so that would allow to use less memory. * * For now one-bo per-assembly would work. + * + * Returns false if it was not able to allocate or map the assembly bo memory. */ -static void +static bool upload_assembly(struct v3dv_pipeline_stage *p_stage, struct v3dv_shader_variant *variant, const void *data, @@ -1107,13 +1120,13 @@ upload_assembly(struct v3dv_pipeline_stage *p_stage, struct v3dv_bo *bo = v3dv_bo_alloc(device, size, name); if (!bo) { fprintf(stderr, "failed to allocate memory for shader\n"); - abort(); + return false; } bool ok = v3dv_bo_map(device, bo, size); if (!ok) { fprintf(stderr, "failed to map source shader buffer\n"); - abort(); + return false; } memcpy(bo->map, data, size); @@ -1121,28 +1134,42 @@ upload_assembly(struct v3dv_pipeline_stage *p_stage, v3dv_bo_unmap(device, bo); variant->assembly_bo = bo; + + return true; } /* For a given key, it returns the compiled version of the shader. If it was * already compiled, it gets it from the p_stage cache, if not it compiles is * through the v3d compiler + * + * If the method returns NULL it means that it was not able to allocate the + * resources for the variant. out_vk_result would return which OOM applies. */ struct v3dv_shader_variant* v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage, struct v3d_key *key, - size_t key_size) + size_t key_size, + const VkAllocationCallbacks *pAllocator, + VkResult *out_vk_result) { struct hash_table *ht = p_stage->cache; struct hash_entry *entry = _mesa_hash_table_search(ht, key); - if (entry) + if (entry) { + *out_vk_result = VK_SUCCESS; return entry->data; + } struct v3dv_device *device = p_stage->pipeline->device; struct v3dv_shader_variant *variant = vk_zalloc(&device->alloc, sizeof(*variant), 8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); + if (variant == NULL) { + *out_vk_result = VK_ERROR_OUT_OF_HOST_MEMORY; + return NULL; + } + struct v3dv_physical_device *physical_device = &p_stage->pipeline->device->instance->physicalDevice; const struct v3d_compiler *compiler = physical_device->compiler; @@ -1175,7 +1202,13 @@ v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage, gl_shader_stage_name(p_stage->stage), p_stage->program_id); } else { - upload_assembly(p_stage, variant, qpu_insts, qpu_insts_size); + if (!upload_assembly(p_stage, variant, qpu_insts, qpu_insts_size)) { + free(qpu_insts); + vk_free2(&device->alloc, pAllocator, variant); + + *out_vk_result = VK_ERROR_OUT_OF_DEVICE_MEMORY; + return NULL; + } } free(qpu_insts); @@ -1190,6 +1223,7 @@ v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage, /* FIXME: pending provide scratch space for register spilling */ assert(variant->prog_data.base->spill_size == 0); + *out_vk_result = VK_SUCCESS; return variant; } @@ -1433,13 +1467,21 @@ pipeline_compile_graphics(struct v3dv_pipeline *pipeline, */ struct v3d_vs_key *key = &pipeline->vs->key.vs; pipeline_populate_v3d_vs_key(key, pCreateInfo, pipeline->vs); + VkResult vk_result; pipeline->vs->current_variant = - v3dv_get_shader_variant(pipeline->vs, &key->base, sizeof(*key)); + v3dv_get_shader_variant(pipeline->vs, &key->base, sizeof(*key), + pAllocator, &vk_result); + if (vk_result != VK_SUCCESS) + return vk_result; key = &pipeline->vs_bin->key.vs; pipeline_populate_v3d_vs_key(key, pCreateInfo, pipeline->vs_bin); pipeline->vs_bin->current_variant = - v3dv_get_shader_variant(pipeline->vs_bin, &key->base, sizeof(*key)); + v3dv_get_shader_variant(pipeline->vs_bin, &key->base, sizeof(*key), + pAllocator, &vk_result); + if (vk_result != VK_SUCCESS) + return vk_result; + break; } case MESA_SHADER_FRAGMENT: { @@ -1451,8 +1493,12 @@ pipeline_compile_graphics(struct v3dv_pipeline *pipeline, lower_fs_io(p_stage->nir); + VkResult vk_result; p_stage->current_variant = - v3dv_get_shader_variant(p_stage, &key->base, sizeof(*key)); + v3dv_get_shader_variant(p_stage, &key->base, sizeof(*key), + pAllocator, &vk_result); + if (vk_result != VK_SUCCESS) + return vk_result; break; } @@ -2110,7 +2156,7 @@ get_attr_type(const struct util_format_description *desc) return attr_type; } -static void +static bool create_default_attribute_values(struct v3dv_pipeline *pipeline, const VkPipelineVertexInputStateCreateInfo *vi_info) { @@ -2123,6 +2169,7 @@ create_default_attribute_values(struct v3dv_pipeline *pipeline, if (!pipeline->default_attribute_values) { fprintf(stderr, "failed to allocate memory for the default " "attribute values\n"); + return false; } } @@ -2130,7 +2177,7 @@ create_default_attribute_values(struct v3dv_pipeline *pipeline, pipeline->default_attribute_values, size); if (!ok) { fprintf(stderr, "failed to map default attribute values buffer\n"); - abort(); + return false; } uint32_t *attrs = pipeline->default_attribute_values->map; @@ -2147,6 +2194,8 @@ create_default_attribute_values(struct v3dv_pipeline *pipeline, } v3dv_bo_unmap(pipeline->device, pipeline->default_attribute_values); + + return true; } static void @@ -2269,7 +2318,9 @@ pipeline_init(struct v3dv_pipeline *pipeline, pipeline->va_count++; } } - create_default_attribute_values(pipeline, vi_info); + + if (!create_default_attribute_values(pipeline, vi_info)) + return VK_ERROR_OUT_OF_DEVICE_MEMORY; return result; } @@ -2296,7 +2347,7 @@ graphics_pipeline_create(VkDevice _device, pAllocator); if (result != VK_SUCCESS) { - vk_free2(&device->alloc, pAllocator, pipeline); + v3dv_destroy_pipeline(pipeline, device, pAllocator); return result; } diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h index f413ea9c0b9..2bd7041cf9d 100644 --- a/src/broadcom/vulkan/v3dv_private.h +++ b/src/broadcom/vulkan/v3dv_private.h @@ -1412,7 +1412,9 @@ struct v3dv_cl_reloc v3dv_write_uniforms(struct v3dv_cmd_buffer *cmd_buffer, struct v3dv_shader_variant * v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage, struct v3d_key *key, - size_t key_size); + size_t key_size, + const VkAllocationCallbacks *pAllocator, + VkResult *out_vk_result); struct v3dv_descriptor * v3dv_descriptor_map_get_descriptor(struct v3dv_descriptor_state *descriptor_state, -- cgit v1.2.3