[Mesa-dev] [PATCH v2 00/24] anv: Handle out of memory situations better

[Iago Toral]

On Wed, 2017-03-15 at 10:47 +0200, Pohjolainen, Topi wrote:
> On Tue, Mar 14, 2017 at 02:15:51PM +0100, Iago Toral wrote:
> > 
> > BTW, in case it is useful, I have the series available here,
> > including
> > fixes to review feedback obtained so far:
> > 
> > https://github.com/Igalia/mesa/tree/vk-oom
> I recorded comments in patches 2, 3, 4, 5, 7, 9, 10, 21 and 22. In
> case of 4
> the question was if we should squash it with 14 - I don't have strong
> preference, either way is fine. And I think we agree on the difficult
> bits.
> Patch 10 I'd like to see how it looks revised, the rest should be
> trivial
> enough to fix locally.

Thanks a lot for all the review effort Topi, the version with your
feedback is more thorough and robust.

I have just sent a v3 of patch 10 for review.

If you want to have a look at the final version of any of the other
patches you can use this branch from my github repo:
https://github.com/Igalia/mesa/tree/vk-oom

Just a couple of things worth pointing out:

1. I made the change where we make the upload_shader() hook return a
bool a separate commit (that is patch #2).

2. Ignore the patch "anv: ensure that we don't ever try to adjust
relocations more than once" (which is not reviewed), that should not be
pushed as the specs for that are under discussion, I only kept it in
the branch because the tests under development by Khronos currently
require this.

Iago

> Otherwise the series is:
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 
> > 
> > 
> > Iago
> > 
> > On Fri, 2017-03-10 at 13:38 +0100, Iago Toral Quiroga wrote:
> > > 
> > > I think this series addresses all the issues raised for v1,
> > > except
> > > making similar changes to anv_cmd_buffer_alloc_dynamic_state and
> > > anv_cmd_buffer_alloc_blorp_binding_table which Jason suggested we
> > > should
> > > probably do as well, and that I'd like to cover separately from
> > > this.
> > > The main change from v1 is that we now track errors in anv_batch,
> > > which
> > > allows us to track and report errors that occur during batch
> > > emissions as
> > > well.
> > > 
> > > As discussed during v1, I have dropped the patch
> > > "anv: ensure that we don't ever try to adjust relocations more
> > > than
> > > once", since
> > > Jason pointed out that the issue this was trying to address was
> > > under
> > > discussion
> > > in Khronos. I have also pointed that discussion to the test
> > > developers so that
> > > the tests are updated accordingly when a final decision is made.
> > > 
> > > Besides addressing review feedback, this series incorporates a
> > > bunch
> > > of
> > > additional improvements including better reporting of errors for
> > > pipelines
> > > (we can do this now because we track out of memory errors in
> > > batches)
> > > and a few other fixes, mostly crash guards that I had missed
> > > before
> > > and
> > > a few other minor things.
> > > 
> > > I have also changed a bit the order of some patches in the series
> > > so
> > > related fixes are closer to each other.
> > > 
> > > Iago Toral Quiroga (24):
> > >   anv: remove unnecessary function prototype.
> > >   anv: do not try to ref/unref NULL shaders
> > >   anv/blorp: return early if we failed to create the shader
> > > binary
> > >   anv/cmd_buffer: report errors in vkBeginCommandBuffer()
> > >   anv/cmd_buffer: add a status field to anv_batch
> > >   anv: add anv_batch_set_error() and anv_batch_has_error()
> > > helpers
> > >   anv: handle allocation failure in anv_batch_emit_batch()
> > >   anv: handle allocation failure in anv_batch_emit_dwords()
> > >   anv: avoid crashes when failing to allocate batches
> > >   anv: handle failures when growing reloc lists
> > >   anv/cmd_buffer: report tracked errors in vkEndCommandBuffer()
> > >   anv/cmd_buffer: skip vkCmdNextSubpass() for broken command
> > > buffers
> > >   anv/cmd_buffer: skip vkCmdEndRenderPass() for broken command
> > > buffers
> > >   anv/cmd_buffer: handle allocation errors during
> > > vkCmdBeginRenderPass()
> > >   anv/cmd_buffer: handle out of memory during vkCmdPushConstants
> > >   anv: handle memory allocation errors during queue submissions
> > >   anv/cmd_buffer: skip vkCmdDraw*() on broken command buffers
> > >   anv/cmd_buffer: skip vkCmdDispatch() on broken command buffers
> > >   anv/cmd_buffer: skip vkCmdExecuteCommands() on broken command
> > > buffers
> > >   anv/device: assert that commands submitted to a queue are not
> > > bogus
> > >   anv/blorp: make anv_cmd_buffer_alloc_blorp_binding_table()
> > > return a
> > >     VkResult
> > >   anv: handle errors while allocating new binding table blocks
> > >   anv: handle errors in emit_binding_table() and emit_samplers()
> > >   anv: improve error reporting when creating pipelines
> > > 
> > >  src/intel/blorp/blorp_genX_exec.h     |  25 +++++---
> > >  src/intel/vulkan/anv_batch_chain.c    |  62 +++++++++++++------
> > >  src/intel/vulkan/anv_blorp.c          |  38 +++++++-----
> > >  src/intel/vulkan/anv_cmd_buffer.c     |  19 +++++-
> > >  src/intel/vulkan/anv_device.c         |   1 +
> > >  src/intel/vulkan/anv_pipeline.c       |   1 +
> > >  src/intel/vulkan/anv_pipeline_cache.c |   5 +-
> > >  src/intel/vulkan/anv_private.h        |  56 +++++++++++++----
> > >  src/intel/vulkan/genX_blorp_exec.c    |  18 +++---
> > >  src/intel/vulkan/genX_cmd_buffer.c    | 113
> > > +++++++++++++++++++++++++---------
> > >  src/intel/vulkan/genX_pipeline.c      |   9 ++-
> > >  11 files changed, 253 insertions(+), 94 deletions(-)
> > >