[PATCH 7/9] glamor: Extract the streamed vertex data code used by Render.

[Eric Anholt]

Markus Wick <markus at selfnet.de> writes:

> Am 2014-03-09 05:07, schrieb Eric Anholt:
>> diff --git a/glamor/glamor_vbo.c b/glamor/glamor_vbo.c
>> new file mode 100644
>> index 0000000..be2c2af
>> --- /dev/null
>> +++ b/glamor/glamor_vbo.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * 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.
>> + */
>> +
>> +/**
>> + * @file glamor_vbo.c
>> + *
>> + * Helpers for managing streamed vertex bufffers used in glamor.
>> + */
>> +
>> +#include "glamor_priv.h"
>> +
>> +/** Default size of the VBO, in bytes.
>> + *
>> + * If a single request is larger than this size, we'll resize the VBO
>> + * and return an appropriate mapping, but we'll resize back down after
>> + * that to avoid hogging that memory forever.  We don't anticipate
>> + * normal usage actually requiring larger VBO sizes.
>> + */
>> +#define GLAMOR_VBO_SIZE (64 * 1024)
>
> This is a bit too small imo. iirc glamor was used to split up renderings 
> to 64k vertices, not 64k bytes.
> What is the cache implact on too big buffers? i965 must fall through to 
> LLC, so will it pollute the L1+L2 caches?
> For non-coherent gpus, write combining also shouldn't pollute any 
> caches.

64k seems like plenty, but against a non-bufferstorage Mesa, 512k did
give 1.6% improvement, so I swapped to your value.

>> +
>> +/**
>> + * Returns a pointer to @size bytes of VBO storage, which should be
>> + * accessed by the GL using vbo_offset within the VBO.
>> + */
>> +void *
>> +glamor_get_vbo_space(ScreenPtr screen, unsigned size, char 
>> **vbo_offset)
>> +{
>> +    glamor_screen_private *glamor_priv = 
>> glamor_get_screen_private(screen);
>> +    void *data;
>> +
>> +    glamor_get_context(glamor_priv);
>> +
>> +    glBindBuffer(GL_ARRAY_BUFFER, glamor_priv->vbo);
>> +
>> +    if (glamor_priv->gl_flavor == GLAMOR_GL_DESKTOP) {
>> +        if (glamor_priv->vbo_size < glamor_priv->vbo_offset + size) {
>> +            glamor_priv->vbo_size = MAX(GLAMOR_VBO_SIZE, size);
>> +            glamor_priv->vbo_offset = 0;
>> +            glBufferData(GL_ARRAY_BUFFER,
>> +                         glamor_priv->vbo_size, NULL, GL_STREAM_DRAW);
>> +        }
>> +
>> +        data = glMapBufferRange(GL_ARRAY_BUFFER,
>> +                                glamor_priv->vbo_offset,
>> +                                size,
>> +                                GL_MAP_WRITE_BIT |
>> +                                GL_MAP_UNSYNCHRONIZED_BIT |
>> +                                GL_MAP_INVALIDATE_RANGE_BIT);
>> +        assert(data != NULL);
>> +        *vbo_offset = (void *)(uintptr_t)glamor_priv->vbo_offset;
>
> (char *) instead of (void *).

Changed.

>> +        glamor_priv->vbo_offset += size;
>> +    } else {
>> +        /* Return a pointer to the statically allocated non-VBO
>> +         * memory. We'll upload it through glBufferData() later.
>> +         */
>> +        if (glamor_priv->vbo_size < size) {
>> +            glamor_priv->vbo_size = MAX(GLAMOR_VBO_SIZE, size);
>> +
>> +            glBufferData(GL_ARRAY_BUFFER,
>> +                         glamor_priv->vbo_size, NULL, GL_STREAM_DRAW);
>
> Is this call needed at all? As we upload by glBufferData, we will alloc 
> the buffer then.

Nope, you're right.

>> +
>> +            free(glamor_priv->vb);
>> +            glamor_priv->vb = XNFalloc(size);
>> +        }
>> +        *vbo_offset = NULL;
>> +        glamor_priv->vbo_offset = 0;
>
> This variable is used for the size on uploading. So it must be set to 
> "size".

Good catch.  Fixed (and commented).

>> +void
>> +glamor_put_vbo_space(ScreenPtr screen)
>> +{
>> +    glamor_screen_private *glamor_priv = 
>> glamor_get_screen_private(screen);
>> +
>> +    glamor_get_context(glamor_priv);
>> +
>> +    if (glamor_priv->gl_flavor == GLAMOR_GL_DESKTOP) {
>> +        glUnmapBuffer(GL_ARRAY_BUFFER);
>> +    } else {
>> +        glBufferData(GL_ARRAY_BUFFER, glamor_priv->vbo_offset,
>> +                     glamor_priv->vb, GL_DYNAMIC_DRAW);
>> +    }
>> +
>> +    glBindBuffer(GL_ARRAY_BUFFER, 0);
>> +
>> +    glamor_put_context(glamor_priv);
>> +}
>> +
>> +void
>> +glamor_init_vbo(ScreenPtr screen)
>> +{
>> +    glamor_screen_private *glamor_priv = 
>> glamor_get_screen_private(screen);
>> +
>> +    glamor_get_context(glamor_priv);
>> +
>> +    glGenBuffers(1, &glamor_priv->vbo);
>> +
>> +    glamor_put_context(glamor_priv);
>> +}
>> +
>> +void
>> +glamor_fini_vbo(ScreenPtr screen)
>> +{
>> +    glamor_screen_private *glamor_priv = 
>> glamor_get_screen_private(screen);
>> +
>> +    glamor_get_context(glamor_priv);
>> +
>> +    glDeleteBuffers(1, &glamor_priv->vbo);
>> +    if (glamor_priv->gl_flavor != GLAMOR_GL_DESKTOP)
>> +        free(glamor_priv->vb);
>> +
>> +    glamor_put_context(glamor_priv);
>> +}
>
> I don't like the idea to select the code by desktop vs gles as both 
> don't have to support ARB_mbr.

Yeah, we should definitely extend this to GLES's ARB_mbr in the future.

> Do we have to care about allocing zero bytes? I don't think so, but it's 
> broken right now.

I don't think so.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140310/2b512d89/attachment.pgp>