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. > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
