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

[Pohjolainen, Topi]

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?

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