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

[Kenneth Graunke]

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.  But whatever you and Eric decide is fine.

> -         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;
> +	 }
>         }
>      }