gl-1.5-vertex-buffer-offsets: test unusual vertex offsets/strides

Submitted by Brian Paul on June 19, 2015, 2:56 p.m.

Details

Message ID 55842D9E.9050707@vmware.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Brian Paul June 19, 2015, 2:56 p.m.
On 06/19/2015 08:52 AM, Ilia Mirkin wrote:
> On Fri, Jun 19, 2015 at 10:48 AM, Brian Paul <brianp@vmware.com> wrote:
>> On 06/19/2015 08:47 AM, Ilia Mirkin wrote:
>>>
>>> On Fri, Jun 19, 2015 at 10:39 AM, Brian Paul <brianp@vmware.com> wrote:
>>>>
>>>> The draw-vertices tests exercises unusual vertex sizes and strides,
>>>> but not with interleaved arrays.
>>>>
>>>> This test creates a VBO with interleaved vertex positions and colors.
>>>> The colors are positioned at offsets like 9, 10 and 11 bytes from the
>>>> start of the vertex.  Unusual strides between vertices are tested too.
>>>>
>>>> Exercises a bug in Gallium's u_vbuf code where the driver was passed
>>>> pipe_vertex_element::src_offset values which weren't a multiple of
>>>> four, even when PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY
>>>> returned 1.
>>>> ---
>>>>    tests/all.py                              |   1 +
>>>>    tests/spec/gl-1.5/CMakeLists.gl.txt       |   1 +
>>>>    tests/spec/gl-1.5/vertex-buffer-offsets.c | 124
>>>> ++++++++++++++++++++++++++++++
>>>>    3 files changed, 126 insertions(+)
>>>>    create mode 100644 tests/spec/gl-1.5/vertex-buffer-offsets.c
>>>>
>>>> diff --git a/tests/all.py b/tests/all.py
>>>> index ad4c494..ee11be6 100755
>>>> --- a/tests/all.py
>>>> +++ b/tests/all.py
>>>> @@ -1042,6 +1042,7 @@ with profile.group_manager(
>>>>          'normal3b3s-invariance-byte', run_concurrent=False)
>>>>        g(['gl-1.5-normal3b3s-invariance', 'GL_SHORT'],
>>>>          'normal3b3s-invariance-short', run_concurrent=False)
>>>> +    g(['gl-1.5-vertex-buffer-offsets'], 'vertex-buffer-offsets')
>>>>
>>>>    with profile.group_manager(
>>>>            PiglitGLTest,
>>>> diff --git a/tests/spec/gl-1.5/CMakeLists.gl.txt
>>>> b/tests/spec/gl-1.5/CMakeLists.gl.txt
>>>> index 8dcd95d..f10c6cb 100644
>>>> --- a/tests/spec/gl-1.5/CMakeLists.gl.txt
>>>> +++ b/tests/spec/gl-1.5/CMakeLists.gl.txt
>>>> @@ -10,5 +10,6 @@ link_libraries (
>>>>    )
>>>>
>>>>    piglit_add_executable (gl-1.5-normal3b3s-invariance
>>>> normal3b3s-invariance.c)
>>>> +piglit_add_executable (gl-1.5-vertex-buffer-offsets
>>>> vertex-buffer-offsets.c)
>>>>
>>>>    # vim: ft=cmake:
>>>> diff --git a/tests/spec/gl-1.5/vertex-buffer-offsets.c
>>>> b/tests/spec/gl-1.5/vertex-buffer-offsets.c
>>>> new file mode 100644
>>>> index 0000000..6d58331
>>>> --- /dev/null
>>>> +++ b/tests/spec/gl-1.5/vertex-buffer-offsets.c
>>>> @@ -0,0 +1,124 @@
>>>> +/*
>>>> + * Copyright 2015  VMware, Inc.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining
>>>> a
>>>> + * copy of this software and associated documentation files (the
>>>> "Software"),
>>>> + * to deal in the Software without restriction, including without
>>>> limitation
>>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>>> sublicense,
>>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>>> + * Software is furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> next
>>>> + * paragraph) shall be included in all copies or substantial portions of
>>>> the
>>>> + * Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>>> EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>> MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>>>> SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>>> OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>>> ARISING
>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>>> + * DEALINGS IN THE SOFTWARE.
>>>> + */
>>>> +
>>>> +/*
>>>> + * Test interleaved vertex arrays with unusual element offsets and
>>>> strides.
>>>> + * Brian Paul
>>>> + */
>>>> +
>>>> +#include "piglit-util-gl.h"
>>>> +
>>>> +PIGLIT_GL_TEST_CONFIG_BEGIN
>>>> +       config.supports_gl_compat_version = 15;
>>>> +       config.window_visual = PIGLIT_GL_VISUAL_RGBA |
>>>> PIGLIT_GL_VISUAL_DOUBLE;
>>>> +PIGLIT_GL_TEST_CONFIG_END
>>>> +
>>>> +
>>>> +void
>>>> +piglit_init(int argc, char **argv)
>>>> +{
>>>> +       /* nothing */
>>>> +}
>>>> +
>>>> +
>>>> +static bool
>>>> +test_offset_stride(int color_offset, int stride)
>>>> +{
>>>> +       static const GLfloat vertex[4][2] = {
>>>> +               { -1, -1 },
>>>> +               {  1, -1 },
>>>> +               {  1,  1 },
>>>> +               { -1,  1 }
>>>> +       };
>>>> +       static const GLfloat color[4] = { 0.0, 1.0, 0.5, 1.0 };
>>>> +       GLubyte buffer[1000];
>>>> +       GLuint buf;
>>>> +       int i, pos;
>>>> +       bool p;
>>>> +
>>>> +       assert(color_offset >= sizeof(vertex[0]));
>>>> +       assert(stride >= color_offset + sizeof(color));
>>>> +
>>>> +       pos = 0;
>>>> +       for (i = 0; i < 4; i++) {
>>>> +               /* copy vertex position into buffer */
>>>> +               memcpy(buffer + pos, vertex[i], sizeof(vertex[i]));
>>>> +
>>>> +               /* copy vertex color into buffer at unusual offset */
>>>> +               memcpy(buffer + pos + color_offset, color,
>>>> sizeof(color));
>>>> +
>>>> +               pos += stride;
>>>> +       }
>>>> +       assert(pos <= sizeof(buffer));
>>>> +
>>>> +       glGenBuffers(1, &buf);
>>>> +       glBindBuffer(GL_ARRAY_BUFFER, buf);
>>>> +       glBufferData(GL_ARRAY_BUFFER, sizeof(buffer),
>>>> +                                        buffer, GL_STATIC_DRAW);
>>>> +
>>>> +       glVertexPointer(2, GL_FLOAT, stride, (void *) 0);
>>>> +       glColorPointer(4, GL_FLOAT, stride, (void *) (size_t)
>>>> color_offset);
>>>> +       glEnable(GL_VERTEX_ARRAY);
>>>> +       glEnable(GL_COLOR_ARRAY);
>>>> +
>>>> +       glClear(GL_COLOR_BUFFER_BIT);
>>>> +       glDrawArrays(GL_TRIANGLE_FAN, 0, 4);
>>>> +
>>>> +       p = piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
>>>> color);
>>>> +
>>>> +       if (!p) {
>>>> +               printf("failure for color_offset %d, stride %d\n",
>>>> +                      color_offset, stride);
>>>> +       }
>>>> +
>>>> +       piglit_present_results();
>>>> +
>>>> +       glDeleteBuffers(1, &buf);
>>>> +
>>>> +       return p;
>>>> +}
>>>> +
>>>> +
>>>> +enum piglit_result
>>>> +piglit_display(void)
>>>> +{
>>>> +       bool pass = true;
>>>> +
>>>> +       /* test nice values */
>>>> +       pass = test_offset_stride(8, 24) && pass;
>>>> +       pass = test_offset_stride(12, 28) && pass;
>>>> +
>>>> +       /* test unual offset */
>>>
>>>
>>> unusual is the usual spelling
>>
>>
>> Fixed.  R-b?
>
> Sorry, don't know enough about vertex buffers =/ The test does seem to
> pass on nouveau (which doesn't use u_vbuf) and llvmpipe/softpipe,
> which I'm guessing both have 1-byte alignment requirement.

vbuf is used via CSO so drivers don't have to worry about it.

I tested it with our in-house driver code (full piglit run) and also 
with softpipe by returning 
PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY=1 and adding some 
assertions:

sp_velems_state));
     if (velems) {
        velems->count = count;


I'll wait for another review.  Thanks.

-Brian

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/softpipe/sp_screen.c 
b/src/gallium/drivers/softpipe/sp_screen.c
index efa46f3..94a4ff0 100644
--- a/src/gallium/drivers/softpipe/sp_screen.c
+++ b/src/gallium/drivers/softpipe/sp_screen.c
@@ -169,9 +169,10 @@  softpipe_get_param(struct pipe_screen *screen, enum 
pipe_cap param)
     case PIPE_CAP_TGSI_CAN_COMPACT_CONSTANTS:
     case PIPE_CAP_VERTEX_BUFFER_OFFSET_4BYTE_ALIGNED_ONLY:
     case PIPE_CAP_VERTEX_BUFFER_STRIDE_4BYTE_ALIGNED_ONLY:
-   case PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY:
     case PIPE_CAP_TEXTURE_MULTISAMPLE:
        return 0;
+   case PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY:
+      return 1;
     case PIPE_CAP_MIN_MAP_BUFFER_ALIGNMENT:
        return 64;
     case PIPE_CAP_QUERY_TIMESTAMP:
diff --git a/src/gallium/drivers/softpipe/sp_state_vertex.c 
b/src/gallium/drivers/softpipe/sp_state_vertex.c
index 48c8d2c..d5e73aa 100644
--- a/src/gallium/drivers/softpipe/sp_state_vertex.c
+++ b/src/gallium/drivers/softpipe/sp_state_vertex.c
@@ -45,7 +45,13 @@  softpipe_create_vertex_elements_state(struct 
pipe_context *pipe,
                                        const struct pipe_vertex_element 
*attribs)
  {
     struct sp_velems_state *velems;
+   unsigned i;
+
     assert(count <= PIPE_MAX_ATTRIBS);
+   for (i = 0; i < count; i++) {
+      assert(attribs[i].src_offset % 4 == 0);
+   }
+
     velems = (struct sp_velems_state *) MALLOC(sizeof(struct 

Comments

On Fri, Jun 19, 2015 at 10:56 AM, Brian Paul <brianp@vmware.com> wrote:
> On 06/19/2015 08:52 AM, Ilia Mirkin wrote:
>>
>> On Fri, Jun 19, 2015 at 10:48 AM, Brian Paul <brianp@vmware.com> wrote:
> vbuf is used via CSO so drivers don't have to worry about it.

Right, but nouveau is one of the few (only?) drivers to report
PIPE_CAP_USER_VERTEX_BUFFERS, and it reports 0 for all the 4byte
things, so vbuf doesn't get installed.

>
> I tested it with our in-house driver code (full piglit run) and also with
> softpipe by returning
> PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY=1 and adding some
> assertions:
>
> diff --git a/src/gallium/drivers/softpipe/sp_screen.c
> b/src/gallium/drivers/softpipe/sp_screen.c
> index efa46f3..94a4ff0 100644
> --- a/src/gallium/drivers/softpipe/sp_screen.c
> +++ b/src/gallium/drivers/softpipe/sp_screen.c
> @@ -169,9 +169,10 @@ softpipe_get_param(struct pipe_screen *screen, enum
> pipe_cap param)
>     case PIPE_CAP_TGSI_CAN_COMPACT_CONSTANTS:
>     case PIPE_CAP_VERTEX_BUFFER_OFFSET_4BYTE_ALIGNED_ONLY:
>     case PIPE_CAP_VERTEX_BUFFER_STRIDE_4BYTE_ALIGNED_ONLY:
> -   case PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY:
>     case PIPE_CAP_TEXTURE_MULTISAMPLE:
>        return 0;
> +   case PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY:
> +      return 1;
>     case PIPE_CAP_MIN_MAP_BUFFER_ALIGNMENT:
>        return 64;
>     case PIPE_CAP_QUERY_TIMESTAMP:
> diff --git a/src/gallium/drivers/softpipe/sp_state_vertex.c
> b/src/gallium/drivers/softpipe/sp_state_vertex.c
> index 48c8d2c..d5e73aa 100644
> --- a/src/gallium/drivers/softpipe/sp_state_vertex.c
> +++ b/src/gallium/drivers/softpipe/sp_state_vertex.c
> @@ -45,7 +45,13 @@ softpipe_create_vertex_elements_state(struct pipe_context
> *pipe,
>                                        const struct pipe_vertex_element
> *attribs)
>  {
>     struct sp_velems_state *velems;
> +   unsigned i;
> +
>     assert(count <= PIPE_MAX_ATTRIBS);
> +   for (i = 0; i < count; i++) {
> +      assert(attribs[i].src_offset % 4 == 0);
> +   }
> +
>     velems = (struct sp_velems_state *) MALLOC(sizeof(struct
> sp_velems_state));
>     if (velems) {
>        velems->count = count;
>
>
> I'll wait for another review.  Thanks.
>
> -Brian
>