On 2 September 2011 12:47, Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> <div class="im">On Fri, 2 Sep 2011 09:06:41 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > Several places in the i965 code make implicit assumptions about the > structure of data in the VUE (vertex URB entry). This patch adds a > function, brw_compute_vue_map(), which computes the structure of the > VUE explicitly. Future patches will modify the rest of the driver to > use the explicitly computed map rather than rely on implicit > assumptions about it. > --- </div><div class="im">> + if (two_side_color) { > + /* front and back colors need to be consecutive */ > + if ((outputs_written & BITFIELD64_BIT(VERT_RESULT_COL1)) && > + (outputs_written & BITFIELD64_BIT(VERT_RESULT_BFC1))) { > + assert(outputs_written & BITFIELD64_BIT(VERT_RESULT_COL0)); > + assert(outputs_written & BITFIELD64_BIT(VERT_RESULT_BFC0)); > + assign_vue_slot(vue_map, VERT_RESULT_COL0); > + assign_vue_slot(vue_map, VERT_RESULT_BFC0); > + assign_vue_slot(vue_map, VERT_RESULT_COL1); > + assign_vue_slot(vue_map, VERT_RESULT_BFC1); > + } else if ((outputs_written & BITFIELD64_BIT(VERT_RESULT_COL0)) && > + (outputs_written & BITFIELD64_BIT(VERT_RESULT_BFC0))) { > + assign_vue_slot(vue_map, VERT_RESULT_COL0); > + assign_vue_slot(vue_map, VERT_RESULT_BFC0); > + } </div>This looks like you could move the else if () up above and drop the COL0/BFC0 concerns from the COL1/BFC1 path. (and then I don't have to think about what can generates COL1/BFC1 and whether the asserts are valid). </blockquote></div> Yeah, I suspect that some of these asserts are incorrect. I remember thinking they were suspicious when I copied them from brw_vs_alloc_regs() and get_attr_override()--for example, what happens if the client code turns on two-sided color and writes to gl_FrontColorSecondary and gl_BackColorSecondary but not gl_FrontColor and gl_BackColor? Then these assertions would fire. That would be kind of weird, but it shouldn't be illegal. Thinking about it further, what I would actually prefer to do is to always put front and back colors next to each other in the VUE (at least for GEN6+), regardless of whether we are doing two-sided color. Then the code would look something like this: if (outputs_written & BITFIELD64_BIT(VERT_RESULT_COL0) assign_vue_slot(vue_map, VERT_RESULT_COL0); if (outputs_written & BITFIELD64_BIT(VERT_RESULT_BFC0) assign_vue_slot(vue_map, VERT_RESULT_BFC0); if (outputs_written & BITFIELD64_BIT(VERT_RESULT_COL1) assign_vue_slot(vue_map, VERT_RESULT_COL1); if (outputs_written & BITFIELD64_BIT(VERT_RESULT_BFC1) assign_vue_slot(vue_map, VERT_RESULT_BFC1); Then we would let get_attr_override() decide whether or not to swizzle the colors based on the setting of two_side_color. I think that would address your comment (since it would remove the assertions). It would also have the advantage of making the layout of the VUE easier for humans to comprehend. I'll try doing that as a follow-up patch. (I don't want to do it by altering this patch because in future patches, where I switch the driver over to using the VUE map, I don't want to cause behavioral changes).