[Mesa-dev] [PATCH] util/primconvert: Avoid point arithmetic; apply offset on all cases.

[Ilia Mirkin]

On Fri, Dec 5, 2014 at 9:29 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 05/12/14 14:18, Ilia Mirkin wrote:
>>
>> On Fri, Dec 5, 2014 at 9:16 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>>>
>>> From: José Fonseca <jfonseca at vmware.com>
>>>
>>> Matches what u_vbuf_get_minmax_index() does.
>>
>>
>> Hm, nouveau nv50 (and probably nvc0) does:
>>
>>        if (ib->buffer) {
>>           nv50->idxbuf.offset = ib->offset;
>>           BCTX_REFN(nv50->bufctx_3d, INDEX, nv04_resource(ib->buffer),
>> RD);
>>        } else {
>>           nv50->idxbuf.user_buffer = ib->user_buffer;
>>        }
>>
>> Is the offset really supposed to be applied to the user buffer?
>
>
> Nouveau is the odd ball here, as softpipe/llvmpipe apply it:
>
>   src/gallium/drivers/llvmpipe/lp_draw_arrays.c
>   src/gallium/drivers/softpipe/sp_draw_arrays.c
>
>> I
>> figured the point of the offset was that you couldn't provide an
>> offset into a pipe_resource otherwise, while with a user ptr, you can
>> just add it in yourself...
>
> True.  Still it's better to apply these things uniformly, because the user
> vs buffer pointer is a distinction which rapidly disappears inside a pipe
> driver. (E.g, the driver can promote the user pointer to a buffer
> internally, and then the information of whether offset should or not be
> taken in account gets lost)

Definitely agree that it's better to apply things uniformly. Just
wasn't sure if "uniformly" meant "fix u_vbuf" or "fix nouveau" :)

FWIW, st/mesa does, in setup_index_buffer:

   if (_mesa_is_bufferobj(bufobj)) {
      /* indices are in a real VBO */
      ibuffer->buffer = st_buffer_object(bufobj)->buffer;
      ibuffer->offset = pointer_to_offset(ib->ptr);
   }
   else {
      /* indices are in user space memory */
      ibuffer->user_buffer = ib->ptr;
   }

But the ibuffer is zero-initialized beforehand. So this discussion is
largely academic. I'm happy to decree that ib->offset shall be applied
to the user_buffer as well and go around fixing nouveau and anyone
else coming in its path.

So... this change Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

Cheers,

  -ilia

>
> Jose
>
>
>>> ---
>>>   src/gallium/auxiliary/indices/u_primconvert.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/indices/u_primconvert.c
>>> b/src/gallium/auxiliary/indices/u_primconvert.c
>>> index 4632781..eba1f9e 100644
>>> --- a/src/gallium/auxiliary/indices/u_primconvert.c
>>> +++ b/src/gallium/auxiliary/indices/u_primconvert.c
>>> @@ -137,8 +137,9 @@ util_primconvert_draw_vbo(struct primconvert_context
>>> *pc,
>>>         src = ib->user_buffer;
>>>         if (!src) {
>>>            src = pipe_buffer_map(pc->pipe, ib->buffer,
>>> -                               PIPE_TRANSFER_READ, &src_transfer) +
>>> ib->offset;
>>> +                               PIPE_TRANSFER_READ, &src_transfer);
>>>         }
>>> +      src = (const uint8_t *)src + ib->offset;
>>>      }
>>>      else {
>>>         u_index_generator(pc->primtypes_mask,
>>> --
>>> 2.1.0
>>>
>