On Fri, Jun 10, 2016 at 6:03 PM, Nanley Chery <nanleych...@gmail.com> wrote:
> From: Nanley Chery <nanley.g.ch...@intel.com> > > 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 seen in the McNopper demo, VKTS_Example09. > > v2: Fix disabled rasterization check (Jason Ekstrand) > > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com> > Cc: "12.0" <mesa-sta...@lists.freedesktop.org> > --- > > v2: Also guard scissor state > Move pColorBlend assert to earliest point known to be non-NULL > Update commit message to mention changes with asserts > > src/intel/vulkan/anv_pipeline.c | 65 > +++++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 22 deletions(-) > > diff --git a/src/intel/vulkan/anv_pipeline.c > b/src/intel/vulkan/anv_pipeline.c > index ae03787..b15296a 100644 > --- a/src/intel/vulkan/anv_pipeline.c > +++ b/src/intel/vulkan/anv_pipeline.c > @@ -979,18 +979,27 @@ copy_non_dynamic_state(struct anv_pipeline *pipeline, > > struct anv_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) { > I think you have the condition backwards. Did you mean !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); > + } > } > We may want a sensible else case. Probably best to just set scissor and viewport count to zero. > > if (states & (1 << VK_DYNAMIC_STATE_LINE_WIDTH)) { > @@ -1008,10 +1017,22 @@ copy_non_dynamic_state(struct anv_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. > + */ > + unsigned num_color_att_unused = 0; > + while (num_color_att_unused < subpass->color_count && > + subpass->color_attachments[num_color_att_unused++] == > VK_ATTACHMENT_UNUSED); > While the while loop here certainly is clever, it took me a while to figure out what it's doing. How about something like bool writes_color = false; for (unsigned i = 0; i < subpass->color_count; i++) { if (subpass->color_attachments[i] != VK_ATTACHMENT_UNUSED) { writes_color = true; break; } } That's a good deal more obvious. > + if (num_color_att_unused < subpass->color_count && > + pCreateInfo->pRasterizationState->rasterizerDiscardEnable) { > assert(pCreateInfo->pColorBlendState); > - typed_memcpy(dynamic->blend_constants, > - pCreateInfo->pColorBlendState->blendConstants, 4); > + > + if (states & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) > + typed_memcpy(dynamic->blend_constants, > + pCreateInfo->pColorBlendState->blendConstants, 4); > } > > /* If there is no depthstencil attachment, then don't read > @@ -1020,14 +1041,17 @@ copy_non_dynamic_state(struct anv_pipeline > *pipeline, > * no need to override the depthstencil defaults in > * anv_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 != VK_ATTACHMENT_UNUSED) { > + if (pCreateInfo->pRasterizationState->rasterizerDiscardEnable && > Again, !rasterizerDiscardEnable Other than the above comments, it seems reasonable. Did you run it through Jenkins? I'm kind-of surprised that the CTS didn't catch the flipped rasterizerDiscardEnable flag. It's possible that it only ever uses dynamic states but that would be silly. --Jason > + subpass->depth_stencil_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 = > @@ -1035,7 +1059,6 @@ copy_non_dynamic_state(struct anv_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 = > @@ -1043,7 +1066,6 @@ copy_non_dynamic_state(struct anv_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 = > @@ -1051,7 +1073,6 @@ copy_non_dynamic_state(struct anv_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 = > -- > 2.8.3 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev