Seems reasonable to me. Acked-by: Edward O'Callaghan <funfunc...@folklore1984.net>
On 10/17/2016 06:32 AM, Darren Salt wrote: > This is a port of commit a4a59172482d50318a5ae7f99021bcf0125e0f53: > > Add guards to prevent dereferencing NULL dynamic pipeline state. Asserts > of pCreateInfo members are moved to the earliest points at which they > should not be NULL. > > This fixes a segfault, related to pColorBlendState, seen in Talos Principle > which I've observed after startup is completed and when exiting the menus, > depending on when Vulkan rendering is selected. > > v2: moved the NULL check in radv_pipeline_init_blend_state to after the > declarations. > --- > src/amd/vulkan/radv_pipeline.c | 67 > +++++++++++++++++++++++++++++------------- > 1 file changed, 47 insertions(+), 20 deletions(-) > > diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c > index eb64b69..d992a10 100644 > --- a/src/amd/vulkan/radv_pipeline.c > +++ b/src/amd/vulkan/radv_pipeline.c > @@ -717,6 +717,10 @@ radv_pipeline_init_blend_state(struct radv_pipeline > *pipeline, > uint32_t blend_enable = 0, blend_need_alpha = 0; > int i; > bool single_cb_enable = false; > + > + if (!vkblend) > + return; > + > if (extra && extra->custom_blend_mode) { > single_cb_enable = true; > mode = extra->custom_blend_mode; > @@ -1069,18 +1073,27 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > > struct radv_dynamic_state *dynamic = &pipeline->dynamic_state; > > - dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount; > - if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) { > - typed_memcpy(dynamic->viewport.viewports, > - pCreateInfo->pViewportState->pViewports, > - pCreateInfo->pViewportState->viewportCount); > - } > + /* Section 9.2 of the Vulkan 1.0.15 spec says: > + * > + * pViewportState is [...] NULL if the pipeline > + * has rasterization disabled. > + */ > + if (!pCreateInfo->pRasterizationState->rasterizerDiscardEnable) { > + assert(pCreateInfo->pViewportState); > + > + dynamic->viewport.count = > pCreateInfo->pViewportState->viewportCount; > + if (states & (1 << VK_DYNAMIC_STATE_VIEWPORT)) { > + typed_memcpy(dynamic->viewport.viewports, > + pCreateInfo->pViewportState->pViewports, > + > pCreateInfo->pViewportState->viewportCount); > + } > > - dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount; > - if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) { > - typed_memcpy(dynamic->scissor.scissors, > - pCreateInfo->pViewportState->pScissors, > - pCreateInfo->pViewportState->scissorCount); > + dynamic->scissor.count = > pCreateInfo->pViewportState->scissorCount; > + if (states & (1 << VK_DYNAMIC_STATE_SCISSOR)) { > + typed_memcpy(dynamic->scissor.scissors, > + pCreateInfo->pViewportState->pScissors, > + pCreateInfo->pViewportState->scissorCount); > + } > } > > if (states & (1 << VK_DYNAMIC_STATE_LINE_WIDTH)) { > @@ -1098,7 +1111,21 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > pCreateInfo->pRasterizationState->depthBiasSlopeFactor; > } > > - if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) { > + /* Section 9.2 of the Vulkan 1.0.15 spec says: > + * > + * pColorBlendState is [...] NULL if the pipeline has rasterization > + * disabled or if the subpass of the render pass the pipeline is > + * created against does not use any color attachments. > + */ > + bool uses_color_att = false; > + for (unsigned i = 0; i < subpass->color_count; ++i) { > + if (subpass->color_attachments[i].attachment != > VK_ATTACHMENT_UNUSED) { > + uses_color_att = true; > + break; > + } > + } > + > + if (uses_color_att && states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) > { > assert(pCreateInfo->pColorBlendState); > typed_memcpy(dynamic->blend_constants, > pCreateInfo->pColorBlendState->blendConstants, 4); > @@ -1110,14 +1137,17 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > * no need to override the depthstencil defaults in > * radv_pipeline::dynamic_state when there is no depthstencil > attachment. > * > - * From the Vulkan spec (20 Oct 2015, git-aa308cb): > + * Section 9.2 of the Vulkan 1.0.15 spec says: > * > - * pDepthStencilState [...] may only be NULL if renderPass and > subpass > - * specify a subpass that has no depth/stencil attachment. > + * pDepthStencilState is [...] NULL if the pipeline has rasterization > + * disabled or if the subpass of the render pass the pipeline is > created > + * against does not use a depth/stencil attachment. > */ > - if (subpass->depth_stencil_attachment.attachment != > VK_ATTACHMENT_UNUSED) { > + if (!pCreateInfo->pRasterizationState->rasterizerDiscardEnable && > + subpass->depth_stencil_attachment.attachment != > VK_ATTACHMENT_UNUSED) { > + assert(pCreateInfo->pDepthStencilState); > + > if (states & (1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS)) { > - assert(pCreateInfo->pDepthStencilState); > dynamic->depth_bounds.min = > pCreateInfo->pDepthStencilState->minDepthBounds; > dynamic->depth_bounds.max = > @@ -1125,7 +1155,6 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > } > > if (states & (1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK)) { > - assert(pCreateInfo->pDepthStencilState); > dynamic->stencil_compare_mask.front = > > pCreateInfo->pDepthStencilState->front.compareMask; > dynamic->stencil_compare_mask.back = > @@ -1133,7 +1162,6 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > } > > if (states & (1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK)) { > - assert(pCreateInfo->pDepthStencilState); > dynamic->stencil_write_mask.front = > > pCreateInfo->pDepthStencilState->front.writeMask; > dynamic->stencil_write_mask.back = > @@ -1141,7 +1169,6 @@ radv_pipeline_init_dynamic_state(struct radv_pipeline > *pipeline, > } > > if (states & (1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE)) { > - assert(pCreateInfo->pDepthStencilState); > dynamic->stencil_reference.front = > > pCreateInfo->pDepthStencilState->front.reference; > dynamic->stencil_reference.back = >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev