[Mesa-dev] [PATCH v2] anv: fix multiple creation with internal failure

[Eduardo Lima Mitev]

On 12/23/2016 05:39 PM, Lionel Landwerlin wrote:
> The specification section 9.4 says :
> 
>    When an application attempts to create many pipelines in a single
>    command, it is possible that some subset may fail creation. In that
>    case, the corresponding entries in the pPipelines output array will
>    be filled with VK_NULL_HANDLE values. If any pipeline fails
>    creation (for example, due to out of memory errors), the
>    vkCreate*Pipelines commands will return an error code. The
>    implementation will attempt to create all pipelines, and only
>    return VK_NULL_HANDLE values for those that actually failed.
> 
> Fixes :
> 
>    dEQP-VK.api.object_management.alloc_callback_fail_multiple.graphics_pipeline
> 
> v2: C is hard let's go shopping (Lionel)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  src/intel/vulkan/genX_pipeline.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
> index 845d0204d8..eb58f2368f 100644
> --- a/src/intel/vulkan/genX_pipeline.c
> +++ b/src/intel/vulkan/genX_pipeline.c
> @@ -1470,21 +1470,19 @@ VkResult genX(CreateGraphicsPipelines)(
>     VkResult result = VK_SUCCESS;
> 
>     unsigned i = 0;
> -   for (; i < count; i++) {
> +   for (; i < count && result == VK_SUCCESS; i++) {
>

Now that you are at it, I would move the declaration and initialization
of "i" into the for statement:

for (unsigned i = 0; i < count && result == VK_SUCCESS; i++) {

>        result = genX(graphics_pipeline_create)(_device,
>                                                pipeline_cache,
>                                                &pCreateInfos[i],
>                                                pAllocator, &pPipelines[i]);
>        if (result != VK_SUCCESS) {
> -         for (unsigned j = 0; j < i; j++) {
> -            anv_DestroyPipeline(_device, pPipelines[j], pAllocator);
> -         }
> -
> -         return result;
> +         for (; i < count; i++)
> +            pPipelines[i] = VK_NULL_HANDLE;
> +         break;
>        }
>     }
> 
> -   return VK_SUCCESS;
> +   return result;
>  }
> 

I know it was the previous behavior too, but from the spec quotation
above it is not clear that once a pipeline creation fails, no more
pipelines should be attempted creation. Actually, it explicitly says
that "The implementation will attempt to create all pipelines", which is
not the case here. I'm thinking on e.g, a momentary out-of-memory
condition. At least I would add a comment saying that it will bail out
upon the first error and why.

>  VkResult genX(CreateComputePipelines)(
> @@ -1500,18 +1498,16 @@ VkResult genX(CreateComputePipelines)(
>     VkResult result = VK_SUCCESS;
> 
>     unsigned i = 0;
> -   for (; i < count; i++) {
> +   for (; i < count && result == VK_SUCCESS; i++) {
>        result = compute_pipeline_create(_device, pipeline_cache,
>                                         &pCreateInfos[i],
>                                         pAllocator, &pPipelines[i]);
>        if (result != VK_SUCCESS) {
> -         for (unsigned j = 0; j < i; j++) {
> -            anv_DestroyPipeline(_device, pPipelines[j], pAllocator);
> -         }
> -
> -         return result;
> +         for (; i < count; i++)
> +            pPipelines[i] = VK_NULL_HANDLE;
> +         break;
>        }
>     }
> 
> -   return VK_SUCCESS;
> +   return result;
>  }

Both comments apply here too.

> --
> 2.11.0
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>