[Mesa-dev] [PATCH] draw: fix line stippling with unfilled prims

[Roland Scheidegger]

Am 07.01.2016 um 16:48 schrieb Jose Fonseca:
> On 06/01/16 22:26, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> The unfilled stage was not filling in the prim header, and the line stage
>> then decided to reset the stipple counter or not based on the
>> uninitialized
>> data. This causes some failures in conform linestipple test (albeit quite
>> randomly happening depending on environment).
>> So fill in the prim header in the unfilled stage - I am not entirely sure
>> if anybody really needs determinant after that stage, but there's at
>> least
>> later stages (wide line for instance) which copy over the determinant
>> as well.
>> ---
>>   src/gallium/auxiliary/draw/draw_pipe_unfilled.c | 56
>> +++++++++++++++++--------
>>   1 file changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
>> b/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
>> index 8e6435c..b9ded14 100644
>> --- a/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
>> +++ b/src/gallium/auxiliary/draw/draw_pipe_unfilled.c
>> @@ -86,27 +86,33 @@ inject_front_face_info(struct draw_stage *stage,
>>   }
>>
>>
>> -static void point( struct draw_stage *stage,
>> -           struct vertex_header *v0 )
>> +static void point(struct draw_stage *stage,
>> +                  struct prim_header *header,
>> +                  struct vertex_header *v0)
>>   {
>>      struct prim_header tmp;
>> +   tmp.det = header->det;
>> +   tmp.flags = 0;
>>      tmp.v[0] = v0;
>> -   stage->next->point( stage->next, &tmp );
>> +   stage->next->point(stage->next, &tmp);
>>   }
>>
>> -static void line( struct draw_stage *stage,
>> -          struct vertex_header *v0,
>> -          struct vertex_header *v1 )
>> +static void line(struct draw_stage *stage,
>> +                 struct prim_header *header,
>> +                 struct vertex_header *v0,
>> +                 struct vertex_header *v1)
>>   {
>>      struct prim_header tmp;
>> +   tmp.det = header->det;
>> +   tmp.flags = 0;
>>      tmp.v[0] = v0;
>>      tmp.v[1] = v1;
>> -   stage->next->line( stage->next, &tmp );
>> +   stage->next->line(stage->next, &tmp);
>>   }
>>
>>
>> -static void points( struct draw_stage *stage,
>> -            struct prim_header *header )
>> +static void points(struct draw_stage *stage,
>> +                   struct prim_header *header)
>>   {
>>      struct vertex_header *v0 = header->v[0];
>>      struct vertex_header *v1 = header->v[1];
>> @@ -114,27 +120,41 @@ static void points( struct draw_stage *stage,
>>
>>      inject_front_face_info(stage, header);
>>
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_0) && v0->edgeflag)
>> point( stage, v0 );
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_1) && v1->edgeflag)
>> point( stage, v1 );
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_2) && v2->edgeflag)
>> point( stage, v2 );
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_0) && v0->edgeflag)
>> +      point(stage, header, v0);
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_1) && v1->edgeflag)
>> +      point(stage, header, v1);
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_2) && v2->edgeflag)
>> +      point(stage, header, v2);
>>   }
>>
>>
>> -static void lines( struct draw_stage *stage,
>> -           struct prim_header *header )
>> +static void lines(struct draw_stage *stage,
>> +                  struct prim_header *header)
>>   {
>>      struct vertex_header *v0 = header->v[0];
>>      struct vertex_header *v1 = header->v[1];
>>      struct vertex_header *v2 = header->v[2];
>>
>>      if (header->flags & DRAW_PIPE_RESET_STIPPLE)
>> -      stage->next->reset_stipple_counter( stage->next );
>> +      /*
>> +       * XXX could revisit this. The only stage which cares is the line
>> +       * stipple stage. Could just emit correct reset flags here and not
>> +       * bother about all the calling through reset_stipple_counter
>> +       * stages. Though technically it is necessary if line stipple is
>> +       * handled by the driver, but this is not actually hooked up when
>> +       * using vbuf (vbuf stage reset_stipple_counter does nothing).
>> +       */
>> +      stage->next->reset_stipple_counter(stage->next);
>>
>>      inject_front_face_info(stage, header);
>>
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_2) && v2->edgeflag) line(
>> stage, v2, v0 );
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_0) && v0->edgeflag) line(
>> stage, v0, v1 );
>> -   if ((header->flags & DRAW_PIPE_EDGE_FLAG_1) && v1->edgeflag) line(
>> stage, v1, v2 );
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_2) && v2->edgeflag)
>> +      line(stage, header, v2, v0);
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_0) && v0->edgeflag)
>> +      line(stage, header, v0, v1);
>> +   if ((header->flags & DRAW_PIPE_EDGE_FLAG_1) && v1->edgeflag)
>> +      line(stage, header, v1, v2);
>>   }
>>
>>
>>
> 
> LGTM.
> 
> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
> 
> But doing `git grep -A10 '\s\+struct prim_header \w\+'` highlights a few
> other potential places that need initialization of flags (and sometimes
> det):
> 
> - src/gallium/auxiliary/draw/draw_pipe_aaline.c
> - src/gallium/auxiliary/draw/draw_pipe_aapoint.c
> - src/gallium/auxiliary/draw/draw_pipe_wide_line.c
> - src/gallium/auxiliary/draw/draw_pipe_wide_point.c

I think the assumption here is that later stages just don't need det or
flags. These are the last 4 stages (apart from vbuf, which doesn't need
them neither), and none of them use either flags nor det themselves.
Albeit that looks a bit brittle...


> - src/gallium/auxiliary/draw/draw_pipe_clip.c:
This one looks genuinely broken. Clipped lines are going to have
uninitialized flags.
I am not quite sure though how clipping affects reset of line stippling
(or even line stippling in general). Is the stipple counter supposed to
count the clipped length only (obviously, wouldn't work with guard
band...)? I suppose in that case should just copy over the flags from
the original prim. Otherwise, I have no idea how that could potentially
be handled correctly.

Note though that we actually will copy over det in cases where we don't
have initialized it (if the prim was a line, point). Albeit still
shouldn't going to need it in that case later...

Roland