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

Pohjolainen, Topi topi.pohjolainen at gmail.com
Tue Mar 14 12:04:17 UTC 2017


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)?


More information about the mesa-dev mailing list