On 30 June 2012 11:50, Olivier Galibert <galibert at pobox.com> wrote:
> The program keys are updated accordingly, but the values are not used
> yet.
>
> Signed-off-by: Olivier Galibert <galibert at pobox.com>
> ---
> src/mesa/drivers/dri/i965/brw_clip.c | 82
> ++++++++++++++++++++++++++++++-
> src/mesa/drivers/dri/i965/brw_clip.h | 1 +
> src/mesa/drivers/dri/i965/brw_context.h | 59 ++++++++++++++++++++++
> src/mesa/drivers/dri/i965/brw_sf.c | 3 +-
> src/mesa/drivers/dri/i965/brw_sf.h | 1 +
> src/mesa/drivers/dri/i965/brw_wm.c | 4 ++
> src/mesa/drivers/dri/i965/brw_wm.h | 1 +
> 7 files changed, 149 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.c
> b/src/mesa/drivers/dri/i965/brw_clip.c
> index d411208..52e8c47 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.c
> +++ b/src/mesa/drivers/dri/i965/brw_clip.c
> @@ -47,6 +47,83 @@
> #define FRONT_UNFILLED_BIT 0x1
> #define BACK_UNFILLED_BIT 0x2
>
> +/**
> + * Lookup the interpolation mode information for every element in the
> + * vue.
> + */
> +static void
> +brw_lookup_interpolation(struct brw_context *brw)
> +{
> + /* pprog means "previous program", i.e. the last program before the
> + * fragment shader. It can only be the vertex shader for now, but
> + * it may be a geometry shader in the future.
> + */
> + const struct gl_program *pprog = &brw->vertex_program->Base;
> + const struct gl_fragment_program *fprog = brw->fragment_program;
> + struct brw_vue_map *vue_map = &brw->vs.prog_data->vue_map;
> +
> + /* Default everything to INTERP_QUALIFIER_NONE */
> + brw_clear_interpolation_modes(brw);
>
I'm not sure that inline functions like this one add any clarity, since
they force the reader to go look up the function to know what's going on.
I would recommend replacing this with
memset(&brw->interpolation_mode, 0, sizeof(brw->interpolation_mode));
> +
> + /* If there is no fragment shader, interpolation won't be needed,
> + * so defaulting to none is good.
> + */
> + if (!fprog)
> + return;
> +
> + for (int i = 0; i < vue_map->num_slots; i++) {
> + /* First lookup the vert result, skip if there isn't one */
> + int vert_result = vue_map->slot_to_vert_result[i];
> + if (vert_result == BRW_VERT_RESULT_MAX)
> + continue;
> +
> + /* HPOS is special, it must be linear
> + */
>
I believe this is correct, but it's counterintuitive. Can you add an
explanation as to why?
> + if (vert_result == VERT_RESULT_HPOS) {
> + brw_set_interpolation_mode(brw, i,
> INTERP_QUALIFIER_NOPERSPECTIVE);
> + continue;
> + }
> +
> + /* There is a 1-1 mapping of vert result to frag attrib except
> + * for BackColor and vars
> + */
> + int frag_attrib = vert_result;
> + if (vert_result >= VERT_RESULT_BFC0 && vert_result <=
> VERT_RESULT_BFC1)
> + frag_attrib = vert_result - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0;
> + else if(vert_result >= VERT_RESULT_VAR0)
> + frag_attrib = vert_result - VERT_RESULT_VAR0 + FRAG_ATTRIB_VAR0;
> +
> + /* If the output is not used by the fragment shader, skip it. */
> + if (!(fprog->Base.InputsRead & BITFIELD64_BIT(frag_attrib)))
> + continue;
> +
> + /* Lookup the interpolation mode */
> + enum glsl_interp_qualifier interpolation_mode =
> fprog->InterpQualifier[frag_attrib];
> +
> + /* If the mode is not specified, then the default varies. Color
> + * values follow the shader model, while all the rest uses
> + * smooth.
> + */
> + if (interpolation_mode == INTERP_QUALIFIER_NONE) {
> + if (frag_attrib >= FRAG_ATTRIB_COL0 && frag_attrib <=
> FRAG_ATTRIB_COL1)
> + interpolation_mode = brw->intel.ctx.Light.ShadeModel ==
> GL_FLAT ? INTERP_QUALIFIER_FLAT : INTERP_QUALIFIER_SMOOTH;
> + else
> + interpolation_mode = INTERP_QUALIFIER_SMOOTH;
> + }
> +
> + /* Finally, if we have both a front color and a back color for
> + * the same channel, the selection will be done before
> + * interpolation and the back color copied over the front color
> + * if necessary. So interpolating the back color is
> + * unnecessary.
> + */
> + if (vert_result >= VERT_RESULT_BFC0 && vert_result <=
> VERT_RESULT_BFC1)
> + if (pprog->OutputsWritten & BITFIELD64_BIT(vert_result -
> VERT_RESULT_BFC0 + VERT_RESULT_COL0))
> + interpolation_mode = INTERP_QUALIFIER_NONE;
> +
> + brw_set_interpolation_mode(brw, i, interpolation_mode);
> + }
> +}
>
> static void compile_clip_prog( struct brw_context *brw,
> struct brw_clip_prog_key *key )
> @@ -141,6 +218,8 @@ brw_upload_clip_prog(struct brw_context *brw)
>
> memset(&key, 0, sizeof(key));
>
> + brw_lookup_interpolation(brw);
>
Can you add a comment "/* BRW_NEW_FRAGMENT_PROGRAM, _NEW_LIGHT */" above
this line? That will help remind us that this function consults the
fragment program and the lighting state.
> +
> /* Populate the key:
> */
> /* BRW_NEW_REDUCED_PRIMITIVE */
> @@ -150,6 +229,7 @@ brw_upload_clip_prog(struct brw_context *brw)
> /* _NEW_LIGHT */
> key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT);
> key.pv_first = (ctx->Light.ProvokingVertex ==
> GL_FIRST_VERTEX_CONVENTION);
> + brw_copy_interpolation_modes(brw, key.interpolation_mode);
>
Similar to my comment above about brw_clear_interpolation_modes(), I would
recommend replacing this with a simple call to memcpy().
> /* _NEW_TRANSFORM (also part of VUE map)*/
> key.nr_userclip = _mesa_bitcount_64(ctx->Transform.ClipPlanesEnabled);
>
> @@ -258,7 +338,7 @@ const struct brw_tracked_state brw_clip_prog = {
> _NEW_TRANSFORM |
> _NEW_POLYGON |
> _NEW_BUFFERS),
> - .brw = (BRW_NEW_REDUCED_PRIMITIVE),
> + .brw = (BRW_NEW_FRAGMENT_PROGRAM|BRW_NEW_REDUCED_PRIMITIVE),
> .cache = CACHE_NEW_VS_PROG
> },
> .emit = brw_upload_clip_prog
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
> b/src/mesa/drivers/dri/i965/brw_clip.h
> index 9185651..6f811ae 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.h
> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
> @@ -43,6 +43,7 @@
> */
> struct brw_clip_prog_key {
> GLbitfield64 attrs;
> + GLbitfield64 interpolation_mode[2]; /* copy of the main context */
> GLuint primitive:4;
> GLuint nr_userclip:4;
> GLuint do_flat_shading:1;
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 69659c4e..1a417e5 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1041,6 +1041,17 @@ struct brw_context
> uint32_t render_target_format[MESA_FORMAT_COUNT];
> bool format_supported_as_render_target[MESA_FORMAT_COUNT];
>
> + /* Interpolation modes, two bits per vue slot, values equal to
> + * glsl_interp_qualifier.
> + *
> + * Used on gen4/5 by the clipper, sf and wm stages. Given the
> + * update order, the clipper is resposible to update it.
>
+ *
> + * Ignored on gen 6+
> + */
> +
> + GLbitfield64 interpolation_mode[2]; /* two bits per vue slot */
> +
>
I don't think the space savings is worth the complication here. How about
changing this (and the other new interpolation_mode arrays) to:
unsigned char interpolation_mode[BRW_VERT_RESULT_MAX];
Then we can replace brw_set_interpolation_mode() and
brw_get_interpolation_mode() by simply indexing into the array.
> /* PrimitiveRestart */
> struct {
> bool in_progress;
> @@ -1262,6 +1273,54 @@ brw_program_reloc(struct brw_context *brw, uint32_t
> state_offset,
>
> bool brw_do_cubemap_normalize(struct exec_list *instructions);
>
> +/**
> + * Pre-gen6, interpolation mode has to be resolved for code generation
> + * in clipper, sf and wm. The resolution is done once by the clipper
> + * and stored in the interpolation_mode array and in the keys. These
> + * functions allow access to that packed array.
> + */
> +
> +/* INTERP_QUALIFIER_NONE is required to be 0, so initialization is easy */
> +static inline
> +void brw_clear_interpolation_modes(struct brw_context *brw)
> +{
> + brw->interpolation_mode[0] = brw->interpolation_mode[1] = 0;
> +}
> +
> +static inline
> +void brw_clear_interpolation_modes_key(GLbitfield64 *interpolation_mode)
> +{
> + interpolation_mode[0] = interpolation_mode[1] = 0;
> +}
> +
> +/* Store an interpolation mode */
> +static inline
> +void brw_set_interpolation_mode(struct brw_context *brw, int slot, enum
> glsl_interp_qualifier mode)
> +{
> + int idx = slot >> 5;
> + int shift = 2*(slot & 31);
> + brw->interpolation_mode[idx] =
> + (brw->interpolation_mode[idx] & ~(3ULL << shift))
> + | (((GLbitfield64)mode) << shift);
> +}
> +
> +/* Retrieve an interpolation mode */
> +static inline
> +enum glsl_interp_qualifier brw_get_interpolation_mode(struct brw_context
> *brw, int slot)
> +{
> + int idx = slot >> 5;
> + int shift = 2*(slot & 31);
> + return (enum glsl_interp_qualifier)((brw->interpolation_mode[idx] >>
> shift) & 3);
> +}
> +
> +/* Copy the interpolation mode information to a key */
> +static inline
> +void brw_copy_interpolation_modes(struct brw_context *brw, GLbitfield64
> *dest)
> +{
> + dest[0] = brw->interpolation_mode[0];
> + dest[1] = brw->interpolation_mode[1];
> +}
> +
>
If we change the type of interpolation_mode, and use memset() and memcpy()
where they make sense, I think we can eliminate the need for all these
inline functions.
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/mesa/drivers/dri/i965/brw_sf.c
> b/src/mesa/drivers/dri/i965/brw_sf.c
> index 7867ab5..0cc4fc7 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf.c
> @@ -194,6 +194,7 @@ brw_upload_sf_prog(struct brw_context *brw)
> key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT);
> key.do_twoside_color = (ctx->Light.Enabled &&
> ctx->Light.Model.TwoSide) ||
> ctx->VertexProgram._TwoSideEnabled;
> + brw_copy_interpolation_modes(brw, key.interpolation_mode);
>
This can also be replaced with a call to memcpy(). Also, we should put a
comment "/* BRW_NEW_FRAGMENT_PROGRAM, _NEW_LIGHT */" above it.
>
> /* _NEW_POLYGON */
> if (key.do_twoside_color) {
> @@ -216,7 +217,7 @@ const struct brw_tracked_state brw_sf_prog = {
> .dirty = {
> .mesa = (_NEW_HINT | _NEW_LIGHT | _NEW_POLYGON | _NEW_POINT |
> _NEW_TRANSFORM | _NEW_BUFFERS),
> - .brw = (BRW_NEW_REDUCED_PRIMITIVE),
> + .brw = (BRW_NEW_FRAGMENT_PROGRAM|BRW_NEW_REDUCED_PRIMITIVE),
> .cache = CACHE_NEW_VS_PROG
> },
> .emit = brw_upload_sf_prog
> diff --git a/src/mesa/drivers/dri/i965/brw_sf.h
> b/src/mesa/drivers/dri/i965/brw_sf.h
> index f908fc0..0a8135c 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf.h
> +++ b/src/mesa/drivers/dri/i965/brw_sf.h
> @@ -46,6 +46,7 @@
>
> struct brw_sf_prog_key {
> GLbitfield64 attrs;
> + GLbitfield64 interpolation_mode[2]; /* copy of the main context */
> uint8_t point_sprite_coord_replace;
> GLuint primitive:2;
> GLuint do_twoside_color:1;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
> b/src/mesa/drivers/dri/i965/brw_wm.c
> index 4a7225c..c7c1c45 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -504,6 +504,10 @@ static void brw_wm_populate_key( struct brw_context
> *brw,
>
> /* _NEW_LIGHT */
> key->flat_shade = (ctx->Light.ShadeModel == GL_FLAT);
> + if (intel->gen < 6)
> + brw_copy_interpolation_modes(brw, key->interpolation_mode);
> + else
> + brw_clear_interpolation_modes_key(key->interpolation_mode);
>
The "else" part isn't necessary, since we memset() the key to zero at the
top of the function.
>
> /* _NEW_FRAG_CLAMP | _NEW_BUFFERS */
> key->clamp_fragment_color = ctx->Color._ClampFragmentColor;
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.h
> b/src/mesa/drivers/dri/i965/brw_wm.h
> index 2cde2a0..53b83f8 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.h
> +++ b/src/mesa/drivers/dri/i965/brw_wm.h
> @@ -60,6 +60,7 @@
> #define AA_ALWAYS 2
>
> struct brw_wm_prog_key {
> + GLbitfield64 interpolation_mode[2]; /* copy of the main context for
> gen4-5, 0 for gen6+ */
> uint8_t iz_lookup;
> GLuint stats_wm:1;
> GLuint flat_shade:1;
> --
> 1.7.10.rc3.1.gb306
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120716/315102d5/attachment-0001.html>
