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

[Chad Versace]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.
> 
> 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. 

I want to avoid code churn unless it produces a tangible benefit. Since
you think the code is sensible without the change, I'll remove this
patch.

- ----
Chad Versace
chad at chad-versace.us
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJN/mwIAAoJEAIvNt057x8il9kP/ilGEd76EdWm329PjDH9rYyO
/7ZUoXmmMmMCAccjn1U7vsgqHoIGYjLTxMTPo0S0WgI10refrHtoo3UL5gBwhul9
xHp6AbTnYCEZv4N73jZQSq9yR2cof0tqYcMiCMIWbtxqk7wZfeOJOSZg8i+wc1/U
lD5cZrap4sRQHycua7Oi1hwmz1uFkElyr8D32jtDBT/1h9kzcbIzVegRT6unMFri
dU73a/eOkk7w9omJO6MInQnKm78rxkdzO3XBUIUbkauFQ/V82/fvsCEh1cBcVW4F
2Rc9Xp8jaRBVhhrEDWOryJ3hWvPfbDu5e5AOD7FZ52aE2M026vZeUzHT+JNbk7lo
ueTegL0zTNGIG58DJEWJcmuYA71arC/xbJXOn2av5b+1pgWt3IS1wV344Kh4z//p
Ik6OlVQ8QQdqUg2RuTPFMLdGXsEIColknFW3rDL5x2aYmxVKykzh9sCMlb3oQo/H
NmjTonL45KZMPJrAn1bb2a/s63CPDCxUZ3ZqBYN3DN4nl571Uw+rP6Rw4gF+f56j
tQfeG7/rlOvF+PSg8hnX2B/SBxYMSFVolgirq/xsbW0gWz42hif3AuB1SO97HSdi
DtDzE9b56W3KIqWsvy8v/pwa5mWXMnRcvaJFHjmeyn/0USC4Xmd9v1FDnxgKaHUV
RngKyILYOmzT2x9O2L3G
=gY3R
-----END PGP SIGNATURE-----