[Mesa-dev] [PATCH 3/3] draw: fix clipping of layer/vp index outputs
Brian Paul
brianp at vmware.com
Wed Dec 2 16:56:42 PST 2015
The series LGTM. Reviewed-by: Brian Paul <brianp at vmware.com>
Just minor whitespace nits below.
On 12/02/2015 05:35 PM, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> This was just plain broken. It used always the value from v0 (for vp_index)
> but would pass the value from the provoking vertex to later stages - but only
> if there was a corresponding fs input, otherwise the layer/vp index would get
> lost completely (as it would try to interpolate the (unsigned) values as
> floats).
> So, make it obey provoking vertex rules (drivers relying on draw will need to
> do the same). And make sure that the default interpolation mode (when no
> corresponding fs input is found) for them is constant.
> Also, change the code a bit so constant inputs aren't interpolated then
> copied over later.
>
> No piglit change (but I'm writing one...).
> ---
> src/gallium/auxiliary/draw/draw_pipe_clip.c | 207 +++++++++++++++++-----------
> 1 file changed, 130 insertions(+), 77 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> index c22758b..7d9ea5e 100644
> --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
> +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
> @@ -58,12 +58,17 @@
> struct clip_stage {
> struct draw_stage stage; /**< base class */
>
> - /* List of the attributes to be flatshaded. */
> - uint num_flat_attribs;
> - uint flat_attribs[PIPE_MAX_SHADER_OUTPUTS];
> -
> - /* Mask of attributes in noperspective mode */
> - boolean noperspective_attribs[PIPE_MAX_SHADER_OUTPUTS];
> + unsigned pos_attr;
> +
> + /* List of the attributes to be constant interpolated. */
> + uint num_const_attribs;
> + uint8_t const_attribs[PIPE_MAX_SHADER_OUTPUTS];
> + /* List of the attributes to be linear interpolated. */
> + uint num_linear_attribs;
> + uint8_t linear_attribs[PIPE_MAX_SHADER_OUTPUTS];
> + /* List of the attributes to be perspective interpolated. */
> + uint num_perspect_attribs;
> + uint8_t perspect_attribs[PIPE_MAX_SHADER_OUTPUTS];
>
> float (*plane)[4];
> };
> @@ -97,9 +102,9 @@ draw_viewport_index(struct draw_context *draw,
> /* All attributes are float[4], so this is easy:
> */
> static void interp_attr( float dst[4],
> - float t,
> - const float in[4],
> - const float out[4] )
> + float t,
> + const float in[4],
> + const float out[4] )
> {
> dst[0] = LINTERP( t, out[0], in[0] );
> dst[1] = LINTERP( t, out[1], in[1] );
> @@ -117,8 +122,8 @@ static void copy_flat( struct draw_stage *stage,
> {
> const struct clip_stage *clipper = clip_stage(stage);
> uint i;
> - for (i = 0; i < clipper->num_flat_attribs; i++) {
> - const uint attr = clipper->flat_attribs[i];
> + for (i = 0; i < clipper->num_const_attribs; i++) {
> + const uint attr = clipper->const_attribs[i];
> COPY_4FV(dst->data[attr], src->data[attr]);
> }
> }
> @@ -126,15 +131,13 @@ static void copy_flat( struct draw_stage *stage,
> /* Interpolate between two vertices to produce a third.
> */
> static void interp( const struct clip_stage *clip,
> - struct vertex_header *dst,
> - float t,
> - const struct vertex_header *out,
> - const struct vertex_header *in,
> + struct vertex_header *dst,
> + float t,
> + const struct vertex_header *out,
> + const struct vertex_header *in,
> unsigned viewport_index )
> {
> - const unsigned nr_attrs = draw_num_shader_outputs(clip->stage.draw);
> - const unsigned pos_attr = draw_current_shader_position_output(clip->stage.draw);
> - const unsigned clip_attr = draw_current_shader_clipvertex_output(clip->stage.draw);
> + const unsigned pos_attr = clip->pos_attr;
> unsigned j;
> float t_nopersp;
>
> @@ -168,6 +171,13 @@ static void interp( const struct clip_stage *clip,
> dst->data[pos_attr][3] = oow;
> }
>
> +
> + /* interp perspective attribs */
> + for (j = 0; j < clip->num_perspect_attribs; j++) {
> + const unsigned attr = clip->perspect_attribs[j];
> + interp_attr(dst->data[attr], t, in->data[attr], out->data[attr]);
> + }
> +
> /**
> * Compute the t in screen-space instead of 3d space to use
> * for noperspective interpolation.
> @@ -177,7 +187,7 @@ static void interp( const struct clip_stage *clip,
> * pick whatever value (the interpolated point won't be in front
> * anyway), so just use the 3d t.
> */
> - {
> + if (clip->num_linear_attribs){
Space between ) and {.
-Brian
> int k;
> t_nopersp = t;
> /* find either in.x != out.x or in.y != out.y */
> @@ -191,24 +201,17 @@ static void interp( const struct clip_stage *clip,
> break;
> }
> }
> - }
> -
> - /* Other attributes
> - */
> - for (j = 0; j < nr_attrs; j++) {
> - if (j != pos_attr && j != clip_attr) {
> - if (clip->noperspective_attribs[j])
> - interp_attr(dst->data[j], t_nopersp, in->data[j], out->data[j]);
> - else
> - interp_attr(dst->data[j], t, in->data[j], out->data[j]);
> + for (j = 0; j < clip->num_linear_attribs; j++) {
> + const unsigned attr = clip->linear_attribs[j];
> + interp_attr(dst->data[attr], t_nopersp, in->data[attr], out->data[attr]);
> }
> }
> }
>
> /**
> - * Checks whether the specifed triangle is empty and if it is returns
> + * Checks whether the specified triangle is empty and if it is returns
> * true, otherwise returns false.
> - * Triangle is considered null/empty if it's area is qual to zero.
> + * Triangle is considered null/empty if it's area is equal to zero.
> */
> static inline boolean
> is_tri_null(struct draw_context *draw, const struct prim_header *header)
> @@ -234,10 +237,10 @@ is_tri_null(struct draw_context *draw, const struct prim_header *header)
> * will be convex and the provoking vertex will always be vertex[0].
> */
> static void emit_poly( struct draw_stage *stage,
> - struct vertex_header **inlist,
> + struct vertex_header **inlist,
> const boolean *edgeflags,
> - unsigned n,
> - const struct prim_header *origPrim)
> + unsigned n,
> + const struct prim_header *origPrim)
> {
> struct prim_header header;
> unsigned i;
> @@ -359,14 +362,15 @@ static inline float getclipdist(const struct clip_stage *clipper,
> */
> static void
> do_clip_tri( struct draw_stage *stage,
> - struct prim_header *header,
> - unsigned clipmask )
> + struct prim_header *header,
> + unsigned clipmask )
> {
> struct clip_stage *clipper = clip_stage( stage );
> struct vertex_header *a[MAX_CLIPPED_VERTICES];
> struct vertex_header *b[MAX_CLIPPED_VERTICES];
> struct vertex_header **inlist = a;
> struct vertex_header **outlist = b;
> + struct vertex_header *prov_vertex;
> unsigned tmpnr = 0;
> unsigned n = 3;
> unsigned i;
> @@ -380,7 +384,20 @@ do_clip_tri( struct draw_stage *stage,
> inlist[1] = header->v[1];
> inlist[2] = header->v[2];
>
> - viewport_index = draw_viewport_index(clipper->stage.draw, inlist[0]);
> + /*
> + * For d3d10, we need to take this from the leading (first) vertex.
> + * For GL, we could do anything (as long as we advertize
> + * GL_UNDEFINED_VERTEX for the VIEWPORT_INDEX_PROVOKING_VERTEX query),
> + * but it needs to be consistent with what other parts (i.e. driver)
> + * will do, and that seems easier with GL_PROVOKING_VERTEX logic.
> + */
> + if (stage->draw->rasterizer->flatshade_first) {
> + prov_vertex = inlist[0];
> + }
> + else {
> + prov_vertex = inlist[2];
> + }
> + viewport_index = draw_viewport_index(clipper->stage.draw, prov_vertex);
>
> if (DEBUG_CLIP) {
> const float *v0 = header->v[0]->clip;
> @@ -512,10 +529,10 @@ do_clip_tri( struct draw_stage *stage,
>
> }
>
> - /* If flat-shading, copy provoking vertex color to polygon vertex[0]
> + /* If constant interpolated, copy provoking vertex attrib to polygon vertex[0]
> */
> if (n >= 3) {
> - if (clipper->num_flat_attribs) {
> + if (clipper->num_const_attribs) {
> if (stage->draw->rasterizer->flatshade_first) {
> if (inlist[0] != header->v[0]) {
> assert(tmpnr < MAX_CLIPPED_VERTICES + 1);
> @@ -547,16 +564,25 @@ do_clip_tri( struct draw_stage *stage,
> */
> static void
> do_clip_line( struct draw_stage *stage,
> - struct prim_header *header,
> - unsigned clipmask )
> + struct prim_header *header,
> + unsigned clipmask )
> {
> const struct clip_stage *clipper = clip_stage( stage );
> struct vertex_header *v0 = header->v[0];
> struct vertex_header *v1 = header->v[1];
> + struct vertex_header *prov_vertex;
> float t0 = 0.0F;
> float t1 = 0.0F;
> struct prim_header newprim;
> - int viewport_index = draw_viewport_index(clipper->stage.draw, v0);
> + int viewport_index;
> +
> + if (stage->draw->rasterizer->flatshade_first) {
> + prov_vertex = v0;
> + }
> + else {
> + prov_vertex = v1;
> + }
> + viewport_index = draw_viewport_index(clipper->stage.draw, prov_vertex);
>
> while (clipmask) {
> const unsigned plane_idx = ffs(clipmask)-1;
> @@ -567,17 +593,17 @@ do_clip_line( struct draw_stage *stage,
> return; //discard nan
>
> if (dp1 < 0.0F) {
> - float t = dp1 / (dp1 - dp0);
> + float t = dp1 / (dp1 - dp0);
> t1 = MAX2(t1, t);
> }
>
> if (dp0 < 0.0F) {
> - float t = dp0 / (dp0 - dp1);
> + float t = dp0 / (dp0 - dp1);
> t0 = MAX2(t0, t);
> }
>
> if (t0 + t1 >= 1.0F)
> - return; /* discard */
> + return; /* discard */
>
> clipmask &= ~(1 << plane_idx); /* turn off this plane's bit */
> }
> @@ -668,7 +694,7 @@ clip_first_point( struct draw_stage *stage,
>
> static void
> clip_line( struct draw_stage *stage,
> - struct prim_header *header )
> + struct prim_header *header )
> {
> unsigned clipmask = (header->v[0]->clipmask |
> header->v[1]->clipmask);
> @@ -715,12 +741,24 @@ find_interp(const struct draw_fragment_shader *fs, int *indexed_interp,
> if (semantic_name == TGSI_SEMANTIC_COLOR ||
> semantic_name == TGSI_SEMANTIC_BCOLOR) {
> interp = indexed_interp[semantic_index];
> + } else if (semantic_name == TGSI_SEMANTIC_POSITION ||
> + semantic_name == TGSI_SEMANTIC_CLIPVERTEX) {
> + /* these inputs are handled specially always */
> + return -1;
> } else {
> /* Otherwise, search in the FS inputs, with a decent default
> * if we don't find it.
> + * This probably only matters for layer, vpindex, culldist, maybe
> + * front_face.
> */
> uint j;
> - interp = TGSI_INTERPOLATE_PERSPECTIVE;
> + if (semantic_name == TGSI_SEMANTIC_LAYER ||
> + semantic_name == TGSI_SEMANTIC_VIEWPORT_INDEX) {
> + interp = TGSI_INTERPOLATE_CONSTANT;
> + }
> + else {
> + interp = TGSI_INTERPOLATE_PERSPECTIVE;
> + }
> if (fs) {
> for (j = 0; j < fs->info.num_inputs; j++) {
> if (semantic_name == fs->info.input_semantic_name[j] &&
> @@ -745,6 +783,9 @@ clip_init_state( struct draw_stage *stage )
> const struct draw_fragment_shader *fs = draw->fs.fragment_shader;
> const struct tgsi_shader_info *info = draw_get_shader_info(draw);
> uint i, j;
> + int indexed_interp[2];
> +
> + clipper->pos_attr = draw_current_shader_position_output(draw);
>
> /* We need to know for each attribute what kind of interpolation is
> * done on it (flat, smooth or noperspective). But the information
> @@ -765,7 +806,6 @@ clip_init_state( struct draw_stage *stage )
> /* First pick up the interpolation mode for
> * gl_Color/gl_SecondaryColor, with the correct default.
> */
> - int indexed_interp[2];
> indexed_interp[0] = indexed_interp[1] = draw->rasterizer->flatshade ?
> TGSI_INTERPOLATE_CONSTANT : TGSI_INTERPOLATE_PERSPECTIVE;
>
> @@ -778,29 +818,33 @@ clip_init_state( struct draw_stage *stage )
> }
> }
>
> - /* Then resolve the interpolation mode for every output attribute.
> - *
> - * Given how the rest of the code, the most efficient way is to
> - * have a vector of flat-mode attributes, and a mask for
> - * noperspective attributes.
> - */
> + /* Then resolve the interpolation mode for every output attribute. */
>
> - clipper->num_flat_attribs = 0;
> - memset(clipper->noperspective_attribs, 0, sizeof(clipper->noperspective_attribs));
> + clipper->num_const_attribs = 0;
> + clipper->num_linear_attribs = 0;
> + clipper->num_perspect_attribs = 0;
> for (i = 0; i < info->num_outputs; i++) {
> /* Find the interpolation mode for a specific attribute */
> int interp = find_interp(fs, indexed_interp,
> info->output_semantic_name[i],
> info->output_semantic_index[i]);
> - /* If it's flat, add it to the flat vector. Otherwise update
> - * the noperspective mask.
> - */
> -
> - if (interp == TGSI_INTERPOLATE_CONSTANT) {
> - clipper->flat_attribs[clipper->num_flat_attribs] = i;
> - clipper->num_flat_attribs++;
> - } else
> - clipper->noperspective_attribs[i] = interp == TGSI_INTERPOLATE_LINEAR;
> + switch (interp) {
> + case TGSI_INTERPOLATE_CONSTANT:
> + clipper->const_attribs[clipper->num_const_attribs] = i;
> + clipper->num_const_attribs++;
> + break;
> + case TGSI_INTERPOLATE_LINEAR:
> + clipper->linear_attribs[clipper->num_linear_attribs] = i;
> + clipper->num_linear_attribs++;
> + break;
> + case TGSI_INTERPOLATE_PERSPECTIVE:
> + clipper->perspect_attribs[clipper->num_perspect_attribs] = i;
> + clipper->num_perspect_attribs++;
> + break;
> + default:
> + assert(interp == -1);
> + break;
> + }
> }
> /* Search the extra vertex attributes */
> for (j = 0; j < draw->extra_shader_outputs.num; j++) {
> @@ -808,16 +852,25 @@ clip_init_state( struct draw_stage *stage )
> int interp = find_interp(fs, indexed_interp,
> draw->extra_shader_outputs.semantic_name[j],
> draw->extra_shader_outputs.semantic_index[j]);
> - /* If it's flat, add it to the flat vector. Otherwise update
> - * the noperspective mask.
> - */
> - if (interp == TGSI_INTERPOLATE_CONSTANT) {
> - clipper->flat_attribs[clipper->num_flat_attribs] = i + j;
> - clipper->num_flat_attribs++;
> - } else
> - clipper->noperspective_attribs[i + j] = interp == TGSI_INTERPOLATE_LINEAR;
> + switch (interp) {
> + case TGSI_INTERPOLATE_CONSTANT:
> + clipper->const_attribs[clipper->num_const_attribs] = i + j;
> + clipper->num_const_attribs++;
> + break;
> + case TGSI_INTERPOLATE_LINEAR:
> + clipper->linear_attribs[clipper->num_linear_attribs] = i + j;
> + clipper->num_linear_attribs++;
> + break;
> + case TGSI_INTERPOLATE_PERSPECTIVE:
> + clipper->perspect_attribs[clipper->num_perspect_attribs] = i + j;
> + clipper->num_perspect_attribs++;
> + break;
> + default:
> + assert(interp == -1);
> + break;
> + }
> }
> -
> +
> stage->tri = clip_tri;
> stage->line = clip_line;
> }
> @@ -825,14 +878,14 @@ clip_init_state( struct draw_stage *stage )
>
>
> static void clip_first_tri( struct draw_stage *stage,
> - struct prim_header *header )
> + struct prim_header *header )
If you're going to reformat, I'd suggest:
static void
clip_first_tri(struct draw_stage *stage, struct prim_header *header)
for these functions.
> {
> clip_init_state( stage );
> stage->tri( stage, header );
> }
>
> static void clip_first_line( struct draw_stage *stage,
> - struct prim_header *header )
> + struct prim_header *header )
> {
> clip_init_state( stage );
> stage->line( stage, header );
> @@ -840,7 +893,7 @@ static void clip_first_line( struct draw_stage *stage,
>
>
> static void clip_flush( struct draw_stage *stage,
> - unsigned flags )
> + unsigned flags )
> {
> stage->tri = clip_first_tri;
> stage->line = clip_first_line;
>
More information about the mesa-dev
mailing list