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

[Ian Romanick]

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.

> 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.

>       /* Emit whole number of quads in total.  dmasz is already a multiple
>        * of 4.
>        */
>       count -= (count-start)%4;
> 
> Which seems more accurate.
> 
>> +      if(count == 0) return;
> 
> if (count == 0)
> 
> note the space. That's the style used in all of mesa.
> 
> $ git grep '\sif(' | wc -l
> 1076
> $ git grep '\sif (' | wc -l
> 58071
> 
> I guess a few 'if(' instances snuck through, mostly in src/gallium.
> But the overwhelming majority are of the 'if (' style.
> 
>> +
>>        INIT(GL_TRIANGLES);
>>
>>        for (j = start; j < count-3; j += 4) {
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev