[Mesa-dev] [PATCH 15/41] intel: Refactor intel_render_texture()

[Eric Anholt]

On Thu, 17 Nov 2011 19:58:42 -0800, Chad Versace <chad.versace at linux.intel.com> wrote:
> This is in preparation for properly implementing glFramebufferTexture*()
> for mipmapped depthstencil textures. The FIXME comments deleted by this
> patch give a rough explanation of what was broken.
> 
> This refactor does the following:
>    - In intel_update_wrapper() and intel_wrap_texture(), prepare to
>      replace the 'att' parameter with a miptree.
>    - Move the call to intel_renderbuffer_set_draw_offsets() from
>      intel_render_texture() into intel_udpate_wrapper().
> 
> Each time I encounter those functions, I dislike their vague names.
> (Update which wrapper? What is wrapped? What is the wrapper?). So, while
> I was mucking around, I also renamed the functions.
> 
> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> ---
>  src/mesa/drivers/dri/intel/intel_fbo.c |  113 +++++++++++++++++++++++---------
>  1 files changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/intel/intel_fbo.c b/src/mesa/drivers/dri/intel/intel_fbo.c
> index 8e4f7a9..a61c74c 100644
> --- a/src/mesa/drivers/dri/intel/intel_fbo.c
> +++ b/src/mesa/drivers/dri/intel/intel_fbo.c
> @@ -945,41 +945,54 @@ intel_framebuffer_renderbuffer(struct gl_context * ctx,
>     intel_draw_buffer(ctx);
>  }
>  
> +/**
> + * NOTE: The 'att' parameter is a kludge that will soon be removed. Its
> + * presence allows us to refactor the wrapping of depthstencil textures that
> + * use separate stencil in two easily manageable steps, rather than in one
> + * large, hairy step. First, refactor the common wrapping code used by all
> + * texture formats. Second, refactor the separate stencil code paths.
> + */
>  static bool
> -intel_update_wrapper(struct gl_context *ctx, struct intel_renderbuffer *irb, 
> -		     struct gl_renderbuffer_attachment *att)
> +intel_renderbuffer_update_wrapper(struct intel_context *intel,
> +                                  struct intel_renderbuffer *irb,
> +                                  struct intel_mipmap_tree *mt,
> +                                  uint32_t level,
> +                                  uint32_t layer,
> +                                  GLenum internal_format,
> +                                  struct gl_renderbuffer_attachment *att)
>  {
> +   struct gl_context *ctx = &intel->ctx;
> +   struct gl_renderbuffer *rb = &irb->Base;
> +
> +   /* The image variables are a kludge. See the note above for the att
> +    * parameter.
> +    */
>     struct gl_texture_image *texImage = _mesa_get_attachment_teximage(att);
>     struct intel_texture_image *intel_image = intel_texture_image(texImage);
> -   int width, height, depth;
>  
> -   if (!intel_span_supports_format(texImage->TexFormat)) {
> +   irb->Base.Format = ctx->Driver.ChooseTextureFormat(ctx, internal_format,
> +                                                      GL_NONE, GL_NONE);
> +
> +   if (!intel_span_supports_format(rb->Format)) {
>        DBG("Render to texture BAD FORMAT %s\n",
> -	  _mesa_get_format_name(texImage->TexFormat));
> +	  _mesa_get_format_name(rb->Format));
>        return false;
>     } else {
> -      DBG("Render to texture %s\n", _mesa_get_format_name(texImage->TexFormat));
> +      DBG("Render to texture %s\n", _mesa_get_format_name(rb->Format));
>     }
>  
> -   intel_miptree_get_dimensions_for_image(texImage, &width, &height, &depth);
> -
> -   irb->Base.Format = texImage->TexFormat;
> -   irb->Base.DataType = intel_mesa_format_to_rb_datatype(texImage->TexFormat);
> -   irb->Base.InternalFormat = texImage->InternalFormat;
> -   irb->Base._BaseFormat = _mesa_base_tex_format(ctx, irb->Base.InternalFormat);
> -   irb->Base.Width = width;
> -   irb->Base.Height = height;
> +   rb->InternalFormat = internal_format;
> +   rb->DataType = intel_mesa_format_to_rb_datatype(rb->Format);
> +   rb->_BaseFormat = _mesa_get_format_base_format(rb->Format);
> +   rb->Width = mt->level[level].width;
> +   rb->Height = mt->level[level].height;
>  
>     irb->Base.Delete = intel_delete_renderbuffer;
>     irb->Base.AllocStorage = intel_nop_alloc_storage;
>  
> -   irb->mt_level = att->TextureLevel;
> -   if (att->CubeMapFace > 0) {
> -      assert(att->Zoffset == 0);
> -      irb->mt_layer = att->CubeMapFace;
> -   } else {
> -      irb->mt_layer= att->Zoffset;
> -   }
> +   intel_miptree_check_level_layer(mt, level, layer);
> +   irb->mt_level = level;
> +   irb->mt_layer = layer;
>  
>     if (intel_image->stencil_rb) {
>        /*  The tex image has packed depth/stencil format, but is using separate
> @@ -1002,29 +1015,46 @@ intel_update_wrapper(struct gl_context *ctx, struct intel_renderbuffer *irb,
>        depth_irb = intel_renderbuffer(intel_image->depth_rb);
>        depth_irb->mt_level = irb->mt_level;
>        depth_irb->mt_layer = irb->mt_layer;
> +      intel_renderbuffer_set_draw_offset(depth_irb);
>  
>        stencil_irb = intel_renderbuffer(intel_image->stencil_rb);
>        stencil_irb->mt_level = irb->mt_level;
>        stencil_irb->mt_layer = irb->mt_layer;
> +      intel_renderbuffer_set_draw_offset(stencil_irb);
>     } else {
>        intel_miptree_reference(&irb->mt, intel_image->mt);
> +      intel_renderbuffer_set_draw_offset(irb);
>     }
> +
>     return true;
>  }
>  
>  /**
> - * When glFramebufferTexture[123]D is called this function sets up the
> - * gl_renderbuffer wrapper around the texture image.
> - * This will have the region info needed for hardware rendering.
> + * \brief Wrap a renderbuffer around a single slice of a miptree.
> + *
> + * Called by glFramebufferTexture*().
> + *
> + * NOTE: The 'att' parameter is a kludge that will soon be removed. Its
> + * presence allows us to refactor the wrapping of depthstencil textures that
> + * use separate stencil in two easily manageable steps, rather than in one
> + * large, hairy step. First, refactor the common wrapping code used by all
> + * texture formats. Second, refactor the separate stencil code paths.
>   */
> -static struct intel_renderbuffer *
> -intel_wrap_texture(struct gl_context * ctx,
> -		   struct gl_renderbuffer_attachment *att)
> +static struct intel_renderbuffer*
> +intel_renderbuffer_wrap_miptree(struct intel_context *intel,
> +                                struct intel_mipmap_tree *mt,
> +                                uint32_t level,
> +                                uint32_t layer,
> +                                GLenum internal_format,
> +                                struct gl_renderbuffer_attachment *att)
> +

I don't think this should be a separate function.  I think the split
between it and intel_update_wrapper() is just leftover mess from long
ago before framebuffer completeness was worked out, and we would really
use the _swrast_render_texture ptahs.  The swrast path should be not
happening today, since the renderbuffer won't be accessed because the
FBO will be incomplete.

I think folding those two together and not calling intel_update_wrapper
twice instead of this patch would clean this code up more.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111118/43cbfb78/attachment-0001.pgp>