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

[Pohjolainen, Topi]

On Tue, Mar 14, 2017 at 02:08:10PM +0100, Iago Toral wrote:
> 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...

I agree, it gets totally out of hand. I'll take one more look if there are
any alternatives.