[Mesa-dev] [PATCH v2 04/24] anv/cmd_buffer: report errors in vkBeginCommandBuffer()

[Pohjolainen, Topi]

On Mon, Mar 13, 2017 at 02:29:31PM +0100, Iago Toral wrote:
> On Mon, 2017-03-13 at 15:16 +0200, Pohjolainen, Topi wrote:
> > On Mon, Mar 13, 2017 at 11:02:30AM +0100, Iago Toral wrote:
> > > 
> > > On Mon, 2017-03-13 at 11:59 +0200, Pohjolainen, Topi wrote:
> > > > 
> > > > On Fri, Mar 10, 2017 at 01:38:17PM +0100, Iago Toral Quiroga
> > > > wrote:
> > > > > 
> > > > > 
> > > > > ---
> > > > >  src/intel/vulkan/genX_cmd_buffer.c | 15 +++++++++------
> > > > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > index 9fdc08b..f74109e 100644
> > > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > > @@ -381,7 +381,7 @@ transition_depth_buffer(struct
> > > > > anv_cmd_buffer
> > > > > *cmd_buffer,
> > > > >  /**
> > > > >   * Setup anv_cmd_state::attachments for vkCmdBeginRenderPass.
> > > > >   */
> > > > > -static void
> > > > > +static VkResult
> > > > >  genX(cmd_buffer_setup_attachments)(struct anv_cmd_buffer
> > > > > *cmd_buffer,
> > > > >                                     struct anv_render_pass
> > > > > *pass,
> > > > >                                     const VkRenderPassBeginInfo
> > > > > *begin)
> > > > > @@ -393,7 +393,7 @@ genX(cmd_buffer_setup_attachments)(struct
> > > > > anv_cmd_buffer *cmd_buffer,
> > > > >  
> > > > >     if (pass->attachment_count == 0) {
> > > > >        state->attachments = NULL;
> > > > > -      return;
> > > > > +      return VK_SUCCESS;
> > > > >     }
> > > > >  
> > > > >     state->attachments = vk_alloc(&cmd_buffer->pool->alloc,
> > > > > @@ -402,7 +402,7 @@ genX(cmd_buffer_setup_attachments)(struct
> > > > > anv_cmd_buffer *cmd_buffer,
> > > > >                                   8,
> > > > > VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> > > > >     if (state->attachments == NULL) {
> > > > >        /* FIXME: Propagate VK_ERROR_OUT_OF_HOST_MEMORY to
> > > > > vkEndCommandBuffer */
> > > > > -      abort();
> > > > > +      return VK_ERROR_OUT_OF_HOST_MEMORY;
> > > > Locally this looks fine. I just don't know what are the
> > > > requirements
> > > > for the
> > > > caller. Should we somehow mark the error down in the driver side
> > > > so
> > > > that any
> > > > following calls are rejected/ignored? The FIXME above talks about
> > > > vkEndCommandBuffer().
> > > This is addressed in a later patch (#14) that removes the FIXME. 
> > Right. I suppose it would be cleaner to squash this with patch 14. Is
> > there
> > something here that is needed before 14?
> 
> At this point in the series we don't have the error tracking in
> anv_batch yet, which patch 14 needs. That support is added in patches 5
> and 6, so we can probably move this right after those and squash patch
> 14 with it if you like that better. I kept both patches separate
> because the patches fix different issues in different API calls, one
> only needs the return value and can report the error immediately, while
> the other one needs the error handler so it can report the error at a
> later time.

I was only worried that removing the abort() before storing the error in the
driver gave us bug reports that were possibly difficult to understand. But
you are probably going to land these patches the same time anyway so I guess
it doesn't make any difference if they are squashed or not.