Re: [Mesa-dev] [PATCH 08/41] intel: Replace intel_renderbuffer::region with a miptree

[Eric Anholt]

On Thu, 17 Nov 2011 19:58:35 -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.
> 
> Signed-off-by: Chad Versace <chad.vers...@linux.intel.com>

In case I get distracted before finishing, patches 1-7 are

Reviewed-by: Eric Anholt <e...@anholt.net>

> ---
> @@ -1381,9 +1396,12 @@ intel_process_dri2_buffer_with_separate_stencil(struct 
> intel_context *intel,
>                                   buffer_name);
>  
>     if (buffer->attachment == __DRI_BUFFER_HIZ) {
> -      intel_region_reference(&rb->hiz_region, region);
> +      intel_region_reference(&rb->mt->hiz_region, region);
>     } else {
> -      intel_region_reference(&rb->region, region);
> +      rb->mt = intel_miptree_create_for_region(intel,
> +                                               GL_TEXTURE_2D,
> +                                               rb->Base.Format,
> +                                               region);;
>     }

Leaked old rb->mt here?  I don't see this function kicking off with an
intel_miptree_release(&rb->mt).  Next quoted hunk has the same issue.

>     if (buffer->attachment == __DRI_BUFFER_HIZ) {
> -      intel_region_reference(&rb->hiz_region, region);
> +      intel_region_reference(&rb->mt->hiz_region, region);
>     } else {
> -      intel_region_reference(&rb->region, region);
> +      rb->mt = intel_miptree_create_for_region(intel,
> +                                               GL_TEXTURE_2D,
> +                                               rb->Base.Format,
> +                                               region);;

extra ';'

> -      intel_region_reference(&intel_get_renderbuffer(fb, 
> BUFFER_DEPTH)->region,
> -                             region);
> -      intel_region_reference(&intel_get_renderbuffer(fb, 
> BUFFER_STENCIL)->region,
> -                             region);
> +      struct intel_mipmap_tree *mt =
> +            intel_miptree_create_for_region(intel,
> +                                            GL_TEXTURE_2D,
> +                                            depth_stencil_rb->Base.Format,
> +                                            region);
>        intel_region_release(&region);
> +      intel_miptree_reference(&intel_get_renderbuffer(fb, BUFFER_DEPTH)->mt, 
> mt);
> +      intel_miptree_reference(&intel_get_renderbuffer(fb, 
> BUFFER_STENCIL)->mt, mt);

Haven't you leaked a mt reference here?
>     }
>  
>     irb = intel_renderbuffer(rb);
> -   intel_region_reference(&irb->region, image->region);
> +   irb->mt = intel_miptree_create_for_region(intel,
> +                                             GL_TEXTURE_2D,
> +                                             image->format,
> +                                             image->region);
> +   if (!irb->mt)
> +      return;

Leak of existing irb->mt?

pgpCCVaLQKkCc.pgp
Description: PGP signature

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev