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.
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
c...@chad-versace.us
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
