[Mesa-dev] [PATCH 2/2] i965: in set_read_rb_tex_image() check _mesa_meta_bind_rb_as_tex_image() did succeed

[Pohjolainen, Topi]

On Fri, May 30, 2014 at 02:37:35PM +0300, Pohjolainen, Topi wrote:
> On Fri, May 30, 2014 at 01:54:48PM +0300, Juha-Pekka Heikkila wrote:
> > Check if _mesa_meta_bind_rb_as_tex_image() did give the texture.
> > If no texture was given there is already either
> > GL_INVALID_VALUE or GL_OUT_OF_MEMORY error set in context.
> > 
> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > ---
> >  src/mesa/drivers/common/meta_blit.c               | 10 +++++++++-
> >  src/mesa/drivers/dri/i965/brw_meta_stencil_blit.c |  6 ++++--
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/common/meta_blit.c b/src/mesa/drivers/common/meta_blit.c
> > index f26ef93..eeaaedd 100644
> > --- a/src/mesa/drivers/common/meta_blit.c
> > +++ b/src/mesa/drivers/common/meta_blit.c
> > @@ -615,13 +615,21 @@ _mesa_meta_bind_rb_as_tex_image(struct gl_context *ctx,
> >                                  GLenum *target)
> >  {
> >     struct gl_texture_image *texImage;
> > +   GLuint tempTex;
> >  
> >     if (rb->NumSamples > 1)
> >        *target = GL_TEXTURE_2D_MULTISAMPLE;
> >     else
> >        *target = GL_TEXTURE_2D;
> >  
> > -   _mesa_GenTextures(1, tex);
> > +   tempTex = 0;
> > +   _mesa_GenTextures(1, &tempTex);
> > +   if (tempTex == 0) {
> > +      return false;
> > +   } else {
> 
> No real need for explicit else - you return already in the other branch.
> 
> > +      *tex = tempTex;
> > +   }
> > +
> >     _mesa_BindTexture(*target, *tex);
> >     *texObj = _mesa_lookup_texture(ctx, *tex);
> >     texImage = _mesa_get_tex_image(ctx, *texObj, *target, 0);
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_stencil_blit.c b/src/mesa/drivers/dri/i965/brw_meta_stencil_blit.c
> > index 5d132b7..6da2ba6 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_stencil_blit.c
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_stencil_blit.c
> > @@ -385,8 +385,10 @@ set_read_rb_tex_image(struct gl_context *ctx, struct fb_tex_blit_state *blit,
> >        *target = tex_obj->Target;
> >        level = att->TextureLevel;
> >     } else {
> > -      _mesa_meta_bind_rb_as_tex_image(ctx, rb, &blit->tempTex, &tex_obj,
> > -                                      target);
> > +      if (_mesa_meta_bind_rb_as_tex_image(ctx, rb, &blit->tempTex, &tex_obj,
> > +                                          target) == false ) {
> 
> Usually the style is:
> 
>          if (!_mesa_meta_bind_rb_as_tex_image(ctx, rb, &blit->tempTex,
>                                               &tex_obj, target))
>             return;
> 
> I need to think a little if there is something else to be considered when
> we fail inside a meta path.

The minimum we need to do is set_read_rb_tex_image() to return status
to brw_meta_stencil_blit() where the fault handling should tear down the
current meta blit (by calling _mesa_meta_end()).

And then if we are going to do this properly all the way, probably
brw_meta_stencil_blit() should return success further back to
brw_meta_fbo_stencil_blit() and brw_meta_stencil_updownsample(), and then
callers of these two should note a failure.
We have similar issue in brw_meta_updownsample(), it ignores the result of
_mesa_BlitFramebuffer().

Only I'm not sure how much all this is worth while - if we ran out of memory,
no other fallback path shouldn't be any better. And in fact they aren'r even
capable of doing the correct thing (that is why these special paths exist).

> 
> > +         return;
> > +      }
> >     }
> >  
> >     blit->baseLevelSave = tex_obj->BaseLevel;
> > -- 
> > 1.8.1.2
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev