Hi Brian, Keith, Zack et al.

So I tried to get i915g running again and it looks like there is a
RGBA vs BGRA bug in the draw module. I have attached two patches that
I would like to have some comments on before commit.

Patch 1 removes a bunch of switch cases that was strew all over the
draw module that all pretty much did exactly the same thing. Which
made it hard to fix the RGBA issue for i915g. In draw_pipe_vbuf and
draw_pt_emit the format for EMIT_4UB was one thing and in
draw_pt_fetch_emit it was another. In light of the format clean up I
standardized on following the same as the float formats for EMIT_4UB.

Patch 2 then introduces EMIT_4UB_BGRA and is used by i915g to make the
colors look correct again.

Comments please.

Cheers Jakob.
From 25fd65438993cf0b1e1f0846a9f94f970280cb32 Mon Sep 17 00:00:00 2001
From: Jakob Bornecrantz <wallbra...@gmail.com>
Date: Thu, 25 Mar 2010 00:18:30 +0100
Subject: [PATCH] draw: Use translate function instead of switch cases

---
 src/gallium/auxiliary/draw/draw_pipe_vbuf.c        |   37 +++--------------
 src/gallium/auxiliary/draw/draw_pt_emit.c          |   37 +++--------------
 src/gallium/auxiliary/draw/draw_pt_fetch_emit.c    |   42 +++++---------------
 .../auxiliary/draw/draw_pt_fetch_shade_emit.c      |   29 ++------------
 src/gallium/auxiliary/draw/draw_vertex.c           |   32 ++++-----------
 src/gallium/auxiliary/draw/draw_vertex.h           |   18 ++++++++
 6 files changed, 53 insertions(+), 142 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_pipe_vbuf.c b/src/gallium/auxiliary/draw/draw_pipe_vbuf.c
index 2709957..69e3304 100644
--- a/src/gallium/auxiliary/draw/draw_pipe_vbuf.c
+++ b/src/gallium/auxiliary/draw/draw_pipe_vbuf.c
@@ -238,38 +238,15 @@ vbuf_start_prim( struct vbuf_stage *vbuf, uint prim )
       unsigned output_format;
       unsigned src_offset = (vbuf->vinfo->attrib[i].src_index * 4 * sizeof(float) );
 
-      switch (vbuf->vinfo->attrib[i].emit) {
-      case EMIT_4F:
-	 output_format = PIPE_FORMAT_R32G32B32A32_FLOAT;
-	 emit_sz = 4 * sizeof(float);
-	 break;
-      case EMIT_3F:
-	 output_format = PIPE_FORMAT_R32G32B32_FLOAT;
-	 emit_sz = 3 * sizeof(float);
-	 break;
-      case EMIT_2F:
-	 output_format = PIPE_FORMAT_R32G32_FLOAT;
-	 emit_sz = 2 * sizeof(float);
-	 break;
-      case EMIT_1F:
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
-	 break;
-      case EMIT_1F_PSIZE:
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
+      output_format = draw_translate_vinfo_format(vbuf->vinfo->attrib[i].emit);
+      emit_sz = draw_translate_vinfo_format_size(vbuf->vinfo->attrib[i].emit);
+
+      /* doesn't handle EMIT_OMIT or unknown */
+      assert(emit_sz != 0);
+
+      if (vbuf->vinfo->attrib[i].emit == EMIT_1F_PSIZE) {
 	 src_buffer = 1;
 	 src_offset = 0;
-	 break;
-      case EMIT_4UB:
-	 output_format = PIPE_FORMAT_A8R8G8B8_UNORM;
-	 emit_sz = 4 * sizeof(ubyte);
-         break;
-      default:
-	 assert(0);
-	 output_format = PIPE_FORMAT_NONE;
-	 emit_sz = 0;
-	 break;
       }
 
       hw_key.element[i].type = TRANSLATE_ELEMENT_NORMAL;
diff --git a/src/gallium/auxiliary/draw/draw_pt_emit.c b/src/gallium/auxiliary/draw/draw_pt_emit.c
index ae357b5..56fab1c 100644
--- a/src/gallium/auxiliary/draw/draw_pt_emit.c
+++ b/src/gallium/auxiliary/draw/draw_pt_emit.c
@@ -86,40 +86,15 @@ void draw_pt_emit_prepare( struct pt_emit *emit,
       unsigned output_format;
       unsigned src_offset = (vinfo->attrib[i].src_index * 4 * sizeof(float) );
 
+      output_format = draw_translate_vinfo_format(vinfo->attrib[i].emit);
+      emit_sz = draw_translate_vinfo_format_size(vinfo->attrib[i].emit);
 
-         
-      switch (vinfo->attrib[i].emit) {
-      case EMIT_4F:
-	 output_format = PIPE_FORMAT_R32G32B32A32_FLOAT;
-	 emit_sz = 4 * sizeof(float);
-	 break;
-      case EMIT_3F:
-	 output_format = PIPE_FORMAT_R32G32B32_FLOAT;
-	 emit_sz = 3 * sizeof(float);
-	 break;
-      case EMIT_2F:
-	 output_format = PIPE_FORMAT_R32G32_FLOAT;
-	 emit_sz = 2 * sizeof(float);
-	 break;
-      case EMIT_1F:
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
-	 break;
-      case EMIT_1F_PSIZE:
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
+      /* doesn't handle EMIT_OMIT or unknown */
+      assert(emit_sz != 0);
+
+      if (vinfo->attrib[i].emit == EMIT_1F_PSIZE) {
 	 src_buffer = 1;
 	 src_offset = 0;
-	 break;
-      case EMIT_4UB:
-	 output_format = PIPE_FORMAT_A8R8G8B8_UNORM;
-	 emit_sz = 4 * sizeof(ubyte);
-         break;
-      default:
-	 assert(0);
-	 output_format = PIPE_FORMAT_NONE;
-	 emit_sz = 0;
-	 break;
       }
 
       hw_key.element[i].type = TRANSLATE_ELEMENT_NORMAL;
diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_emit.c b/src/gallium/auxiliary/draw/draw_pt_fetch_emit.c
index 2a60447..4c8086f 100644
--- a/src/gallium/auxiliary/draw/draw_pt_fetch_emit.c
+++ b/src/gallium/auxiliary/draw/draw_pt_fetch_emit.c
@@ -129,41 +129,19 @@ static void fetch_emit_prepare( struct draw_pt_middle_end *middle,
       unsigned input_offset = src->src_offset;
       unsigned output_format;
 
-      switch (vinfo->attrib[i].emit) {
-      case EMIT_4UB:
-	 output_format = PIPE_FORMAT_R8G8B8A8_UNORM;
-	 emit_sz = 4 * sizeof(unsigned char);
-         break;
-      case EMIT_4F:
-	 output_format = PIPE_FORMAT_R32G32B32A32_FLOAT;
-	 emit_sz = 4 * sizeof(float);
-         break;
-      case EMIT_3F:
-	 output_format = PIPE_FORMAT_R32G32B32_FLOAT;
-	 emit_sz = 3 * sizeof(float);
-         break;
-      case EMIT_2F:
-	 output_format = PIPE_FORMAT_R32G32_FLOAT;
-	 emit_sz = 2 * sizeof(float);
-         break;
-      case EMIT_1F:
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
-         break;
-      case EMIT_1F_PSIZE:
+      output_format = draw_translate_vinfo_format(vinfo->attrib[i].emit);
+      emit_sz = draw_translate_vinfo_format_size(vinfo->attrib[i].emit);
+
+      if (vinfo->attrib[i].emit == EMIT_OMIT)
+	 continue;
+
+      /* doesn't handle unknown */
+      assert(emit_sz != 0);
+
+      if (vinfo->attrib[i].emit == EMIT_1F_PSIZE) {
 	 input_format = PIPE_FORMAT_R32_FLOAT;
 	 input_buffer = draw->pt.nr_vertex_buffers;
 	 input_offset = 0;
-	 output_format = PIPE_FORMAT_R32_FLOAT;
-	 emit_sz = 1 * sizeof(float);
-         break;
-      case EMIT_OMIT:
-         continue;
-      default:
-         assert(0);
-	 output_format = PIPE_FORMAT_NONE;
-	 emit_sz = 0;
-	 continue;
       }
 
       key.element[i].type = TRANSLATE_ELEMENT_NORMAL;
diff --git a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_emit.c b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_emit.c
index 1aecb51..18f3c42 100644
--- a/src/gallium/auxiliary/draw/draw_pt_fetch_shade_emit.c
+++ b/src/gallium/auxiliary/draw/draw_pt_fetch_shade_emit.c
@@ -130,31 +130,10 @@ static void fse_prepare( struct draw_pt_middle_end *middle,
       unsigned dst_offset = 0;
 
       for (i = 0; i < vinfo->num_attribs; i++) {
-         unsigned emit_sz = 0;
-
-         switch (vinfo->attrib[i].emit) {
-         case EMIT_4F:
-            emit_sz = 4 * sizeof(float);
-            break;
-         case EMIT_3F:
-            emit_sz = 3 * sizeof(float);
-            break;
-         case EMIT_2F:
-            emit_sz = 2 * sizeof(float);
-            break;
-         case EMIT_1F:
-            emit_sz = 1 * sizeof(float);
-            break;
-         case EMIT_1F_PSIZE:
-            emit_sz = 1 * sizeof(float);
-            break;
-         case EMIT_4UB:
-            emit_sz = 4 * sizeof(ubyte);
-            break;
-         default:
-            assert(0);
-            break;
-         }
+         unsigned emit_sz = draw_translate_vinfo_format_size(vinfo->attrib[i].emit);
+
+         /* doesn't handle EMIT_OMIT or any unknown formats */
+         assert(emit_sz != 0);
 
          /* The elements in the key correspond to vertex shader output
           * numbers, not to positions in the hw vertex description --
diff --git a/src/gallium/auxiliary/draw/draw_vertex.c b/src/gallium/auxiliary/draw/draw_vertex.c
index 3214213..6699889 100644
--- a/src/gallium/auxiliary/draw/draw_vertex.c
+++ b/src/gallium/auxiliary/draw/draw_vertex.c
@@ -48,30 +48,14 @@ draw_compute_vertex_size(struct vertex_info *vinfo)
    uint i;
 
    vinfo->size = 0;
-   for (i = 0; i < vinfo->num_attribs; i++) {
-      switch (vinfo->attrib[i].emit) {
-      case EMIT_OMIT:
-         break;
-      case EMIT_4UB:
-         /* fall-through */
-      case EMIT_1F_PSIZE:
-         /* fall-through */
-      case EMIT_1F:
-         vinfo->size += 1;
-         break;
-      case EMIT_2F:
-         vinfo->size += 2;
-         break;
-      case EMIT_3F:
-         vinfo->size += 3;
-         break;
-      case EMIT_4F:
-         vinfo->size += 4;
-         break;
-      default:
-         assert(0);
-      }
-   }
+   for (i = 0; i < vinfo->num_attribs; i++)
+      vinfo->size += draw_translate_vinfo_format_size(vinfo->attrib[i].emit);
+
+   /* XXX check for emit out of bounds */
+
+   assert(vinfo->size % 4 == 0);
+   /* in dwords */
+   vinfo->size /= 4;
 }
 
 
diff --git a/src/gallium/auxiliary/draw/draw_vertex.h b/src/gallium/auxiliary/draw/draw_vertex.h
index 8c3c7be..f890f61 100644
--- a/src/gallium/auxiliary/draw/draw_vertex.h
+++ b/src/gallium/auxiliary/draw/draw_vertex.h
@@ -160,5 +160,23 @@ static INLINE unsigned draw_translate_vinfo_format(unsigned format )
    }
 }
 
+static INLINE unsigned draw_translate_vinfo_format_size(unsigned format)
+{
+   switch (format) {
+   case EMIT_1F:
+   case EMIT_1F_PSIZE:
+      return 1 * sizeof(float);
+   case EMIT_2F:
+      return 2 * sizeof(float);
+   case EMIT_3F:
+      return 3 * sizeof(float);
+   case EMIT_4F:
+      return 4 * sizeof(float);
+   case EMIT_4UB:
+      return 4 * sizeof(unsigned char);
+   default:
+      return 0;
+   }
+}
 
 #endif /* DRAW_VERTEX_H */
-- 
1.6.0.4

From f30622102f0abb619ecf1951cef7c18df0f3835b Mon Sep 17 00:00:00 2001
From: Jakob Bornecrantz <wallbra...@gmail.com>
Date: Thu, 25 Mar 2010 13:45:42 +0100
Subject: [PATCH] draw: Add EMIT_4UB_BGRA format

Needed for i915g, also fixed swizzle in draw_vs_aos_io.
---
 src/gallium/auxiliary/draw/draw_vertex.c      |    7 +++++++
 src/gallium/auxiliary/draw/draw_vertex.h      |    7 ++++++-
 src/gallium/auxiliary/draw/draw_vs_aos_io.c   |   12 +++++-------
 src/gallium/drivers/i915/i915_prim_emit.c     |    7 +++++++
 src/gallium/drivers/i915/i915_state_derived.c |    4 ++--
 src/gallium/drivers/nvfx/nvfx_draw.c          |    6 ++++++
 6 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/gallium/auxiliary/draw/draw_vertex.c b/src/gallium/auxiliary/draw/draw_vertex.c
index 6699889..0d72a56 100644
--- a/src/gallium/auxiliary/draw/draw_vertex.c
+++ b/src/gallium/auxiliary/draw/draw_vertex.c
@@ -104,6 +104,13 @@ draw_dump_emitted_vertex(const struct vertex_info *vinfo, const uint8_t *data)
          debug_printf("%u ", *data++);
          debug_printf("%u ", *data++);
          break;
+      case EMIT_4UB_BGRA:
+         debug_printf("EMIT_4UB_BGRA:\t");
+         debug_printf("%u ", *data++);
+         debug_printf("%u ", *data++);
+         debug_printf("%u ", *data++);
+         debug_printf("%u ", *data++);
+         break;
       default:
          assert(0);
       }
diff --git a/src/gallium/auxiliary/draw/draw_vertex.h b/src/gallium/auxiliary/draw/draw_vertex.h
index f890f61..f962b54 100644
--- a/src/gallium/auxiliary/draw/draw_vertex.h
+++ b/src/gallium/auxiliary/draw/draw_vertex.h
@@ -54,7 +54,8 @@ enum attrib_emit {
    EMIT_2F,
    EMIT_3F,
    EMIT_4F,
-   EMIT_4UB  /**< XXX may need variations for RGBA vs BGRA, etc */
+   EMIT_4UB, /**< is RGBA like the rest */
+   EMIT_4UB_BGRA
 };
 
 
@@ -155,6 +156,8 @@ static INLINE unsigned draw_translate_vinfo_format(unsigned format )
       return PIPE_FORMAT_R32G32B32A32_FLOAT;
    case EMIT_4UB:
       return PIPE_FORMAT_R8G8B8A8_UNORM;
+   case EMIT_4UB_BGRA:
+      return PIPE_FORMAT_B8G8R8A8_UNORM;
    default:
       return PIPE_FORMAT_NONE;
    }
