[Mesa-dev] [PATCH 21/36] i965: SF: New implementation of get_attr_override using the VUE map.

[Paul Berry]

On 2 September 2011 13:57, Eric Anholt <eric at anholt.net> wrote:

> On Fri,  2 Sep 2011 09:07:00 -0700, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > This patch changes get_attr_override() (which computes the
> > relationship between vertex shader outputs and fragment shader inputs)
> > to use the VUE map.
> > ---
> >  src/mesa/drivers/dri/i965/brw_state.h     |    3 +-
> >  src/mesa/drivers/dri/i965/gen6_sf_state.c |  108
> +++++++++++++++++------------
> >  src/mesa/drivers/dri/i965/gen7_sf_state.c |   13 +++-
> >  3 files changed, 75 insertions(+), 49 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_state.h
> b/src/mesa/drivers/dri/i965/brw_state.h
> > index cede4e5..167134b 100644
> > --- a/src/mesa/drivers/dri/i965/brw_state.h
> > +++ b/src/mesa/drivers/dri/i965/brw_state.h
> > @@ -206,7 +206,8 @@ void upload_default_color(struct brw_context *brw,
> >
> >  /* gen6_sf_state.c */
> >  uint32_t
> > -get_attr_override(struct brw_context *brw, int fs_attr, int
> two_side_color);
> > +get_attr_override(struct brw_vue_map *vue_map, int
> urb_entry_read_offset,
> > +                  int fs_attr);
> >
> >  /* gen7_misc_state.c */
> >  unsigned int
> > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > index 714914a..5e121f7 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
>
> > +   if (vs_attr < 0 || vs_attr == VERT_RESULT_HPOS) {
> > +      /* These attributes will be overwritten by the fragment shader's
> > +       * interpolation code (see emit_interp() in brw_wm_fp.c), so just
> let
> > +       * them reference attribute the first available attribute.
> > +       */
> > +      return 0;
>
> One too many attributes in that comment.
>

Oops, I'll extra delete the extra word.


>
> > +   /* Compute the location of the attribute relative to
> urb_entry_read_offset.
> > +    * Each increment of urb_entry_read_offset represents a 256-bit
> value, so
> > +    * it counts for two 128-bit VUE slots.
> > +    */
> > +   attr_override = slot - 2*urb_entry_read_offset;
>
> I like spaces between binary operators unless you're really hurting for
> space.
>

Yeah, I flip flop on whether to put spaces around "*".  The programmer in me
wants the spaces, the mathematician in me deplores them.  I guess I was
feeling like a mathematician when I wrote this line.


>
> > +   assert (attr_override >= 0 && attr_override < 32);
> >
> > -   return attr_index;
> > +   /* If the VUE slot following this one represents a back-facing color,
> then
> > +    * we need to instruct the SF unit to do back-facing swizzling.
> > +    */
> > +   if (vue_map->slot_to_vert_result[slot] == VERT_RESULT_COL0 &&
> > +       vue_map->slot_to_vert_result[slot+1] == VERT_RESULT_BFC0)
> > +      attr_override |= (ATTRIBUTE_SWIZZLE_INPUTATTR_FACING <<
> ATTRIBUTE_SWIZZLE_SHIFT);
> > +   else if (vue_map->slot_to_vert_result[slot] == VERT_RESULT_COL1 &&
> > +            vue_map->slot_to_vert_result[slot+1] == VERT_RESULT_BFC1)
> > +      attr_override |= (ATTRIBUTE_SWIZZLE_INPUTATTR_FACING <<
> ATTRIBUTE_SWIZZLE_SHIFT);
> > +
> > +   return attr_override;
>
> I think that this is the wrong criteria for backface color swizzle, not
> that it's a new bug you're introducing.  We need some tests for this.
> From the GL 3.2 compatibility spec, page 75:
>
>    "Additionally, vertex and geometry shaders can operate in two-sided
>     color mode. When a vertex or geometry shader is active, front and
>     back colors can be computed by the shader and written to the
>     gl_FrontColor, gl_BackColor, gl_FrontSecondaryColor and
>     gl_BackSecondaryColor outputs. If VERTEX_PROGRAM_TWO_SIDE is
>     enabled, the GL chooses between front and back colors, as described
>     below. Otherwise, the front color output is always
>     selected. Two-sided color mode is enabled and disabled by calling
>     Enable or Disable with the symbolic value VERTEX_PROGRAM_TWO_SIDE.
>

Agreed--we definitely have bugs in this area, and this patch is not going to
make those bugs any easier to fix.  IIRC, Mesa doesn't even pay attention to
VERTEX_PROGRAM_TWO_SIDE.  If you want to get two-sided color behavior in
your vertex shader, you need to enable GL_LIGHTING and set
GL_LIGHT_MODEL_TWO_SIDE to 1, both of which seem unnecessary.  Getting
VERTEX_PROGRAM_TWO_SIDE to work properly is going to require delving into
some areas of Mesa I'm not familiar with yet, so I think I'd like to
postpone it to a future patch series.

Unless you have an alternate suggestion, I'll change this code so that it
only does the swizzle if two_side_color is set.  There won't be any
functional change for now, but that way it will be easier to find what code
needs changing when we go back later to implement VERTEX_PROGRAM_TWO_SIDE
properly.  And I'll file a bug to remind us about the
VERTEX_PROGRAM_TWO_SIDE problem, since I may not have time to get to it for
a while.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110902/fbb29c2d/attachment.htm>