[Mesa-dev] [PATCH] intel: Replace intel_renderbuffer::region with a miptree [v2]

[Chad Versace]

On 11/18/2011 05:03 PM, Eric Anholt wrote:
> On Fri, 18 Nov 2011 12:50:36 -0800, Chad Versace <chad.versace at linux.intel.com> wrote:
>> Essentially, this patch just globally substitutes `irb->region` with
>> `irb->mt->region` and then does some minor cleanups to avoid segfaults
>> and other problems.
>>
>> This is in preparation for
>>   1. Fixing scatter/gather for mipmapped separate stencil textures.
>>   2. Supporting HiZ for mipmapped depth textures.
>>
>> As a nice benefit, this lays down some preliminary groundwork for easily
>> texturing from any renderbuffer, even those of the window system.
>>
>> A future commit will replace intel_mipmap_tree::hiz_region with a miptree.
>>
>> v2:
>>    - Return early in intel_process_dri2_buffer_*() if region allocation
>>      fails.
>>    - Fix double semicolon.
>>    - Fix miptree reference leaks in the following functions:
>>        intel_process_dri2_buffer_with_separate_stencil()
>>        intel_image_target_renderbuffer_storage()
> 
>> @@ -702,20 +696,21 @@ intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer
>>        _mesa_reference_renderbuffer(&irb->wrapped_stencil, stencil_rb);
>>  
>>     } else {
>> -      irb->region = intel_region_alloc(intel->intelScreen, tiling, cpp,
>> -				       width, height, true);
>> -      if (!irb->region)
>> +      irb->mt = intel_miptree_create_for_renderbuffer(intel, rb->Format,
>> +                                                      tiling, cpp,
>> +                                                      width, height);
>> +      if (!irb->mt)
>>  	 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,
>> -					      true);
>> -	 if (!irb->hiz_region) {
>> -	    intel_region_release(&irb->region);
>> +	 irb->mt->hiz_region = intel_region_alloc(intel->intelScreen,
>> +	                                          I915_TILING_Y,
>> +	                                          cpp,
>> +	                                          rb->Width,
>> +	                                          rb->Height,
>> +	                                          true);
>> +	 if (!irb->mt) {
>> +	    intel_miptree_release(&irb->mt);
>>  	    return false;
> 
> I think this was meant to be if (!irb->mt->his_region).
> 
> Other than that,
> 
> Reviewed-by: Eric Anholt <eric at anholt.net>

I really meant to release irb->mt, not irb->mt->hiz_mt. Perhaps rephrasing
the code like this reveals the code's intention more explicity.

      irb->mt = intel_miptree_create_for_renderbuffer(...)

      if (!irb->mt)
	 return false;

      if (intel->vtbl.is_hiz_depth_format(intel, rb->Format)) {
	 intel_miptree_alloc_hiz(intel, irb->mt);
	 if (!irb->mt->hiz_mt) {
	    intel_miptree_release(&irb->mt);
	    return false;
	 }
      }

I chose to release the renderbuffer when the hiz miptree allocation fails
because on gen6, hiz and separate stencil must be enabled together. If,
when hiz miptree allocation failed, we allowed intel_alloc_rb_storage()
to continue and succeed, this problematic situation could occur:
   1. User creates a GL_DEPTH_COMPONENT24 renderbuffer. HiZ miptree allocation failed.
   2. User attaches the depthbuffer to a fb and does some drawing. No problem.
   3. User creates a GL_STENCIL_INDEX8 renderbuffer.
   4. User attaches the stencilbuffer to the same fb. Here we must to detect
      that the accompanying depthbuffer, without hiz, is incompatible, and
      mark the fb as incomplete.
   5. User says WTF.

I'm withholding your rb until you reply.

----
Chad Versace
chad.versace at linux.intel.com