[Mesa-dev] [PATCH 5/6] mesa: Make Mesa core set up wrapped texture renderbuffer state.

[Kenneth Graunke]

On 04/30/2013 12:56 PM, Eric Anholt wrote:
> Everyone was doing effectively the same thing, except for some funky code
> reuse in Intel, and swrast mistakenly recomputing _BaseFormat instead of
> using the texture's _BaseFormat.  swrast's sRGB handling is left in place,
> though it should be done by using _mesa_get_render_format() at render time
> instead (as-is, it will miss updates to GL_FRAMEBUFFER_SRGB).
> ---
>   src/mesa/drivers/dri/intel/intel_fbo.c     |  6 ------
>   src/mesa/drivers/dri/nouveau/nouveau_fbo.c | 18 ------------------
>   src/mesa/main/fbobject.c                   |  7 +++++++
>   src/mesa/state_tracker/st_cb_fbo.c         |  5 -----
>   src/mesa/swrast/s_texrender.c              |  5 -----
>   5 files changed, 7 insertions(+), 34 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_fbo.c b/src/mesa/drivers/dri/intel/intel_fbo.c
> index a3817eb..f037445 100644
> --- a/src/mesa/drivers/dri/intel/intel_fbo.c
> +++ b/src/mesa/drivers/dri/intel/intel_fbo.c
> @@ -489,12 +489,6 @@ intel_renderbuffer_update_wrapper(struct intel_context *intel,
>      struct intel_mipmap_tree *mt = intel_image->mt;
>      int level = image->Level;
>
> -   rb->Format = image->TexFormat;
> -   rb->InternalFormat = image->InternalFormat;
> -   rb->_BaseFormat = image->_BaseFormat;
> -   rb->NumSamples = mt->num_samples;
> -   rb->Width = image->Width2;
> -   rb->Height = image->Height2;
>      rb->Delete = intel_delete_renderbuffer;
>      rb->AllocStorage = intel_nop_alloc_storage;
>
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> index adead3d..a692051 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> @@ -247,21 +247,6 @@ nouveau_framebuffer_renderbuffer(struct gl_context *ctx, struct gl_framebuffer *
>   	context_dirty(ctx, FRAMEBUFFER);
>   }
>
> -static GLenum
> -get_tex_format(struct gl_texture_image *ti)
> -{
> -	switch (ti->TexFormat) {
> -	case MESA_FORMAT_ARGB8888:
> -		return GL_RGBA8;
> -	case MESA_FORMAT_XRGB8888:
> -		return GL_RGB8;
> -	case MESA_FORMAT_RGB565:
> -		return GL_RGB5;
> -	default:
> -		return GL_NONE;
> -	}
> -}
> -
>   static void
>   nouveau_render_texture(struct gl_context *ctx, struct gl_framebuffer *fb,
>   		       struct gl_renderbuffer_attachment *att)
> @@ -271,9 +256,6 @@ nouveau_render_texture(struct gl_context *ctx, struct gl_framebuffer *fb,
>   		att->Texture->Image[att->CubeMapFace][att->TextureLevel];
>
>   	/* Update the renderbuffer fields from the texture. */
> -	set_renderbuffer_format(rb, get_tex_format(ti));

I think the call to set_renderbuffer_format is still necessary, since it 
does:

struct nouveau_surface *s = &to_nouveau_renderbuffer(rb)->surface;
s->cpp = ...;

and the core Mesa code doesn't do that.  Otherwise, this looks good.

I looked over the series for about an hour and didn't notice any other 
large issues.  I'm really glad to see this cleaned up - it was really 
awkward having the core Mesa code rely on driver hooks setting up the 
renderbuffer wrapper.

For the series (assuming this nouveau thing gets fixed):
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> -	rb->Width = ti->Width;
> -	rb->Height = ti->Height;
>   	nouveau_surface_ref(&to_nouveau_teximage(ti)->surface,
>   			    &to_nouveau_renderbuffer(rb)->surface);
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 26d1cce..d88c062 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -380,6 +380,13 @@ _mesa_update_texture_renderbuffer(struct gl_context *ctx,
>         rb->AllocStorage = NULL;
>      }
>
> +   rb->_BaseFormat = texImage->_BaseFormat;
> +   rb->Format = texImage->TexFormat;
> +   rb->InternalFormat = texImage->InternalFormat;
> +   rb->Width = texImage->Width2;
> +   rb->Height = texImage->Height2;
> +   rb->NumSamples = texImage->NumSamples;
> +
>      ctx->Driver.RenderTexture(ctx, fb, att);
>   }
>
> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
> index affe656..aa245d3 100644
> --- a/src/mesa/state_tracker/st_cb_fbo.c
> +++ b/src/mesa/state_tracker/st_cb_fbo.c
> @@ -414,11 +414,6 @@ st_render_texture(struct gl_context *ctx,
>      strb->rtt_level = att->TextureLevel;
>      strb->rtt_face = att->CubeMapFace;
>      strb->rtt_slice = att->Zoffset;
> -   rb->NumSamples = texImage->NumSamples;
> -   rb->Width = texImage->Width2;
> -   rb->Height = texImage->Height2;
> -   rb->_BaseFormat = texImage->_BaseFormat;
> -   rb->InternalFormat = texImage->InternalFormat;
>
>      pipe_resource_reference( &strb->texture, pt );
>
> diff --git a/src/mesa/swrast/s_texrender.c b/src/mesa/swrast/s_texrender.c
> index f56a0d5..00b3ca5 100644
> --- a/src/mesa/swrast/s_texrender.c
> +++ b/src/mesa/swrast/s_texrender.c
> @@ -50,11 +50,6 @@ update_wrapper(struct gl_context *ctx, struct gl_renderbuffer_attachment *att)
>         zOffset = att->Zoffset;
>      }
>
> -   rb->Width = swImage->Base.Width;
> -   rb->Height = swImage->Base.Height;
> -   rb->InternalFormat = swImage->Base.InternalFormat;
> -   rb->_BaseFormat = _mesa_get_format_base_format(format);
> -
>      /* Want to store linear values, not sRGB */
>      rb->Format = _mesa_get_srgb_format_linear(format);
>
>