[Mesa-dev] [PATCH v2 21/24] anv/blorp: make anv_cmd_buffer_alloc_blorp_binding_table() return a VkResult

[Pohjolainen, Topi]

On Tue, Mar 14, 2017 at 12:18:59PM +0100, Iago Toral wrote:
> On Tue, 2017-03-14 at 11:39 +0200, Pohjolainen, Topi wrote:
> > On Fri, Mar 10, 2017 at 01:38:34PM +0100, Iago Toral Quiroga wrote:
> > > 
> > > Instead of asserting inside the function, and then use use that
> > > information
> > > to return early from its callers upon failure.
> > > ---
> > >  src/intel/vulkan/anv_blorp.c       | 35 ++++++++++++++++++++----
> > > -----------
> > >  src/intel/vulkan/anv_private.h     |  5 +++--
> > >  src/intel/vulkan/genX_blorp_exec.c |  8 ++++++--
> > >  3 files changed, 29 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/src/intel/vulkan/anv_blorp.c
> > > b/src/intel/vulkan/anv_blorp.c
> > > index c871f03..de5b0ec 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -904,31 +904,31 @@ void anv_CmdClearDepthStencilImage(
> > >     blorp_batch_finish(&batch);
> > >  }
> > >  
> > > -struct anv_state
> > > +VkResult
> > >  anv_cmd_buffer_alloc_blorp_binding_table(struct anv_cmd_buffer
> > > *cmd_buffer,
> > >                                           uint32_t num_entries,
> > > -                                         uint32_t *state_offset)
> > > +                                         uint32_t *state_offset,
> > > +                                         struct anv_state
> > > *bt_state)
> > >  {
> > > -   struct anv_state bt_state =
> > > -      anv_cmd_buffer_alloc_binding_table(cmd_buffer, num_entries,
> > > -                                         state_offset);
> > > -   if (bt_state.map == NULL) {
> > > +   *bt_state = anv_cmd_buffer_alloc_binding_table(cmd_buffer,
> > > num_entries,
> > > +                                                  state_offset);
> > > +   if (bt_state->map == NULL) {
> > >        /* We ran out of space.  Grab a new binding table block. */
> > > -      MAYBE_UNUSED VkResult result =
> > > -         anv_cmd_buffer_new_binding_table_block(cmd_buffer);
> > > -      assert(result == VK_SUCCESS);
> > > +      VkResult result =
> > > anv_cmd_buffer_new_binding_table_block(cmd_buffer);
> > > +      if (result != VK_SUCCESS)
> > > +         return result;
> > >  
> > >        /* Re-emit state base addresses so we get the new surface
> > > state base
> > >         * address before we start emitting binding tables etc.
> > >         */
> > >        anv_cmd_buffer_emit_state_base_address(cmd_buffer);
> > >  
> > > -      bt_state = anv_cmd_buffer_alloc_binding_table(cmd_buffer,
> > > num_entries,
> > > -                                                    state_offset);
> > > -      assert(bt_state.map != NULL);
> > > +      *bt_state = anv_cmd_buffer_alloc_binding_table(cmd_buffer,
> > > num_entries,
> > > +                                                     state_offset)
> > > ;
> > > +      assert(bt_state->map != NULL);
> > >     }
> > >  
> > > -   return bt_state;
> > > +   return VK_SUCCESS;
> > >  }
> > >  
> > >  static uint32_t
> > > @@ -936,8 +936,13 @@ binding_table_for_surface_state(struct
> > > anv_cmd_buffer *cmd_buffer,
> > >                                  struct anv_state surface_state)
> > >  {
> > >     uint32_t state_offset;
> > > -   struct anv_state bt_state =
> > > -      anv_cmd_buffer_alloc_blorp_binding_table(cmd_buffer, 1,
> > > &state_offset);
> > > +   struct anv_state bt_state;
> > > +
> > > +   VkResult result =
> > > +      anv_cmd_buffer_alloc_blorp_binding_table(cmd_buffer, 1,
> > > &state_offset,
> > > +                                               &bt_state);
> > > +   if (result != VK_SUCCESS)
> > > +      return 0;
> > This leads to gpu hang as well I think: clear_color_attachment()
> > calls
> > blorp_clear_attachments() which sends a batch to gpu regardless of
> > binding
> > table offset being zero.
> 
> Also, thinking a bit more about this, I am not sure that 0 can't be a
> valid binding table offset...
> 
> How about we make this function take a pointer to the biding table
> offset instead and we make it return a VkResult? Then we make callers
> bail if the result is not VK_SUCCESS.

Sounds good, thanks :)