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.
> >
> >
> > uint32_t *bt_map = bt_state.map;
> > bt_map[0] = surface_state.offset + state_offset;
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 7490d0c..adb8ce2 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1463,10 +1463,11 @@ void anv_cmd_buffer_resolve_subpass(struct
> > anv_cmd_buffer *cmd_buffer);
> > const struct anv_image_view *
> > anv_cmd_buffer_get_depth_stencil_view(const struct anv_cmd_buffer
> > *cmd_buffer);
> >
> > -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);
> >
> > void anv_cmd_buffer_dump(struct anv_cmd_buffer *cmd_buffer);
> >
> > diff --git a/src/intel/vulkan/genX_blorp_exec.c
> > b/src/intel/vulkan/genX_blorp_exec.c
> > index 90adefe..0649d3b 100644
> > --- a/src/intel/vulkan/genX_blorp_exec.c
> > +++ b/src/intel/vulkan/genX_blorp_exec.c
> > @@ -89,9 +89,13 @@ blorp_alloc_binding_table(struct blorp_batch
> > *batch, unsigned num_entries,
> > struct anv_cmd_buffer *cmd_buffer = batch->driver_batch;
> >
> > uint32_t state_offset;
> > - struct anv_state bt_state =
> > + struct anv_state bt_state;
> > +
> > + VkResult result =
> > anv_cmd_buffer_alloc_blorp_binding_table(cmd_buffer,
> > num_entries,
> > - &state_offset);
> > + &state_offset,
> > &bt_state);
> > + if (result != VK_SUCCESS)
> > + return;
> >
> > uint32_t *bt_map = bt_state.map;
> > *bt_offset = bt_state.offset;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
