[Mesa-dev] [PATCH v2 3/3] mapi/glthread: generate asynchronous code for PBO transfer

[Nicolai Hähnle]

On 13.04.2017 18:39, gregory hainaut wrote:
> On Thu, 13 Apr 2017 18:31:06 +0200
> Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>
>> On 05.04.2017 12:30, Gregory Hainaut wrote:
>>>              # Classify fixed and variable parameters.
>>>              self.fixed_params = []
>>>              self.variable_params = []
>>>              for p in self.parameters:
>>>                  if p.is_padding:
>>>                      continue
>>>     -            if p.is_variable_length():
>>>     +            if self.marshal == "upbo" and p.is_pointer():
>>>     +                # Pixel buffer transfer API is tricky. By default
>>> it contains
>>>     +                # a pointer to user memory so a variable length parameter.
>>>     +                # When a pixel buffer is bound, the pointer
>>> becomes an offset.
>>>     +                #
>>>     +                # Non-PBO transfer will be synchronous so
>>> parameter type isn't
>>>     +                # important. PBO transfer will be asynchronous so
>>> the parameter
>>>     +                # must be marked as fixed
>>>     +                self.fixed_params.append(p)
>>>
>>>
>>>> If this is needed for upbo, shouldn't it also be needed for ppbo?
>>>>
>>>> Cheers,
>>>> Nicolai
>>>
>>> Hello Nicolai,
>>>
>>> It isn't symmetrical. In case of UPBO data ought to be copied from app
>>> thread to gl thread. You can see variable_length parameter as input
>>> pointer. Variable length will generate the memcpy code.
>>>
>>> However PPBO will copy from GPU to user pointer. There is no data
>>> associated with the pointer so the pointer isn't "used" by glthread,
>>> only transferred to GL.
>>>
>>> I think the code would love an extra comment.
>>
>> Okay, I get it now. But the comment could maybe use some re-wording,
>> especially because this whole issue only applies to some functions:
>> those where the marshaling code theoretically has a size, such as with
>> glCompressedTexImage2D. It would be good to clarify that.
>
> Yes. I added this comment. Though I'm not sure it enough.
>
> + # variable_params are meaningful when pointers are associated
> + # with a payload hence it only impacts upbo but not ppbo.

I'd rewrite the thing. Here's a suggestion:

   # There are some texture upload functions (the CompressedTexImage
   # family) which provide an imageSize together with the image
   # pointer. The default marshaling behavior is to copy the contents
   # of image to a variable length section, which is incorrect when
   # image refers to a pixel buffer.
   #
   # This ensures that the pointer is marshaled as is. The non-PBO
   # case is always synchronous and so does not use marshaling anyway.

Cheers,
Nicolai
-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.