[Mesa-dev] [PATCH 1/3] i915: 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.

Long time ago things worked by accident because
_mesa_choose_tex_format() checked for ARB_depth_texture
and thus returned MESA_FORMAT_NONE on gen2 hardware. Somehow
that ended up working when depthBits==16 because the driver
would then pick DEPTH_FRMT_16_FIXED. Not sure how, but things
also seemed to work with depthBits==24.

Things started to go more sideways at:
 commit 6ae473221a53d8bcb584021483c5328797c6b67c
 Author: Eric Anholt <eric at anholt.net>
 Date:   Mon Apr 22 16:04:25 2013 -0700

    intel: Fold the one last function intel_tex_format.c into the caller.

since that caused intel_miptree_create_layout() to divide by zero
when encoutering MESA_FORMAT_NONE (bw==0). So after this
commit things were broken enough that many applications wouldn't even
run.

Things got a bit better at:
 commit c245efe7e8247ba0c845dee7b77e63fdbfc7e1b3
 Author: Eric Anholt <eric at anholt.net>
 Date:   Thu Mar 21 09:50:45 2013 -0700

    mesa: Remove extension checking from ChooseTexFormat.

since now _mesa_choose_tex_format() would return MESA_FORMAT_X8_Z24
for GL_DEPTH_COMPONENT due to i915 erroneosly claiming that
MESA_FORMAT_X8_S24 (and others) are supported texture formats even
on gen2 hardware. So now the the div-by-zero was gone, but now the
driver would pick DEPTH_FRMT_24_FIXED_8_OTHER even when
depthBits==16 which caused rendering problems.

If we prevent rb->Format from getting clobbered for the depth buffer
things work much better. This makes the spinning title text visible
again in chromium-bsu at 16bpp, for example.

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

diff --git a/src/mesa/drivers/dri/i915/intel_fbo.c b/src/mesa/drivers/dri/i915/intel_fbo.c
index a806a42..2538fac 100644
--- a/src/mesa/drivers/dri/i915/intel_fbo.c
+++ b/src/mesa/drivers/dri/i915/intel_fbo.c
@@ -165,17 +165,10 @@ intel_unmap_renderbuffer(struct gl_context *ctx,
    intel_miptree_unmap(intel, irb->mt, irb->mt_level, irb->mt_layer);
 }
 
-/**
- * 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 intel_context *intel = intel_context(ctx);
-   struct intel_renderbuffer *irb = intel_renderbuffer(rb);
 
    switch (internalFormat) {
    default:
@@ -184,19 +177,28 @@ 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 = intel->ctx.Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
-							 internalFormat,
-							 GL_NONE, GL_NONE);
-      break;
+      return intel->ctx.Driver.ChooseTextureFormat(ctx, GL_TEXTURE_2D,
+                                                   internalFormat,
+                                                   GL_NONE, GL_NONE);
    case GL_STENCIL_INDEX:
    case GL_STENCIL_INDEX1_EXT:
    case GL_STENCIL_INDEX4_EXT:
    case GL_STENCIL_INDEX8_EXT:
    case GL_STENCIL_INDEX16_EXT:
       /* These aren't actual texture formats, so force them here. */
-      rb->Format = MESA_FORMAT_Z24_UNORM_S8_UINT;
-      break;
+      return MESA_FORMAT_Z24_UNORM_S8_UINT;
    }
+}
+
+static GLboolean
+intel_alloc_private_renderbuffer_storage(struct gl_context * ctx, struct gl_renderbuffer *rb,
+                                         GLenum internalFormat,
+                                         GLuint width, GLuint height)
+{
+   struct intel_context *intel = intel_context(ctx);
+   struct intel_renderbuffer *irb = intel_renderbuffer(rb);
+
+   assert(rb->Format != MESA_FORMAT_NONE);
 
    rb->Width = width;
    rb->Height = height;
@@ -219,6 +221,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,
@@ -343,7 +357,7 @@ intel_create_private_renderbuffer(mesa_format format)
    struct intel_renderbuffer *irb;
 
    irb = intel_create_renderbuffer(format);
-   irb->Base.Base.AllocStorage = intel_alloc_renderbuffer_storage;
+   irb->Base.Base.AllocStorage = intel_alloc_private_renderbuffer_storage;
 
    return irb;
 }
-- 
1.8.3.2