[Mesa-dev] [PATCH 3/3] i965: Fix format of private renderbuffers

[ville.syrjala at linux.intel.com]

From: Ville Syrjälä <ville.syrjala at linux.intel.com>

intel_alloc_renderbuffer_storage() will clobber rb->Format which was
already set up by intel_create_renderbuffer(). This causes the driver
to potentially create the depth buffer in the wrong format.

In practice this makes the depth buffer Z24 even if the visual has
depthBits==16.

The incorrect depth buffer format doesn't seem to cause any actual
problems in i965, but it seems like we should fix it anyway. I see
Z16 has been more or less deprecated in the driver except the for
the depthBits==16 case. But if we want to use Z24 even in that
case (not sure it's really legal?) it would look better if the
code made that decision explicitly rather than relying on the
format to get magically overwritten by the renderbuffer code.

Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
---
 src/mesa/drivers/dri/i965/intel_fbo.c | 52 ++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
index f58d7c8..e6e35fe 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -260,20 +260,10 @@ intel_quantize_num_samples(struct intel_screen *intel, unsigned num_samples)
    return quantized_samples;
 }
 
-
-/**
- * Called via glRenderbufferStorageEXT() to set the format and allocate
- * storage for a user-created renderbuffer.
- */
-static GLboolean
-intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer *rb,
-                                 GLenum internalFormat,
-                                 GLuint width, GLuint height)
+static mesa_format
+intel_renderbuffer_format(struct gl_context * ctx, GLenum internalFormat)
 {
    struct brw_context *brw = brw_context(ctx);
-   struct intel_screen *screen = brw->intelScreen;
-   struct intel_renderbuffer *irb = intel_renderbuffer(rb);
-   rb->NumSamples = intel_quantize_num_samples(screen, rb->NumSamples);
 
    switch (internalFormat) {
    default:
@@ -282,9 +272,9 @@ intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer
        * except they're less useful because you can't texture with
        * them.
        */
-      rb->Format = ctx->Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
-                                                   internalFormat,
-                                                   GL_NONE, GL_NONE);
+      return ctx->Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
+                                             internalFormat,
+                                             GL_NONE, GL_NONE);
       break;
    case GL_STENCIL_INDEX:
    case GL_STENCIL_INDEX1_EXT:
@@ -293,14 +283,26 @@ intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer
    case GL_STENCIL_INDEX16_EXT:
       /* These aren't actual texture formats, so force them here. */
       if (brw->has_separate_stencil) {
-	 rb->Format = MESA_FORMAT_S_UINT8;
+	 return MESA_FORMAT_S_UINT8;
       } else {
 	 assert(!brw->must_use_separate_stencil);
-	 rb->Format = MESA_FORMAT_Z24_UNORM_S8_UINT;
+	 return MESA_FORMAT_Z24_UNORM_S8_UINT;
       }
-      break;
    }
+}
+
+static GLboolean
+intel_alloc_private_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer *rb,
+                                         GLenum internalFormat,
+                                         GLuint width, GLuint height)
+{
+   struct brw_context *brw = brw_context(ctx);
+   struct intel_screen *screen = brw->intelScreen;
+   struct intel_renderbuffer *irb = intel_renderbuffer(rb);
 
+   assert(rb->Format != MESA_FORMAT_NONE);
+
+   rb->NumSamples = intel_quantize_num_samples(screen, rb->NumSamples);
    rb->Width = width;
    rb->Height = height;
    rb->_BaseFormat = _mesa_base_fbo_format(ctx, internalFormat);
@@ -325,6 +327,18 @@ intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer
    return true;
 }
 
+/**
+ * Called via glRenderbufferStorageEXT() to set the format and allocate
+ * storage for a user-created renderbuffer.
+ */
+static GLboolean
+intel_alloc_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer *rb,
+                                 GLenum internalFormat,
+                                 GLuint width, GLuint height)
+{
+   rb->Format = intel_renderbuffer_format(ctx, internalFormat);
+   return intel_alloc_private_renderbuffer_storage(ctx, rb, internalFormat, width, height);
+}
 
 static void
 intel_image_target_renderbuffer_storage(struct gl_context *ctx,
@@ -469,7 +483,7 @@ intel_create_private_renderbuffer(mesa_format format, unsigned num_samples)
    struct intel_renderbuffer *irb;
 
    irb = intel_create_renderbuffer(format, num_samples);
-   irb->Base.Base.AllocStorage = intel_alloc_renderbuffer_storage;
+   irb->Base.Base.AllocStorage = intel_alloc_private_renderbuffer_storage;
 
    return irb;
 }
-- 
1.8.3.2