[Mesa-dev] [PATCH 2/2] draw: remove clip from vertex header
Roland Scheidegger
sroland at vmware.com
Fri Dec 11 08:03:24 PST 2015
Ahh actually I suspect this really isn't right, and probably the root
cause of https://bugs.freedesktop.org/show_bug.cgi?id=80183 (which I
hack-fixed with 6d2ecdb4a63350cfeee803c00ac283ee013a5ee5 but did not
understand the reasons).
So I believe (regardless of this patch) for the ordinary xyz clipping,
always the data from gl_Position should be used, and not the one from
gl_ClipVertex. Does that sound right?
I wonder if it would make things easier if we'd just get rid of
clipvertex in the vs output - that is transform the shader to one using
clipdist instead. It looks though like the code isn't well prepared for
that (as it would remove outputs and add other outputs instead, plus
need the clip planes as inputs). Just about every driver does that
already in some way in fact... And draw llvm pretty much does that
already too, albeit it doesn't store the clip distances output (just
uses them for the built-in clip test).
Roland
Am 11.12.2015 um 05:47 schrieb Roland Scheidegger:
> FWIW I'm unconvinced this is really worth doing, though it makes it a
> bit more explicit which variable is really used.
> And it appears to work...
> Though there's something I'm wondering about, albeit it's unchanged from
> before, if the shader writes clipVertex (and position), for the ordinary
> xyz clipping should clipVertex or position be used?
>
> Roland
>
>
> Am 11.12.2015 um 05:43 schrieb sroland at vmware.com:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> vertex header had both clip and pre_clip_pos. There was some confusion what
>> these were really used for, so just remove one.
>> We need one in any case (pre_clip_pos) because the draw llvm shader would
>> overwrite the position output from the vs with the viewport transformed.
>> However, we don't really need the second one, which was only really used
>> for gl_ClipVertex - if the shader didn't have that the values were just
>> duplicated to both clip and pre_clip_pos. So, just use this from the vs
>> output instead.
>> ---
>> src/gallium/auxiliary/draw/draw_cliptest_tmp.h | 1 -
>> src/gallium/auxiliary/draw/draw_llvm.c | 21 ++-----
>> src/gallium/auxiliary/draw/draw_llvm.h | 4 --
>> src/gallium/auxiliary/draw/draw_pipe_clip.c | 76 +++++++++++++++++++-------
>> src/gallium/auxiliary/draw/draw_private.h | 1 -
>> 5 files changed, 61 insertions(+), 42 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> index 779b237..1e5bceb 100644
>> --- a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> +++ b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> @@ -91,7 +91,6 @@ static boolean TAG(do_cliptest)( struct pt_post_vs *pvs,
>> }
>>
>> for (i = 0; i < 4; i++) {
>> - out->clip[i] = clipvertex[i];
>> out->pre_clip_pos[i] = position[i];
>> }
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c b/src/gallium/auxiliary/draw/draw_llvm.c
>> index ee97424..f449a80 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>> @@ -375,14 +375,13 @@ static LLVMTypeRef
>> create_jit_vertex_header(struct gallivm_state *gallivm, int data_elems)
>> {
>> LLVMTargetDataRef target = gallivm->target;
>> - LLVMTypeRef elem_types[4];
>> + LLVMTypeRef elem_types[3];
>> LLVMTypeRef vertex_header;
>> char struct_name[24];
>>
>> util_snprintf(struct_name, 23, "vertex_header%d", data_elems);
>>
>> elem_types[DRAW_JIT_VERTEX_VERTEX_ID] = LLVMIntTypeInContext(gallivm->context, 32);
>> - elem_types[DRAW_JIT_VERTEX_CLIP] = LLVMArrayType(LLVMFloatTypeInContext(gallivm->context), 4);
>> elem_types[DRAW_JIT_VERTEX_PRE_CLIP_POS] = LLVMArrayType(LLVMFloatTypeInContext(gallivm->context), 4);
>> elem_types[DRAW_JIT_VERTEX_DATA] = LLVMArrayType(elem_types[1], data_elems);
>>
>> @@ -403,9 +402,6 @@ create_jit_vertex_header(struct gallivm_state *gallivm, int data_elems)
>> target, vertex_header,
>> DRAW_JIT_VERTEX_VERTEX_ID);
>> */
>> - LP_CHECK_MEMBER_OFFSET(struct vertex_header, clip,
>> - target, vertex_header,
>> - DRAW_JIT_VERTEX_CLIP);
>> LP_CHECK_MEMBER_OFFSET(struct vertex_header, pre_clip_pos,
>> target, vertex_header,
>> DRAW_JIT_VERTEX_PRE_CLIP_POS);
>> @@ -1014,7 +1010,7 @@ store_clip(struct gallivm_state *gallivm,
>> const struct lp_type vs_type,
>> LLVMValueRef io_ptr,
>> LLVMValueRef (*outputs)[TGSI_NUM_CHANNELS],
>> - boolean pre_clip_pos, int idx)
>> + int idx)
>> {
>> LLVMBuilderRef builder = gallivm->builder;
>> LLVMValueRef soa[4];
>> @@ -1041,14 +1037,8 @@ store_clip(struct gallivm_state *gallivm,
>> soa[2] = LLVMBuildLoad(builder, outputs[idx][2], ""); /*z0 z1 .. zn*/
>> soa[3] = LLVMBuildLoad(builder, outputs[idx][3], ""); /*w0 w1 .. wn*/
>>
>> - if (!pre_clip_pos) {
>> - for (i = 0; i < vs_type.length; i++) {
>> - clip_ptrs[i] = draw_jit_header_clip(gallivm, io_ptrs[i]);
>> - }
>> - } else {
>> - for (i = 0; i < vs_type.length; i++) {
>> - clip_ptrs[i] = draw_jit_header_pre_clip_pos(gallivm, io_ptrs[i]);
>> - }
>> + for (i = 0; i < vs_type.length; i++) {
>> + clip_ptrs[i] = draw_jit_header_pre_clip_pos(gallivm, io_ptrs[i]);
>> }
>>
>> lp_build_transpose_aos(gallivm, vs_type, soa, soa);
>> @@ -1766,8 +1756,7 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant,
>>
>> if (pos != -1 && cv != -1) {
>> /* store original positions in clip before further manipulation */
>> - store_clip(gallivm, vs_type, io, outputs, FALSE, key->clip_user ? cv : pos);
>> - store_clip(gallivm, vs_type, io, outputs, TRUE, pos);
>> + store_clip(gallivm, vs_type, io, outputs, pos);
>>
>> /* do cliptest */
>> if (enable_cliptest) {
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.h b/src/gallium/auxiliary/draw/draw_llvm.h
>> index d153c16..2b4d151 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.h
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.h
>> @@ -104,7 +104,6 @@ enum {
>>
>> enum {
>> DRAW_JIT_VERTEX_VERTEX_ID = 0,
>> - DRAW_JIT_VERTEX_CLIP,
>> DRAW_JIT_VERTEX_PRE_CLIP_POS,
>> DRAW_JIT_VERTEX_DATA
>> };
>> @@ -162,9 +161,6 @@ enum {
>> #define draw_jit_header_id(_gallivm, _ptr) \
>> lp_build_struct_get_ptr(_gallivm, _ptr, DRAW_JIT_VERTEX_VERTEX_ID, "id")
>>
>> -#define draw_jit_header_clip(_gallivm, _ptr) \
>> - lp_build_struct_get_ptr(_gallivm, _ptr, DRAW_JIT_VERTEX_CLIP, "clip")
>> -
>> #define draw_jit_header_pre_clip_pos(_gallivm, _ptr) \
>> lp_build_struct_get_ptr(_gallivm, _ptr, DRAW_JIT_VERTEX_PRE_CLIP_POS, "pre_clip_pos")
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_pipe_clip.c b/src/gallium/auxiliary/draw/draw_pipe_clip.c
>> index 47765cd..94006fb 100644
>> --- a/src/gallium/auxiliary/draw/draw_pipe_clip.c
>> +++ b/src/gallium/auxiliary/draw/draw_pipe_clip.c
>> @@ -59,6 +59,7 @@ struct clip_stage {
>> struct draw_stage stage; /**< base class */
>>
>> unsigned pos_attr;
>> + int cv_attr;
>>
>> /* List of the attributes to be constant interpolated. */
>> uint num_const_attribs;
>> @@ -150,7 +151,10 @@ static void interp(const struct clip_stage *clip,
>>
>> /* Interpolate the clip-space coords.
>> */
>> - interp_attr(dst->clip, t, in->clip, out->clip);
>> + if (clip->cv_attr >= 0) {
>> + interp_attr(dst->data[clip->cv_attr], t,
>> + in->data[clip->cv_attr], out->data[clip->cv_attr]);
>> + }
>> /* interpolate the clip-space position */
>> interp_attr(dst->pre_clip_pos, t, in->pre_clip_pos, out->pre_clip_pos);
>>
>> @@ -214,9 +218,9 @@ static void interp(const struct clip_stage *clip,
>> * Triangle is considered null/empty if its area is equal to zero.
>> */
>> static inline boolean
>> -is_tri_null(struct draw_context *draw, const struct prim_header *header)
>> +is_tri_null(const struct clip_stage *clip, const struct prim_header *header)
>> {
>> - const unsigned pos_attr = draw_current_shader_position_output(draw);
>> + const unsigned pos_attr = clip->pos_attr;
>> float x1 = header->v[1]->data[pos_attr][0] - header->v[0]->data[pos_attr][0];
>> float y1 = header->v[1]->data[pos_attr][1] - header->v[0]->data[pos_attr][1];
>> float z1 = header->v[1]->data[pos_attr][2] - header->v[0]->data[pos_attr][2];
>> @@ -242,6 +246,7 @@ static void emit_poly(struct draw_stage *stage,
>> unsigned n,
>> const struct prim_header *origPrim)
>> {
>> + const struct clip_stage *clipper = clip_stage(stage);
>> struct prim_header header;
>> unsigned i;
>> ushort edge_first, edge_middle, edge_last;
>> @@ -281,7 +286,7 @@ static void emit_poly(struct draw_stage *stage,
>> header.v[2] = inlist[0]; /* the provoking vertex */
>> }
>>
>> - tri_null = is_tri_null(stage->draw, &header);
>> + tri_null = is_tri_null(clipper, &header);
>> /* If we generated a triangle with an area, aka. non-null triangle,
>> * or if the previous triangle was also null then skip all subsequent
>> * null triangles */
>> @@ -306,11 +311,18 @@ static void emit_poly(struct draw_stage *stage,
>> debug_printf("Clipped tri: (flat-shade-first = %d)\n",
>> stage->draw->rasterizer->flatshade_first);
>> for (j = 0; j < 3; j++) {
>> - debug_printf(" Vert %d: clip: %f %f %f %f\n", j,
>> - header.v[j]->clip[0],
>> - header.v[j]->clip[1],
>> - header.v[j]->clip[2],
>> - header.v[j]->clip[3]);
>> + debug_printf(" Vert %d: pre clip: %f %f %f %f\n", j,
>> + header.v[j]->pre_clip_pos[0],
>> + header.v[j]->pre_clip_pos[1],
>> + header.v[j]->pre_clip_pos[2],
>> + header.v[j]->pre_clip_pos[3]);
>> + if (clipper->cv_attr >= 0) {
>> + debug_printf(" Vert %d: cv: %f %f %f %f\n", j,
>> + header.v[j]->data[clipper->cv_attr][0],
>> + header.v[j]->data[clipper->cv_attr][1],
>> + header.v[j]->data[clipper->cv_attr][2],
>> + header.v[j]->data[clipper->cv_attr][3]);
>> + }
>> for (k = 0; k < draw_num_shader_outputs(stage->draw); k++) {
>> debug_printf(" Vert %d: Attr %d: %f %f %f %f\n", j, k,
>> header.v[j]->data[k][0],
>> @@ -320,7 +332,7 @@ static void emit_poly(struct draw_stage *stage,
>> }
>> }
>> }
>> - stage->next->tri( stage->next, &header );
>> + stage->next->tri(stage->next, &header);
>> }
>> }
>>
>> @@ -353,7 +365,16 @@ static inline float getclipdist(const struct clip_stage *clipper,
>> dp = vert->data[draw_current_shader_clipdistance_output(clipper->stage.draw, cdi)][vidx];
>> } else {
>> plane = clipper->plane[plane_idx];
>> - dp = dot4(vert->clip, plane);
>> + /*
>> + * XXX is it ok to use clip and not pre_clip_pos if plane_idx < 6?
>> + * Does it even matter? Keeping previous behavior.
>> + */
>> + if (clipper->cv_attr >= 0) {
>> + dp = dot4(vert->data[clipper->cv_attr], plane);
>> + }
>> + else {
>> + dp = dot4(vert->pre_clip_pos, plane);
>> + }
>> }
>> return dp;
>> }
>> @@ -400,13 +421,22 @@ do_clip_tri(struct draw_stage *stage,
>> viewport_index = draw_viewport_index(clipper->stage.draw, prov_vertex);
>>
>> if (DEBUG_CLIP) {
>> - const float *v0 = header->v[0]->clip;
>> - const float *v1 = header->v[1]->clip;
>> - const float *v2 = header->v[2]->clip;
>> - debug_printf("Clip triangle:\n");
>> + const float *v0 = header->v[0]->pre_clip_pos;
>> + const float *v1 = header->v[1]->pre_clip_pos;
>> + const float *v2 = header->v[2]->pre_clip_pos;
>> + debug_printf("Pre-Clip triangle:\n");
>> debug_printf(" %f, %f, %f, %f\n", v0[0], v0[1], v0[2], v0[3]);
>> debug_printf(" %f, %f, %f, %f\n", v1[0], v1[1], v1[2], v1[3]);
>> debug_printf(" %f, %f, %f, %f\n", v2[0], v2[1], v2[2], v2[3]);
>> + if (clipper->cv_attr >= 0) {
>> + const float *v0 = header->v[0]->data[clipper->cv_attr];
>> + const float *v1 = header->v[1]->data[clipper->cv_attr];
>> + const float *v2 = header->v[2]->data[clipper->cv_attr];
>> + debug_printf("Clip triangle:\n");
>> + debug_printf(" %f, %f, %f, %f\n", v0[0], v0[1], v0[2], v0[3]);
>> + debug_printf(" %f, %f, %f, %f\n", v1[0], v1[1], v1[2], v1[3]);
>> + debug_printf(" %f, %f, %f, %f\n", v2[0], v2[1], v2[2], v2[3]);
>> + }
>> }
>>
>> /*
>> @@ -555,7 +585,7 @@ do_clip_tri(struct draw_stage *stage,
>>
>> /* Emit the polygon as triangles to the setup stage:
>> */
>> - emit_poly( stage, inlist, inEdges, n, header );
>> + emit_poly(stage, inlist, inEdges, n, header);
>> }
>> }
>>
>> @@ -567,7 +597,7 @@ do_clip_line(struct draw_stage *stage,
>> struct prim_header *header,
>> unsigned clipmask)
>> {
>> - const struct clip_stage *clipper = clip_stage( stage );
>> + 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;
>> @@ -671,9 +701,9 @@ clip_point_guard_xy(struct draw_stage *stage, struct prim_header *header)
>> * automatically). These would usually be captured by depth clip
>> * too but this can be disabled.
>> */
>> - if (header->v[0]->clip[3] <= 0.0f ||
>> - util_is_inf_or_nan(header->v[0]->clip[0]) ||
>> - util_is_inf_or_nan(header->v[0]->clip[1]))
>> + if (header->v[0]->pre_clip_pos[3] <= 0.0f ||
>> + util_is_inf_or_nan(header->v[0]->pre_clip_pos[0]) ||
>> + util_is_inf_or_nan(header->v[0]->pre_clip_pos[1]))
>> return;
>> }
>> stage->next->point(stage->next, header);
>> @@ -781,6 +811,12 @@ clip_init_state(struct draw_stage *stage)
>> int indexed_interp[2];
>>
>> clipper->pos_attr = draw_current_shader_position_output(draw);
>> + if (draw_current_shader_clipvertex_output(draw) != clipper->pos_attr) {
>> + clipper->cv_attr = (int)draw_current_shader_clipvertex_output(draw);
>> + }
>> + else {
>> + clipper->cv_attr = -1;
>> + }
>>
>> /* We need to know for each attribute what kind of interpolation is
>> * done on it (flat, smooth or noperspective). But the information
>> diff --git a/src/gallium/auxiliary/draw/draw_private.h b/src/gallium/auxiliary/draw/draw_private.h
>> index 5584c4a..a4bd093 100644
>> --- a/src/gallium/auxiliary/draw/draw_private.h
>> +++ b/src/gallium/auxiliary/draw/draw_private.h
>> @@ -89,7 +89,6 @@ struct vertex_header {
>> unsigned have_clipdist:1;
>> unsigned vertex_id:16;
>>
>> - float clip[4];
>> float pre_clip_pos[4];
>>
>> /* This will probably become float (*data)[4] soon:
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I&m=HdthFJB6e-xcXgs6PDbc0TBF5uqSMaCciBPdBoNVMEs&s=FIHw6pPOQ_joZZK9R0g261Mgc1slVFVFgAyfv3HV9KA&e=
>
More information about the mesa-dev
mailing list