[Mesa-dev] [PATCH 2/6] st/mesa: overhaul vertex setup for clearing, glDrawPixels, glBitmap

[Brian Paul]

On 02/14/2016 10:44 AM, Ilia Mirkin wrote:
> On Sun, Feb 14, 2016 at 9:47 AM, Brian Paul <brianp at vmware.com> wrote:
>> On 02/13/2016 01:03 PM, Ilia Mirkin wrote:
>>>
>>> On Fri, Feb 12, 2016 at 10:43 AM, Brian Paul <brianp at vmware.com> wrote:
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_context.c
>>>> b/src/mesa/state_tracker/st_context.c
>>>> index 9016846..cb2c390 100644
>>>> --- a/src/mesa/state_tracker/st_context.c
>>>> +++ b/src/mesa/state_tracker/st_context.c
>>>> @@ -241,16 +241,23 @@ st_create_context_priv( struct gl_context *ctx,
>>>> struct pipe_context *pipe,
>>>>       else
>>>>          st->internal_target = PIPE_TEXTURE_RECT;
>>>>
>>>> -   /* Vertex element objects used for drawing rectangles for glBitmap,
>>>> -    * glDrawPixels, glClear, etc.
>>>> +   /* Setup vertex element info for 'struct st_util_vertex'.
>>>>        */
>>>> -   for (i = 0; i < ARRAY_SIZE(st->velems_util_draw); i++) {
>>>> -      memset(&st->velems_util_draw[i], 0, sizeof(struct
>>>> pipe_vertex_element));
>>>> -      st->velems_util_draw[i].src_offset = i * 4 * sizeof(float);
>>>> -      st->velems_util_draw[i].instance_divisor = 0;
>>>> -      st->velems_util_draw[i].vertex_buffer_index =
>>>> -            cso_get_aux_vertex_buffer_slot(st->cso_context);
>>>> -      st->velems_util_draw[i].src_format =
>>>> PIPE_FORMAT_R32G32B32A32_FLOAT;
>>>> +   {
>>>> +      const unsigned slot =
>>>> cso_get_aux_vertex_buffer_slot(st->cso_context);
>>>
>>>
>>> Can the aux vertex buffer slot change over time? If so, you need some
>>> logic to update the vertex_buffer_index for these. From what I can
>>> tell it's always 0, not sure what the intention behind it is... seems
>>> like it'll be a very annoying problem to debug down the line should it
>>> ever change. Thoughts?
>>
>>
>> It's hard-wired to zero as you say but I imagine it could be computed by
>> examining the current vertex buffer bindings state to find a free slot such
>> that we might be able to avoid saving/restoring all the vertex buffer
>> bindings.  I believe Marek wrote the code in question.
>>
>> In any case, my patch doesn't change how/where the
>> cso_get_aux_vertex_buffer_slot() function is used.  For now, I could add an
>> assertion that slot==0 to help catch future issues.
>>
>> Sound OK?
>
> My concern is the following sequence:
>
> aux vertex buffer slot == 0;
> initialize vertex elements assuming that aux vertex buffer slot == 0;
> set aux vertex buffer slot = 1;
> bind vbo's based on current aux vertex buffer slot ( == 1);
>
> So I'd rather that assert go where the vertex buffers get bound,
> rather than here. Maybe just assert that vertex_buffer_index ==
> aux_buffer_slot when you bind the vertex elements?

OK.


>
>>
>> Does the series look OK to you overall?
>
> I'm looking through it... but I'm slow at this stuff, since I'm not
> super familiar with all the concepts. Perhaps convince one of your
> coworkers to also have a look? :)

Yes, Jose already did, but I wasn't sure if I should wait for more from 
you.  Thanks.

-Brian