[Mesa-dev] [PATCH 3/4] intel: Unobfuscate intel_alloc_renderbuffer_storage

[Chad Versace]

On Sun, 19 Jun 2011 02:56:51 -0700, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 06/17/2011 03:43 PM, Chad Versace wrote:
> > Hiz buffer allocation can only occur if the 'else' branch has been taken,
> > so move the hiz buffer allocation into the 'else' branch.
> >
> > Having the hiz buffer allocation dangling outside of the if-tree was just
> > damn confusing.
> >
> > Signed-off-by: Chad Versace<chad at chad-versace.us>
> > ---
> >   src/mesa/drivers/dri/intel/intel_fbo.c |   31 ++++++++++++++-----------------
> >   1 files changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/intel/intel_fbo.c b/src/mesa/drivers/dri/intel/intel_fbo.c
> > index b48eac4..0d49a55 100644
> > --- a/src/mesa/drivers/dri/intel/intel_fbo.c
> > +++ b/src/mesa/drivers/dri/intel/intel_fbo.c
> > @@ -199,23 +199,20 @@ intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer
> >      } else {
> >         irb->region = intel_region_alloc(intel->intelScreen, tiling, cpp,
> >   				       width, height, GL_TRUE);
> > -   }
> > -
> > -   if (!irb->region)
> > -      return GL_FALSE;       /* out of memory? */
> > -
> > -   ASSERT(irb->region->buffer);
> 
> NAK.  Allocation may fail in the S8 case.  Prior to this patch, it would 
> check for !irb->region and properly return false.  By moving this check 
> into the "else" branch, you fail to check this.  Leaving that check 
> below seems sensible, as it hits both clauses.

Yes. I neglected to duplicate that the (!irb->region) check in the S8
case. I'll add it to the patch.

But, "leaving that check below" is no longer possible due to patch
4 --- !irb->region is always false for the wrapped S8_Z24 case. So the
check must be removed from its dangling position and duplicated in each
appropriate if-branch.

 
> Presumably "out of memory" could happen if the application requests 
> ridiculously huge buffers.  So properly checking and saying no seems 
> like a good thing to do.  (Of course, I haven't checked if the callers 
> of this function actually check the return code...)
> 
> > -   if (intel->vtbl.is_hiz_depth_format(intel, rb->Format)) {
> 
> Clearly this only applies in the "else" case (S8 is not a HiZ format), 
> so moving this there would be fine...
> 
> > -      irb->hiz_region = intel_region_alloc(intel->intelScreen,
> > -                                           I915_TILING_Y,
> > -                                           irb->region->cpp,
> > -                                           irb->region->width,
> > -                                           irb->region->height,
> > -                                           GL_TRUE);
> > -      if (!irb->hiz_region) {
> > -         intel_region_release(&irb->region);
> 
> Therein lies the problem: You do need to check !irb->region before the 
> intel_region_release.  So you can't move this into the else clause and 
> leave the if (!irb->region) return false; below.  You'd have to 
> duplicate it, I suppose.
> 
> I actually think the code was quite sensible before this change, and 
> it's fine to leave it as is.

Since the dangling (!irb->region) check must be deleted/duplicated
anyway, it seemed a good opportunity to clean up the location of
hiz_region allocation too.

If the hiz allocation remains dangling outside the if-tree, a future
maintainer may encouter it and confoundingly ask himself "Is it possible
for the hiz allocation here to accidentally occurr when the else-branch
is not taken?". There's no harm in making the code easier for him to
understand.

> 
> > -         return GL_FALSE;
> > +      if (!irb->region)
> > +	 return false;
> > +
> > +      if (intel->vtbl.is_hiz_depth_format(intel, rb->Format)) {
> > +	 irb->hiz_region = intel_region_alloc(intel->intelScreen,
> > +					      I915_TILING_Y,
> > +					      irb->region->cpp,
> > +					      irb->region->width,
> > +					      irb->region->height,
> > +					      GL_TRUE);
> > +	 if (!irb->hiz_region) {
> > +	    intel_region_release(&irb->region);
> > +	    return false;
> > +	 }
> >         }
> >      }

----
Chad Versace
chad at chad-versace.us