On Thu, Dec 17, 2015 at 11:19 AM, Roland Scheidegger <srol...@vmware.com> wrote:
> Am 17.12.2015 um 19:13 schrieb Brian Paul:
>> On 12/17/2015 09:58 AM, srol...@vmware.com wrote:
>>> From: Roland Scheidegger <srol...@vmware.com>
>>>
>>> NaNs mean it should be clipped, otherwise the NaNs might get passed to
>>> the
>>> next stages (if clipping didn't happen for another reason already), which
>>> might cause all kind of problems.
>>> The llvm path got this right already (possibly by luck), but this
>>> isn't used
>>> when there's a gs active.
>>> Found by code inspection, verified with some hacked piglit test and
>>> some more
>>> hacked debug output.
>>> (Note the clipper can still itself incorrectly generate NaN and INF
>>> position
>>> values in its output prims (at least after w divide / viewport
>>> transform) even
>>> if the inputs weren't NaNs, if the position data of the vertices is
>>> "sufficiently bad".)
>>> ---
>>> src/gallium/auxiliary/draw/draw_cliptest_tmp.h | 28
>>> +++++++++++++-------------
>>> src/gallium/auxiliary/draw/draw_llvm.c | 4 ++++
>>> 2 files changed, 18 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>>> b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>>> index 779b237..d8866cd 100644
>>> --- a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>>> +++ b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>>> @@ -95,30 +95,31 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>>> *pvs,
>>> out->pre_clip_pos[i] = position[i];
>>> }
>>>
>>> + /* Be careful with NaNs. Comparisons must be true for them. */
>>> /* Do the hardwired planes first:
>>> */
>>> if (flags & DO_CLIP_XY_GUARD_BAND) {
>>> - if (-0.50 * position[0] + position[3] < 0) mask |= (1<<0);
>>> - if ( 0.50 * position[0] + position[3] < 0) mask |= (1<<1);
>>> - if (-0.50 * position[1] + position[3] < 0) mask |= (1<<2);
>>> - if ( 0.50 * position[1] + position[3] < 0) mask |= (1<<3);
>>> + if (!(-0.50 * position[0] + position[3] >= 0)) mask |=
>>> (1<<0);
>>> + if (!( 0.50 * position[0] + position[3] >= 0)) mask |=
>>> (1<<1);
>>> + if (!(-0.50 * position[1] + position[3] >= 0)) mask |=
>>> (1<<2);
>>> + if (!( 0.50 * position[1] + position[3] >= 0)) mask |=
>>> (1<<3);
>>> }
>>> else if (flags & DO_CLIP_XY) {
>>> - if (-position[0] + position[3] < 0) mask |= (1<<0);
>>> - if ( position[0] + position[3] < 0) mask |= (1<<1);
>>> - if (-position[1] + position[3] < 0) mask |= (1<<2);
>>> - if ( position[1] + position[3] < 0) mask |= (1<<3);
>>> + if (!(-position[0] + position[3] >= 0)) mask |= (1<<0);
>>> + if (!( position[0] + position[3] >= 0)) mask |= (1<<1);
>>> + if (!(-position[1] + position[3] >= 0)) mask |= (1<<2);
>>> + if (!( position[1] + position[3] >= 0)) mask |= (1<<3);
>>> }
>>>
>>> /* Clip Z planes according to full cube, half cube or none.
>>> */
>>> if (flags & DO_CLIP_FULL_Z) {
>>> - if ( position[2] + position[3] < 0) mask |= (1<<4);
>>> - if (-position[2] + position[3] < 0) mask |= (1<<5);
>>> + if (!( position[2] + position[3] >= 0)) mask |= (1<<4);
>>> + if (!(-position[2] + position[3] >= 0)) mask |= (1<<5);
>>> }
>>> else if (flags & DO_CLIP_HALF_Z) {
>>> - if ( position[2] < 0) mask |= (1<<4);
>>> - if (-position[2] + position[3] < 0) mask |= (1<<5);
>>> + if (!( position[2] >= 0)) mask |= (1<<4);
>>> + if (!(-position[2] + position[3] >= 0)) mask |= (1<<5);
>>> }
>>>
>>> if (flags & DO_CLIP_USER) {
>>> @@ -146,7 +147,7 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>>> *pvs,
>>> if (clipdist < 0 || util_is_inf_or_nan(clipdist))
>>> mask |= 1 << plane_idx;
>>> } else {
>>> - if (dot4(clipvertex, plane[plane_idx]) < 0)
>>> + if (!(dot4(clipvertex, plane[plane_idx]) >= 0))
>>> mask |= 1 << plane_idx;
>>> }
>>> }
>>> @@ -192,7 +193,6 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>>> *pvs,
>>>
>>> out = (struct vertex_header *)( (char *)out + info->stride );
>>> }
>>> -
>>> return need_pipeline != 0;
>>> }
>>>
>>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
>>> b/src/gallium/auxiliary/draw/draw_llvm.c
>>> index 8435991..dad523a 100644
>>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>>> @@ -1200,6 +1200,10 @@ generate_clipmask(struct draw_llvm *llvm,
>>> cv_w = pos_w;
>>> }
>>>
>>> + /*
>>> + * Be careful with the comparisons and NaNs (using llvm's unordered
>>> + * comparisons here).
>>> + */
>>> /* Cliptest, for hardwired planes */
>>> if (clip_xy) {
>>> /* plane 1 */
>>>
>>
>> Looks OK to me, but I'm not sure I understand what'll happen when we
>> find a vertex with a NaN coordinate. Does the whole triangle get
>> culled? Otherwise, what would be the result of computing the
>> intersection point on the clip plane?
>>
>> Reviewed-by: Brian Paul <bri...@vmware.com>
>>
>
> If we set the clip mask bit when there's a NaN, this simply ensures
> we're entering the clip stage (and the clip stage will also actually
> perform clipping against the affected clip plane).
> We won't actually cull such a tri directly, but the math in the clip
> stage should ensure it will eventually be discarded. Because
> getclipdist() then should return a NaN too, which will cause the code to
> immediately return without emitting the tri (or any already generated
> tri due to clipping with other planes) immediately. As far as I can see,
> this logic should work reliably (the nans cannot disappear even if we
> performed some math on them due to clipping with other planes).
> There's more bugs in the clip stage but I don't quite understand it.
> Though I spotted some issue which could be problematic, there's some
> DIFFERENT_SIGNS logic (which is also used to avoid division by zero)
> which doesn't look bullet-proof. This is defined as:
> DIFFERENT_SIGNS(x, y) ((x) * (y) <= 0.0F && (x) - (y) != 0.0F)
> But for sufficiently small x and y (in the order of 2^-63) this will be
> true even if both x and y have the same sign (as the product would be
> zero). But there's probably more to it which causes it to generate bogus
> tris in the end...
I changed the implementation of DIFFERENT_SIGNS in src/mesa/main/macros.h to be
return signbit(x) != signbit(y);
a while ago. Perhaps you could just pull that into draw_pipe_clip.c?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
