[Mesa-dev] [PATCH v2 03/24] anv/blorp: return early if we failed to create the shader binary

[Iago Toral]

On Tue, 2017-03-14 at 11:53 +0200, Pohjolainen, Topi wrote:
> On Tue, Mar 14, 2017 at 10:35:43AM +0100, Iago Toral wrote:
> > 
> > On Tue, 2017-03-14 at 10:12 +0200, Pohjolainen, Topi wrote:
> > > 
> > > On Fri, Mar 10, 2017 at 01:38:16PM +0100, Iago Toral Quiroga
> > > wrote:
> > > > 
> > > > 
> > > > ---
> > > >  src/intel/vulkan/anv_blorp.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/src/intel/vulkan/anv_blorp.c
> > > > b/src/intel/vulkan/anv_blorp.c
> > > > index d79c5e0..c871f03 100644
> > > > --- a/src/intel/vulkan/anv_blorp.c
> > > > +++ b/src/intel/vulkan/anv_blorp.c
> > > > @@ -72,6 +72,9 @@ upload_blorp_shader(struct blorp_context
> > > > *blorp,
> > > >                                         key, key_size, kernel,
> > > > kernel_size,
> > > >                                         prog_data,
> > > > prog_data_size,
> > > > &bind_map);
> > > >  
> > > > +   if (!bin)
> > > > +      return;
> > > > +
> > > This is called by brw_blorp_get_blit_kernel() which in turn is
> > > called
> > > from
> > > try_blorp_blit(). The latter will happily submit batch to the
> > > kernel
> > > where
> > > there is now no kernel to execute - that will surely lead to gpu
> > > hang.
> > > 
> > > It looks to me we would need to mark down the failure here for
> > > anv so
> > > that
> > > it knows the blit failed. In addition,
> > > brw_blorp_get_blit_kernel()
> > > should
> > > tell to try_blorp_blit() that shader upload failed so that
> > > try_blorp_blit()
> > > won't send the invalid batch to the gpu.
> > The problem here is that as far as I understand, all this happens
> > in
> > code that doesn't know if it is being run inside anv or i965 and
> > that
> > does not have access to an anv batch object.
> > 
> > One solution would be to make all the functions in between these
> > and
> > the entry points (blorp_copy and blorp_blit as far as I can tell)
> > return a bool to indicate failure so that when we call this from
> > anv we
> > can register the error in the corresponding batch.
> At least blorp_context::upload_shader() needs to indicate failure so
> that
> common code doesn't try to submit invalid batches.

Agreed, at least that seems like a reasonable first step.

Iago