On Fri, Jun 10, 2016 at 06:19:12PM -0700, Jason Ekstrand wrote:
> 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?
>
>
Yes, I did. Thanks for catching this mistake.
> > + 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.
>
>
The line,
pipeline->dynamic_state = default_dynamic_state;
near the top of the function already initializes these variables to zero,
so an else would be redundant in this case.
> >
> > 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.
>
>
Sure, I'll go back to the original loop from the v1.
> > + 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.
>
I forgot to run this v2 through Jenkins. I'll do so on the v3 before I send
it out.
- Nanley
> --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
