[Mesa-dev] [PATCH 5/5] st/mesa: clean up min/max_index handling in st_draw_vbo

[Marek Olšák]

On Tue, Apr 25, 2017 at 8:41 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 24.04.2017 15:31, Marek Olšák wrote:
>>
>> On Mon, Apr 24, 2017 at 12:01 PM, Nicolai Hähnle <nhaehnle at gmail.com>
>> wrote:
>>>
>>> On 23.04.2017 01:10, Marek Olšák wrote:
>>>>
>>>>
>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> There is no reason to check for ~0.
>>>> Also remove the incorrect comment.
>>>> ---
>>>>  src/mesa/state_tracker/st_draw.c | 9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_draw.c
>>>> b/src/mesa/state_tracker/st_draw.c
>>>> index d710284..e510d43 100644
>>>> --- a/src/mesa/state_tracker/st_draw.c
>>>> +++ b/src/mesa/state_tracker/st_draw.c
>>>> @@ -200,28 +200,23 @@ st_draw_vbo(struct gl_context *ctx,
>>>>     if (ib) {
>>>>        /* Get index bounds for user buffers. */
>>>>        if (!index_bounds_valid)
>>>>           if (!all_varyings_in_vbos(arrays))
>>>>              vbo_get_minmax_indices(ctx, prims, ib, &min_index,
>>>> &max_index,
>>>>                                     nr_prims);
>>>>
>>>>        setup_index_buffer(st, ib);
>>>>
>>>>        info.indexed = TRUE;
>>>> -      if (min_index != ~0U && max_index != ~0U) {
>>>> -         info.min_index = min_index;
>>>> -         info.max_index = max_index;
>>>> -      }
>>>> +      info.min_index = min_index;
>>>> +      info.max_index = max_index;
>>>
>>>
>>>
>>> For this and the previous patch, I think it would be cleaner to keep the
>>> semantics of pipe_draw_info::min_index/max_index to always make sense.
>>> This
>>> would mean dropping the previous patch, and changing this patch to only
>>> set
>>> the min_index/max_index if index_bounds_valid (rather than the ~0U
>>> check).
>>
>>
>> The old code didn't check index_bounds_valid, and u_vbuf already
>> treats max_index == ~0 as invalid range. Instead of starting to use
>> index_bounds_valid here, I'd prefer it if we removed
>> index_bounds_valid and only rely on max_index == ~0 from vbo.
>
>
> Hmm. Legitimate and valid max_index == ~0 is definitely possible on 64-bit
> systems. It's ridiculously unlikely in practice though, so I guess it's
> worth considering this change in the name of eliminating a branch here.
>
> That said, it looks like some drivers are actually using max_index, so at
> least it needs to be checked that this change is okay for them.

It's not necessary if vbo sets min=0 max=~0, which is what
util_draw_init_info does.

Marek