-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Sun, 19 Jun 2011 02:56:51 -0700, Kenneth Graunke <kenn...@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<c...@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 c...@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----- _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev