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.vers...@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 <e...@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.vers...@linux.intel.com
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
