pvr: Fix segfaults when pDepthStencilAttachment is NULL

depth_stencil_attachment has been changed from a pointer to the attachment idx
to just the attachment idx, as this avoids the driver having to check for NULL
when comparing attachments indexes with depth_stencil_attachment.

Anyplace that relies on depth_stencil_attachment being a valid index must
already check that depth_stencil_attachment is not VK_ATTACHMENT_UNUSED, so
this change avoids having to check both the pointer and the index for the same
information.

Noticed when running dEQP-VK.api.smoke.triangle

Signed-off-by: Jarred Davies <jarred.davies@imgtec.com>
Reviewed-by: Frank Binns <frank.binns@imgtec.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21690>

diff --git a/src/imagination/vulkan/pvr_cmd_buffer.c b/src/imagination/vulkan/pvr_cmd_buffer.c
index 8c2bee2..4282a6d 100644 (file)
--- a/src/imagination/vulkan/pvr_cmd_buffer.c
+++ b/src/imagination/vulkan/pvr_cmd_buffer.c
@@ -764,11 +764,11 @@ pvr_load_op_constants_create_and_upload(struct pvr_cmd_buffer *cmd_buffer,
       const struct pvr_render_pass_attachment *attachment;
       const struct pvr_image_view *image_view;
 
-      assert(*load_op->subpass->depth_stencil_attachment !=
+      assert(load_op->subpass->depth_stencil_attachment !=
              VK_ATTACHMENT_UNUSED);
       assert(!load_op->is_hw_object);
       attachment =
-         &pass->attachments[*load_op->subpass->depth_stencil_attachment];
+         &pass->attachments[load_op->subpass->depth_stencil_attachment];
 
       image_view = render_pass_info->attachments[attachment->index];
 
@@ -783,10 +783,10 @@ pvr_load_op_constants_create_and_upload(struct pvr_cmd_buffer *cmd_buffer,
       const struct pvr_render_pass_attachment *attachment;
       VkClearValue clear_value;
 
-      assert(*load_op->subpass->depth_stencil_attachment !=
+      assert(load_op->subpass->depth_stencil_attachment !=
              VK_ATTACHMENT_UNUSED);
       attachment =
-         &pass->attachments[*load_op->subpass->depth_stencil_attachment];
+         &pass->attachments[load_op->subpass->depth_stencil_attachment];
 
       clear_value = render_pass_info->clear_values[attachment->index];
 
@@ -4364,11 +4364,11 @@ pvr_setup_isp_faces_and_control(struct pvr_cmd_buffer *const cmd_buffer,
 
    const bool rasterizer_discard = dynamic_state->rs.rasterizer_discard_enable;
    const uint32_t subpass_idx = pass_info->subpass_idx;
-   const uint32_t *depth_stencil_attachment_idx =
+   const uint32_t depth_stencil_attachment_idx =
       pass_info->pass->subpasses[subpass_idx].depth_stencil_attachment;
    const struct pvr_image_view *const attachment =
-      depth_stencil_attachment_idx
-         ? pass_info->attachments[*depth_stencil_attachment_idx]
+      depth_stencil_attachment_idx != VK_ATTACHMENT_UNUSED
+         ? pass_info->attachments[depth_stencil_attachment_idx]
          : NULL;
 
    const enum PVRX(TA_OBJTYPE)
@@ -7032,12 +7032,15 @@ pvr_stencil_has_self_dependency(const struct pvr_cmd_buffer_state *const state)
       pvr_get_current_subpass(state);
    const uint32_t *const input_attachments = current_subpass->input_attachments;
 
+   if (current_subpass->depth_stencil_attachment == VK_ATTACHMENT_UNUSED)
+      return false;
+
    /* We only need to check the current software subpass as we don't support
     * merging to/from a subpass with self-dep stencil.
     */
 
    for (uint32_t i = 0; i < current_subpass->input_count; i++) {
-      if (input_attachments[i] == *current_subpass->depth_stencil_attachment)
+      if (input_attachments[i] == current_subpass->depth_stencil_attachment)
          return true;
    }
 

diff --git a/src/imagination/vulkan/pvr_hw_pass.c b/src/imagination/vulkan/pvr_hw_pass.c
index ee6dc66..dccb9d5 100644 (file)
--- a/src/imagination/vulkan/pvr_hw_pass.c
+++ b/src/imagination/vulkan/pvr_hw_pass.c
@@ -541,12 +541,12 @@ pvr_subpass_setup_render_init(struct pvr_renderpass_context *ctx)
            hw_subpass->stencil_clear)) {
          struct pvr_render_int_attachment *int_ds_attach;
 
-         assert(*input_subpass->depth_stencil_attachment !=
+         assert(input_subpass->depth_stencil_attachment !=
                 VK_ATTACHMENT_UNUSED);
-         assert(*input_subpass->depth_stencil_attachment <
+         assert(input_subpass->depth_stencil_attachment <
                 ctx->pass->attachment_count);
          int_ds_attach =
-            &ctx->int_attach[*input_subpass->depth_stencil_attachment];
+            &ctx->int_attach[input_subpass->depth_stencil_attachment];
 
          assert(hw_render->ds_attach_idx == VK_ATTACHMENT_UNUSED ||
                 hw_render->ds_attach_idx == int_ds_attach->attachment->index);
@@ -561,7 +561,7 @@ pvr_subpass_setup_render_init(struct pvr_renderpass_context *ctx)
          }
       }
 
-      if (*input_subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED)
+      if (input_subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED)
          first_ds = false;
 
       for (uint32_t j = 0U; j < input_subpass->color_count; j++) {
@@ -937,11 +937,11 @@ pvr_copy_z_replicate_details(struct pvr_renderpass_context *ctx,
    uint32_t z_replicate;
    bool found = false;
 
-   assert(*input_subpass->depth_stencil_attachment >= 0U &&
-          *input_subpass->depth_stencil_attachment <
+   assert(input_subpass->depth_stencil_attachment >= 0U &&
+          input_subpass->depth_stencil_attachment <
              (int32_t)ctx->pass->attachment_count);
 
-   int_ds_attach = &ctx->int_attach[*input_subpass->depth_stencil_attachment];
+   int_ds_attach = &ctx->int_attach[input_subpass->depth_stencil_attachment];
 
    assert(hw_subpass->z_replicate == -1);
 
@@ -1475,7 +1475,7 @@ pvr_enable_z_replicate(struct pvr_renderpass_context *ctx,
       struct pvr_renderpass_subpass *subpass = &ctx->subpasses[i];
       struct pvr_render_subpass *input_subpass = subpass->input_subpass;
 
-      if (*input_subpass->depth_stencil_attachment == replicate_attach_idx) {
+      if (input_subpass->depth_stencil_attachment == replicate_attach_idx) {
          first_use = i;
          break;
       }
@@ -1488,8 +1488,7 @@ pvr_enable_z_replicate(struct pvr_renderpass_context *ctx,
       struct pvr_render_subpass *input_subpass = subpass->input_subpass;
 
       /* If the subpass writes to the attachment then enable z replication. */
-      if (input_subpass->depth_stencil_attachment &&
-          *input_subpass->depth_stencil_attachment == replicate_attach_idx &&
+      if (input_subpass->depth_stencil_attachment == replicate_attach_idx &&
           !subpass->z_replicate) {
          subpass->z_replicate = true;
 
@@ -1693,7 +1692,7 @@ pvr_is_z_replicate_space_available(const struct pvr_device_info *dev_info,
          struct pvr_renderpass_subpass *subpass = &ctx->subpasses[i];
          struct pvr_render_subpass *input_subpass = subpass->input_subpass;
 
-         if (*input_subpass->depth_stencil_attachment == (int32_t)attach_idx) {
+         if (input_subpass->depth_stencil_attachment == (int32_t)attach_idx) {
             first_use = i;
             break;
          }
@@ -1820,12 +1819,12 @@ pvr_is_subpass_space_available(const struct pvr_device_info *dev_info,
    }
 
    if (sp_depth->incoming_ds_is_input) {
-      if (sp_depth->existing_ds_attach != *subpass->depth_stencil_attachment) {
+      if (sp_depth->existing_ds_attach != subpass->depth_stencil_attachment) {
          result = pvr_is_z_replicate_space_available(
             dev_info,
             ctx,
             alloc,
-            *subpass->depth_stencil_attachment,
+            subpass->depth_stencil_attachment,
             &sp_dsts->incoming_zrep);
          if (result != VK_SUCCESS)
             goto err_free_alloc;
@@ -1970,12 +1969,10 @@ pvr_merge_subpass(const struct pvr_device *device,
    bool ret;
 
    /* Depth attachment for the incoming subpass. */
-   if (*input_subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {
-      int_ds_attach =
-         &ctx->int_attach[*input_subpass->depth_stencil_attachment];
-   } else {
+   if (input_subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED)
+      int_ds_attach = &ctx->int_attach[input_subpass->depth_stencil_attachment];
+   else
       int_ds_attach = NULL;
-   }
 
    /* Attachment ID for the existing depth attachment. */
    if (ctx->int_ds_attach)
@@ -1986,7 +1983,7 @@ pvr_merge_subpass(const struct pvr_device *device,
    /* Is the incoming depth attachment used as an input to the incoming subpass?
     */
    sp_depth.incoming_ds_is_input =
-      pvr_is_input(input_subpass, *input_subpass->depth_stencil_attachment);
+      pvr_is_input(input_subpass, input_subpass->depth_stencil_attachment);
 
    /* Is the current depth attachment used as an input to the incoming subpass?
     */
@@ -2216,12 +2213,12 @@ pvr_merge_subpass(const struct pvr_device *device,
    }
 
    if (sp_depth.incoming_ds_is_input) {
-      if (*input_subpass->depth_stencil_attachment !=
+      if (input_subpass->depth_stencil_attachment !=
           sp_depth.existing_ds_attach) {
          result =
             pvr_enable_z_replicate(ctx,
                                    hw_render,
-                                   *input_subpass->depth_stencil_attachment,
+                                   input_subpass->depth_stencil_attachment,
                                    &sp_dsts.incoming_zrep);
          if (result != VK_SUCCESS)
             goto end_merge_subpass;
@@ -2324,9 +2321,9 @@ static VkResult pvr_schedule_subpass(const struct pvr_device *device,
                                 subpass_num,
                                 subpass->input_attachments,
                                 subpass->input_count);
-   if (*subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {
+   if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {
       struct pvr_render_int_attachment *int_depth_attach =
-         &ctx->int_attach[*subpass->depth_stencil_attachment];
+         &ctx->int_attach[subpass->depth_stencil_attachment];
 
       assert(int_depth_attach->remaining_count > 0U);
       int_depth_attach->remaining_count--;
@@ -2573,7 +2570,7 @@ VkResult pvr_create_renderpass_hwsetup(
          int_attach->remaining_count +=
             color_output_uses + input_attachment_uses;
 
-         if ((uint32_t)*subpass->depth_stencil_attachment == i)
+         if ((uint32_t)subpass->depth_stencil_attachment == i)
             int_attach->remaining_count++;
       }
 

diff --git a/src/imagination/vulkan/pvr_pass.c b/src/imagination/vulkan/pvr_pass.c
index 23f751b..49abb3a 100644 (file)
--- a/src/imagination/vulkan/pvr_pass.c
+++ b/src/imagination/vulkan/pvr_pass.c
@@ -72,14 +72,6 @@ static inline bool pvr_subpass_has_msaa_input_attachment(
    return false;
 }
 
-static inline size_t
-pvr_num_subpass_attachments(const VkSubpassDescription2 *desc)
-{
-   return desc->inputAttachmentCount + desc->colorAttachmentCount +
-          (desc->pResolveAttachments ? desc->colorAttachmentCount : 0) +
-          (desc->pDepthStencilAttachment != NULL);
-}
-
 static bool pvr_is_subpass_initops_flush_needed(
    const struct pvr_render_pass *pass,
    const struct pvr_renderpass_hwsetup_render *hw_render)
@@ -453,8 +445,10 @@ VkResult pvr_CreateRenderPass2(VkDevice _device,
 
    subpass_attachment_count = 0;
    for (uint32_t i = 0; i < pCreateInfo->subpassCount; i++) {
+      const VkSubpassDescription2 *desc = &pCreateInfo->pSubpasses[i];
       subpass_attachment_count +=
-         pvr_num_subpass_attachments(&pCreateInfo->pSubpasses[i]);
+         desc->inputAttachmentCount + desc->colorAttachmentCount +
+         (desc->pResolveAttachments ? desc->colorAttachmentCount : 0);
    }
 
    vk_multialloc_add(&ma,
@@ -579,9 +573,10 @@ VkResult pvr_CreateRenderPass2(VkDevice _device,
       }
 
       if (desc->pDepthStencilAttachment) {
-         subpass->depth_stencil_attachment = subpass_attachments++;
-         *subpass->depth_stencil_attachment =
+         subpass->depth_stencil_attachment =
             desc->pDepthStencilAttachment->attachment;
+      } else {
+         subpass->depth_stencil_attachment = VK_ATTACHMENT_UNUSED;
       }
 
       /* Give the dependencies a slice of the subpass_attachments array. */

diff --git a/src/imagination/vulkan/pvr_pipeline.c b/src/imagination/vulkan/pvr_pipeline.c
index 9442cd7..89962d9 100644 (file)
--- a/src/imagination/vulkan/pvr_pipeline.c
+++ b/src/imagination/vulkan/pvr_pipeline.c
@@ -1941,9 +1941,9 @@ pvr_create_subpass_info(const VkGraphicsPipelineCreateInfo *const info)
          pass->attachments[subpass->color_attachments[i]].aspects;
    }
 
-   if (subpass->depth_stencil_attachment) {
+   if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) {
       attachment_aspects |=
-         pass->attachments[*subpass->depth_stencil_attachment].aspects;
+         pass->attachments[subpass->depth_stencil_attachment].aspects;
    }
 
    return (struct vk_subpass_info){

diff --git a/src/imagination/vulkan/pvr_private.h b/src/imagination/vulkan/pvr_private.h
index ab96603..28a9262 100644 (file)
--- a/src/imagination/vulkan/pvr_private.h
+++ b/src/imagination/vulkan/pvr_private.h
@@ -1031,7 +1031,7 @@ struct pvr_render_subpass {
    uint32_t input_count;
    uint32_t *input_attachments;
 
-   uint32_t *depth_stencil_attachment;
+   uint32_t depth_stencil_attachment;
 
    /*  Derived and other state. */
    uint32_t dep_count;