[Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin <imirkin at alum.mit.edu> writes:

> On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
>>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut <marius.predut at intel.com> wrote:
>>>> Comparison with a signed expression and unsigned value
>>>> is converted to unsigned value, reason for minus value is interpreted
>>>> as a big unsigned value. For this case the "for" loop
>>>> is going into unexpected behavior.
>>>>
>>>> v1: Brian Paul: code style fix.
>>>> v2: Ian Romanick: glDrawArrays(GL_QUADS, 0, (n * 4) + k) fail , k < 4.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
>>>> Signed-off-by: Marius Predut <marius.predut at intel.com>
>>>> ---
>>>>  src/mesa/tnl_dd/t_dd_dmatmp.h | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>>> index 7be3954..f99d977 100644
>>>> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
>>>> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
>>>> @@ -627,6 +627,13 @@ static void TAG(render_quads_verts)( struct gl_context *ctx,
>>>>        LOCAL_VARS;
>>>>        GLuint j;
>>>>
>>>> +      /* Page 18 (page 32 of the PDF) of the OpenGL 2.1 spec says:
>>>> +       * The total number of vertices between Begin and End is 4n + k,
>>>> +       * where 0 ≤ k ≤ 3; if k is not zero, the final k vertices are  ignored.
>>>> +       */
>>>> +      count = (count / 4) * 4;
>>>
>>> Might be just me, but I'd find
>>>
>>> count &= ~0x3
>>>
>>> to be a lot clearer. Don't know if the compiler can make such an
>>> optimization.
>>
>> I think it can if count is unsigned.  Of course, GLsizei is not
>> unsigned.  It is already invalid for count < 0, so your optimization is
>> safe.
>
> Actually count is a GLuint, so you're probably right that the compiler
> can work it out. I definitely have to think about what it's doing
> though, whereas with something like & ~3 it's pretty obvious. Perhaps
> I've been in bit-land too long.
>
>>
>>> However this seems wrong... you're supposed to draw
>>> start..count, so that's the value that has to be div-by-4. Further up,
>>> when there's native quad support, the logic does:
>>
>> I don't think that's right.  Count is the number of vertices, not the
>> index of the last vertex.  Calling
>>
>>     glDrawArrays(GL_QUADS, 47000, 4);
>>
>> still draws one quad.
>>
>> Look at the pseudocode on page 28 (page 42 of the PDF) of the OpenGL 2.1
>> spec:
>>
>>     "The command
>>
>>         void DrawArrays(enum mode, int first, sizei count);
>>
>>     constructs a sequence of geometric primitives using elements first
>>     through first + count − 1 of each enabled array. mode specifies
>>     what kind of primitives are constructed; it accepts the same token
>>     values as the mode parameter of the Begin command. The effect of
>>
>>         DrawArrays(mode, first, count);
>>
>>     is the same as the effect of the command sequence
>>
>>         if (mode or count is invalid)
>>             generate appropriate error
>>         else {
>>             Begin(mode);
>>             for (int i = 0; i < count; i++)
>>                 ArrayElement(first + i);
>>             End();
>>         }"
>>
>> Combining that with the language previously quoted, I think this change
>> is right.
>
> Well, the code in question is
>
>       for (j = start; j < count-3; j += 4) {
>          void *tmp = ALLOC_VERTS( 6 );
>          /* Send v0, v1, v3
>           */
>          tmp = EMIT_VERTS(ctx, j,     2, tmp);
>          tmp = EMIT_VERTS(ctx, j + 3, 1, tmp);
>          /* Send v1, v2, v3
>           */
>          tmp = EMIT_VERTS(ctx, j + 1, 3, tmp);
>          (void) tmp;
>       }
>
> If count worked the way you're suggesting, then this would never work
> for start != 0. I think "count" is really "end" in this case. Here is
> one of the callers of this function:
>
> tab[prim & PRIM_MODE_MASK]( ctx, start, start + length, prim );
>
> The fact that the variable is called 'count' is actively misleading of
> course, but that doesn't make the code any more right. The HAVE_QUADS
> and HAVE_ELTS cases both have:
>
>       count -= (count-start)%4;
>
> which I believe further confirms my analysis.

So we have three main branches and one failure branch, and all three
main branches tries to do the exact same thing (looping from start to
count in steps of 4). Maybe the common stuff should be moved out of the
branches?

(Actually, the first two also differ in their style of setting dmasz and
currentsz to a multiple of 4. Maybe they should be made consistent for
easier reading?)

eirik