@@ -174,6 +177,8 @@ static INLINE unsigned draw_translate_vinfo_format_size(unsigned format)
       return 4 * sizeof(float);
    case EMIT_4UB:
       return 4 * sizeof(unsigned char);
+   case EMIT_4UB_BGRA:
+      return 4 * sizeof(unsigned char);
    default:
       return 0;
    }
diff --git a/src/gallium/auxiliary/draw/draw_vs_aos_io.c b/src/gallium/auxiliary/draw/draw_vs_aos_io.c
index ece1ddd..8f8bbe7 100644
--- a/src/gallium/auxiliary/draw/draw_vs_aos_io.c
+++ b/src/gallium/auxiliary/draw/draw_vs_aos_io.c
@@ -401,13 +401,11 @@ static boolean emit_output( struct aos_compilation *cp,
       emit_store_R32G32B32A32(cp, ptr, dataXMM);
       break;
    case EMIT_4UB:
-      if (1) {
-         emit_swizzle(cp, dataXMM, dataXMM, SHUF(Z,Y,X,W));
-         emit_store_R8G8B8A8_UNORM(cp, ptr, dataXMM);
-      }
-      else {
-         emit_store_R8G8B8A8_UNORM(cp, ptr, dataXMM);
-      }
+      emit_store_R8G8B8A8_UNORM(cp, ptr, dataXMM);
+      break;
+   case EMIT_4UB_BGRA:
+      emit_swizzle(cp, dataXMM, dataXMM, SHUF(Z,Y,X,W));
+      emit_store_R8G8B8A8_UNORM(cp, ptr, dataXMM);
       break;
    default:
       AOS_ERROR(cp, "unhandled output format");
diff --git a/src/gallium/drivers/i915/i915_prim_emit.c b/src/gallium/drivers/i915/i915_prim_emit.c
index d9a5c40..dd997e2 100644
--- a/src/gallium/drivers/i915/i915_prim_emit.c
+++ b/src/gallium/drivers/i915/i915_prim_emit.c
@@ -102,6 +102,13 @@ emit_hw_vertex( struct i915_context *i915,
          count += 4;
          break;
       case EMIT_4UB:
+         OUT_BATCH( pack_ub4(float_to_ubyte( attrib[0] ),
+                             float_to_ubyte( attrib[1] ),
+                             float_to_ubyte( attrib[2] ),
+                             float_to_ubyte( attrib[3] )) );
+         count += 1;
+         break;
+      case EMIT_4UB_BGRA:
          OUT_BATCH( pack_ub4(float_to_ubyte( attrib[2] ),
                              float_to_ubyte( attrib[1] ),
                              float_to_ubyte( attrib[0] ),
diff --git a/src/gallium/drivers/i915/i915_state_derived.c b/src/gallium/drivers/i915/i915_state_derived.c
index 0eb1e3f..4da4677 100644
--- a/src/gallium/drivers/i915/i915_state_derived.c
+++ b/src/gallium/drivers/i915/i915_state_derived.c
@@ -101,14 +101,14 @@ static void calculate_vertex_layout( struct i915_context *i915 )
    /* primary color */
    if (colors[0]) {
       src = draw_find_shader_output(i915->draw, TGSI_SEMANTIC_COLOR, 0);
-      draw_emit_vertex_attr(&vinfo, EMIT_4UB, colorInterp, src);
+      draw_emit_vertex_attr(&vinfo, EMIT_4UB_BGRA, colorInterp, src);
       vinfo.hwfmt[0] |= S4_VFMT_COLOR;
    }
 
    /* secondary color */
    if (colors[1]) {
       src = draw_find_shader_output(i915->draw, TGSI_SEMANTIC_COLOR, 1);
-      draw_emit_vertex_attr(&vinfo, EMIT_4UB, colorInterp, src);
+      draw_emit_vertex_attr(&vinfo, EMIT_4UB_BGRA, colorInterp, src);
       vinfo.hwfmt[0] |= S4_VFMT_SPEC_FOG;
    }
 
diff --git a/src/gallium/drivers/nvfx/nvfx_draw.c b/src/gallium/drivers/nvfx/nvfx_draw.c
index 5379b29..68e50a3 100644
--- a/src/gallium/drivers/nvfx/nvfx_draw.c
+++ b/src/gallium/drivers/nvfx/nvfx_draw.c
@@ -79,6 +79,12 @@ nvfx_render_vertex(struct nvfx_context *nvfx, const struct vertex_header *v)
 					    float_to_ubyte(v->data[idx][1]),
 					    float_to_ubyte(v->data[idx][2]),
 					    float_to_ubyte(v->data[idx][3])));
+		case EMIT_4UB_BGRA:
+			BEGIN_RING(chan, eng3d, NV34TCL_VTX_ATTR_4UB(hw), 1);
+			OUT_RING  (chan, pack_ub4(float_to_ubyte(v->data[idx][2]),
+					    float_to_ubyte(v->data[idx][1]),
+					    float_to_ubyte(v->data[idx][0]),
+					    float_to_ubyte(v->data[idx][3])));
 			break;
 		default:
 			assert(0);
-- 
1.6.0.4

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

Reply via email to