On Tue, 2017-03-07 at 09:32 -0800, Jason Ekstrand wrote:
> On Mon, Mar 6, 2017 at 11:15 PM, Iago Toral Quiroga <ito...@igalia.co
> m> wrote:
> > The vkCmd*() functions do not report errors, instead, any errors
> > should be
> > reported by the time we call vkEndCommandBuffer(). This means that
> > we
> > need to track things such as "out of host memory" and use that
> > information
> > to avoid executing code that could lead to crashes (due to the fact
> > that the
> > command buffer could not allocate the memory it needs) and report
> > the error
> > when the client calls vkEndCommandBuffer().
> >
> > Notice this patch is not sufficient to track and report all
> > possible cases
> > of out of host memory situations that can be produced while
> > recording
> > command buffers. Later patches will fix some of the missing cases.
> > Also,
> > some allocations may occur deep into the driver code (for example
> > during
> > batch emissions) and are more difficult to track because at that
> > point we
> > don't even have access to the command buffer object. Dealing with
> > these
> > scenarios would require more changes but for now this is sufficient
> > to make
> > CTS happy.
> Thinking about things a bit more, I don't think that is actually all
> that hard...
>
> > Fixes:
> > dEQP-VK.api.out_of_host_memory.cmd_begin_render_pass
> > ---
> > src/intel/vulkan/anv_cmd_buffer.c | 5 +++++
> > src/intel/vulkan/anv_private.h | 7 +++++++
> > src/intel/vulkan/genX_cmd_buffer.c | 22 +++++++++++++++++++---
> > 3 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> > b/src/intel/vulkan/anv_cmd_buffer.c
> > index cab1dd7..01447b0 100644
> > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > @@ -117,6 +117,8 @@ anv_cmd_state_reset(struct anv_cmd_buffer
> > *cmd_buffer)
> > {
> > struct anv_cmd_state *state = &cmd_buffer->state;
> >
> > + cmd_buffer->error_status = VK_SUCCESS;
> > +
> > memset(&state->descriptors, 0, sizeof(state->descriptors));
> > memset(&state->push_constants, 0, sizeof(state-
> > >push_constants));
> > memset(state->binding_tables, 0, sizeof(state-
> > >binding_tables));
> > @@ -185,6 +187,8 @@ static VkResult anv_create_cmd_buffer(
> > if (cmd_buffer == NULL)
> > return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> >
> > + cmd_buffer->error_status = VK_SUCCESS;
> > +
> > cmd_buffer->_loader_data.loaderMagic = ICD_LOADER_MAGIC;
> > cmd_buffer->device = device;
> > cmd_buffer->pool = pool;
> > @@ -217,6 +221,7 @@ static VkResult anv_create_cmd_buffer(
> > return VK_SUCCESS;
> >
> > fail:
> > + cmd_buffer->error_status = result;
> > vk_free(&cmd_buffer->pool->alloc, cmd_buffer);
> >
> > return result;
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index d3e32bc..46b51a9 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1377,6 +1377,13 @@ struct anv_cmd_buffer {
> > VkCommandBufferLevel level;
> >
> > struct anv_cmd_state state;
> > +
> > + /**
> > + * Current error status of the command buffer. Used to track
> > inconsistent
> > + * or incomplete command buffer states that are the consequence
> > of run-time
> > + * errors such as out of memory scenarios.
> > + */
> > + VkResult error_status;
> If we put this in anv_batch, we can easily set the status from "deep"
> parts of the driver.
Yeah, that seems like a a better approach, I'll give it a try, thanks!
> Also, I'm not sure the "error" part of the name is needed. "status"
> should be ok.
That was because having a field called "state" and another called
"status" in the same struct seemed like it could be a bit confusing. In
any case, if we are going to move this to anv_batch that is no longer
an issue.
> One other thing we may want to do is to add a anv_batch_set_error
> helper and only take first error. That way we can track
> OUT_OF_HOST_MEMORY vs. OUT_OF_DEVICE_MEMORY.
Yes, I was also thinking about doing like this to avoid stomping
previous errors. I'll do it.
> > };
> >
> > VkResult anv_cmd_buffer_init_batch_bo_chain(struct anv_cmd_buffer
> > *cmd_buffer);
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index b46a922..cbad144 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -401,8 +401,8 @@ genX(cmd_buffer_setup_attachments)(struct
> > anv_cmd_buffer *cmd_buffer,
> > sizeof(state-
> > >attachments[0]),
> > 8,
> > VK_SYSTEM_ALLOCATION_SCOPE_OBJECT);
> > if (state->attachments == NULL) {
> > - /* FIXME: Propagate VK_ERROR_OUT_OF_HOST_MEMORY to
> > vkEndCommandBuffer */
> > - return VK_ERROR_OUT_OF_HOST_MEMORY;
> > + /* Propagate VK_ERROR_OUT_OF_HOST_MEMORY to
> > vkEndCommandBuffer */
> > + return cmd_buffer->error_status =
> > VK_ERROR_OUT_OF_HOST_MEMORY;
> > }
> >
> > bool need_null_state = false;
> > @@ -615,6 +615,9 @@ genX(EndCommandBuffer)(
> > {
> > ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> >
> > + if (cmd_buffer->error_status != VK_SUCCESS)
> > + return cmd_buffer->error_status;
> > +
> > /* We want every command buffer to start with the PMA fix in a
> > known state,
> > * so we disable it at the end of the command buffer.
> > */
> > @@ -2470,7 +2473,14 @@ void genX(CmdBeginRenderPass)(
> > cmd_buffer->state.framebuffer = framebuffer;
> > cmd_buffer->state.pass = pass;
> > cmd_buffer->state.render_area = pRenderPassBegin->renderArea;
> > - genX(cmd_buffer_setup_attachments)(cmd_buffer, pass,
> > pRenderPassBegin);
> > + VkResult result =
> > + genX(cmd_buffer_setup_attachments)(cmd_buffer, pass,
> > pRenderPassBegin);
> > +
> > + /* If we failed to setup the attachments we should not try to
> > go further */
> > + if (result != VK_SUCCESS) {
> > + assert(cmd_buffer->error_status != VK_SUCCESS);
> > + return;
> > + }
> >
> > genX(flush_pipeline_select_3d)(cmd_buffer);
> >
> > @@ -2483,6 +2493,9 @@ void genX(CmdNextSubpass)(
> > {
> > ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> >
> > + if (cmd_buffer->error_status != VK_SUCCESS)
> > + return;
> > +
> > assert(cmd_buffer->level == VK_COMMAND_BUFFER_LEVEL_PRIMARY);
> >
> > anv_cmd_buffer_resolve_subpass(cmd_buffer);
> > @@ -2499,6 +2512,9 @@ void genX(CmdEndRenderPass)(
> > {
> > ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> >
> > + if (cmd_buffer->error_status != VK_SUCCESS)
> > + return;
> > +
> > anv_cmd_buffer_resolve_subpass(cmd_buffer);
> >
> > /* Perform transitions to the final layout after all writes
> > have occurred.
> > --
> > 2.7.4
> >
> > _______________________________________________
> > 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
