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

[Iago Toral]

On Tue, 2017-03-14 at 16:14 +0200, Pohjolainen, Topi wrote:
> On Tue, Mar 14, 2017 at 04:02:17PM +0200, Pohjolainen, Topi wrote:
> > 
> > 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.
> Couldn't we still make anv_batch_emit_reloc() return VkResult and to
> provide
> the offset as argument. Those two callers, _anv_combine_address() and
> blorp_emit_reloc(), could set the error state and like you said they
> could
> return zeros as it doesn't matter what gets filled into the batch.
> 
> Would that work?

Yes, we can do that,  although I don't see why that would make things
better: we still need to do the same, only that now we do it in those
two callers rather than in the function they call, I understand that
you see a benefit in this that I am missing?

In any case I have no objections, so if you like that better I am happy
to do it that way.

> > > 
> > > 
> > > 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.
> >