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

[Ilia Mirkin]

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?

>
> 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? :)

  -ilia