[Mesa-dev] [PATCH v2 10/24] anv: handle failures when growing reloc lists

[Iago Toral]

On Tue, 2017-03-14 at 14:04 +0200, Pohjolainen, Topi wrote:
> On Tue, Mar 14, 2017 at 12:06:16PM +0100, Iago Toral wrote:
> > 
> > On Tue, 2017-03-14 at 12:01 +0100, Iago Toral wrote:
> > > 
> > > On Tue, 2017-03-14 at 11:25 +0200, Pohjolainen, Topi wrote:
> > > > 
> > > > 
> > > > On Fri, Mar 10, 2017 at 01:38:23PM +0100, Iago Toral Quiroga
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > Growing the reloc list happens through calling
> > > > > anv_reloc_list_add()
> > > > > or
> > > > > anv_reloc_list_append(). Make sure that we call these through
> > > > > helpers
> > > > > that check the result and set the batch error status if
> > > > > needed.
> > > > > 
> > > > > v2:
> > > > >   - Handling the crashes is not good enough, we need to keep
> > > > > track
> > > > > of
> > > > >     the error, for that, keep track of the errors in the
> > > > > batch
> > > > > instead (Jason).
> > > > >   - Make reloc list growth go through helpers so we can have
> > > > > a
> > > > > central
> > > > >     place where we can do error tracking (Jason).
> > > > > ---
> > > > >  src/intel/vulkan/anv_batch_chain.c | 29
> > > > > ++++++++++++++++++++----
> > > > > -----
> > > > >  src/intel/vulkan/genX_blorp_exec.c |  7 +++++--
> > > > >  src/intel/vulkan/genX_cmd_buffer.c | 25 ++++++++++++++----
> > > > > ----
> > > > > ---
> > > > >  3 files changed, 39 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/src/intel/vulkan/anv_batch_chain.c
> > > > > b/src/intel/vulkan/anv_batch_chain.c
> > > > > index 3774172..2add5bd 100644
> > > > > --- a/src/intel/vulkan/anv_batch_chain.c
> > > > > +++ b/src/intel/vulkan/anv_batch_chain.c
> > > > > @@ -151,8 +151,9 @@ anv_reloc_list_add(struct anv_reloc_list
> > > > > *list,
> > > > >     const uint32_t domain =
> > > > >        target_bo->is_winsys_bo ? I915_GEM_DOMAIN_RENDER : 0;
> > > > >  
> > > > > -   anv_reloc_list_grow(list, alloc, 1);
> > > > > -   /* TODO: Handle failure */
> > > > > +   VkResult result = anv_reloc_list_grow(list, alloc, 1);
> > > > > +   if (result != VK_SUCCESS)
> > > > > +      return 0;
> > > > None of the callers of anv_reloc_list_add() or
> > > > anv_reloc_list_append() are
> > > > currently interested of the return value.
> > > and_reloc_list_append() already returns a VkResult.
> > > 
> > > As for the callers of anv_reloc_list_add(),
> > > anv_batch_emit_reloc()
> > > needs the offset computed by anv_reloc_list_add(), but that seems
> > > to
> > > be
> > > the only one, so we could do what you suggest below.
> > > 
> > > > 
> > > > 
> > > >  And if they were, they could
> > > > easily calculate it themselves: both target_bo and delta are
> > > > inputs.
> > > > So instead of relying on zero offset indicating error we could
> > > > simply
> > > > change the return type to VkResult. Or did I miss something?
> > > I think this should be fine.
> > Well, except that anv_batch_emit_reloc needs to return that offset
> > to
> > its callers, so even if receives a VkResult and computes the offset
> > itself, in the case of an error we still need to return an offset,
> > so I
> > don't think there is much gain in doing this, right?
> Oh, I missed that. Yeah, then it doesn't work. I'm just not super
> happy
> relying on offset == 0 indicating failure. Does the diff get out hand
> if 
> provided offset as outgoing argument (and returning VkResult)?

Right I don't particularly like this either, however, I am not sure we
can do much about it. This is called from two places:

1) _blorp_combine_address() -> blorp_emit_reloc()
-> anv_batch_emit_reloc()

2) _anv_combine_address() -> anv_batch_emit_reloc()

_blorp_combine_address and _anv_combine_address are wrapped by the
_gen_combine_address macros that are used all over the place in auto-
generated files to fill dwords in various state packets.

So the thing is, these state packets need to be filled one way or
another. If we don't they will just have garbage, so in case of
failure, writing 0 does not sound particularly bad to me, the focus
needs to be on avoiding to execute things that are known to be broken,
which is what we are trying to do with this series.

In other words, we could modify the functions in those chains above to
return a VkResult, that should not be a big change, however,
eventually, the generator script would have to do something depending
on the result: if it is a failure there is not much we can do, we could
skip writing the state packet, but that would only leave garbage, or we
could decide to write it to 0, but then all this change would be for
nothing anyway.

Also, I count  ~380 call sites of the _gen_combine_address macro in the
autogenerated files, so for each call site, the generator would
generate code that:

1. declares a variable so it can pass a pointer through that chain of
functions.
2. Checks the result
3. Depending on the result, decides what to write to the state packet.

to me it doesn't look worth the trouble when there is not a clear
gain...

Iago