On 30 June 2012 11:50, Olivier Galibert <<a href="mailto:galibert@pobox.com" target="_blank">galibert@pobox.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The program keys are updated accordingly, but the values are not used yet. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Signed-off-by: Olivier Galibert <<a href="mailto:galibert@pobox.com">galibert@pobox.com</a>> --- 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); </blockquote><div> 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)); </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* 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 + */ </blockquote><div> I believe this is correct, but it's counterintuitive. Can you add an explanation as to why? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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); </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + /* 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); </blockquote><div> Similar to my comment above about brw_clear_interpolation_modes(), I would recommend replacing this with a simple call to memcpy(). </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /* _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. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + * + * Ignored on gen 6+ + */ + + GLbitfield64 interpolation_mode[2]; /* two bits per vue slot */ + </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /* 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]; +} + </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> #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); </blockquote><div> This can also be replaced with a call to memcpy(). Also, we should put a comment "/* BRW_NEW_FRAGMENT_PROGRAM, _NEW_LIGHT */" above it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /* _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); </blockquote><div> The "else" part isn't necessary, since we memset() the key to zero at the top of the function. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /* _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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>