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

[Chad Versace]

On 11/21/2011 11:27 AM, Chad Versace wrote:
> 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.

Oops. I fixed that hunk to this

	 if (!irb->mt->hiz_region) {
	    intel_miptree_release(&irb->mt);
	    return false;
	 }

and now accept your rb.

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