[Mesa-dev] [PATCH 1/5] include/GL: add mesa_glinterop.h for OpenGL-OpenCL interop (v3)

[Marek Olšák]

On Sat, Mar 12, 2016 at 12:35 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 9 March 2016 at 18:36, Marek Olšák <maraeo at gmail.com> wrote:
>> On Wed, Mar 9, 2016 at 6:51 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 9 March 2016 at 17:11, Marek Olšák <maraeo at gmail.com> wrote:
>>>> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> On 8 March 2016 at 22:29, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>
>>>>>> Actually, I don't see how the version number would make it any better
>>>>>> for the structures, but returning the version number by
>>>>>> QueryDeviceInfo would be useful for the caller to know what to expect
>>>>>> if Mesa version < caller version. The sizes are still useful if Mesa
>>>>>> version > caller version.
>>>>>>
>>>>> If any of this is an issue, then the whole DRI model just won't work ;-)
>>>>
>>>> The DRI extension versions only determine the number of function
>>>> callbacks, not function parameters and return values. This interop
>>>> thing is a lot more complicated than that, since it allows the "in"
>>>> and "out" structures to grow, and different rules apply to each.
>>> The fact that the DRI extension version is used only to determine the
>>> presence of certain function, is implementation detail imho.
>>>
>>> If you look at the struct as a whole, it doesn't matter what the
>>> contents (part the version field) are. For all one care there could be
>>> none ?
>>>
>>>> Also,
>>>> the implementation of DRI extensions allocates the structures, while
>>>> in the interop the user allocates the structures. It's a totally
>>>> different model.
>>>>
>>> True, it's a slightly different model. The following should still work ?
>>>
>>> Library A: allocates memory for the struct, and set the currently
>>> maximum supported version
>>> Library B: retrieves the 'empty' struct. Checks which version is lower
>>> (self and the one in the struct) and only populates for up-to that
>>> version.
>>>
>>>>>
>>>>> I'm thinking that the following should work. Please let me know if I'm
>>>>> loosing the plot.
>>>>>
>>>>> Caller sets the structure and sets version of the interface it
>>>>> provides. Then callee first checks if it can work with the provider
>>>>> version. then proceeds as planned.
>>>>
>>>> The callee must always proceed even if the version is too low or too
>>>> high.
>>> What do you mean with "proceed" here ?
>>>
>>> Are you saying that even if the callee can work with up-to v2, it
>>> should somehow populate the v3 fields ?
>>> Shouldn't it use the lowest version of the two and only handle those fields ?
>>
>> A v1 callee can only read and write v1 fields.
>> A v2 callee can only read and write v2 fields, but will only write v1
>> fields if the size doesn't include v2 fields.
>> I could go on.
>>
>> The callee doesn't care about the caller's version at all.
>>
> With the last sentence removed, that's roughly what I was saying.
>
> The library that allocates the struct, sets the layout version ('size'
> if you prefer). Then both libraries can read/write fields up-to the
> smaller version (size) between the one already set in the struct and
> the one they support.

Yeah, that's what the code does with the exception that v5 uses
"interop_version" returned by QueryDeviceInfo instead of a size.

> To make is more prominent one can explicitly
> mark the version as const and add a small comment that only the
> function which allocates the struct is responsible for setting the
> variable. This last sentence also applies for 'size' fwiw.

Well, the sizes are already function parameters, not structure members.

>
> Out of curiosity: did you draw some inspiration about using 'size'
> based on some other interfaces ? I'm sorry but they serve no benefit,
> plus the implementation requires some nasty code to work with them.

"offset_after" isn't nasty. :) Yeah, I was inspired by other APIs I
had seen. The sizes make even more sense when they are function
parameters, because the caller can just do:
(sizeof(in), &in, sizeof(out), &out)

>
>>>
>>>> The caller knows the Mesa interop version from QueryDeviceInfo
>>>> (defined in v4 and later) and knows what it should expect. Mesa only
>>>> checks the sizes, but otherwise doesn't care about the caller's
>>>> version.
>>>>
>>>>> Passing around multiple sizes is ugly and error prone to a point. One
>>>>> gets the order wrong (swaps in_size and out_size) or just the same
>>>>> sizeof(struct foo) in both places.
>>>>
>>>> The order of parameters in v5 is: in_size, *in, out_size, *out
>>>
>>>> If the implementation swaps in_size and out_size, it's no my problem.
>>> No offence, but you realise that this sounds rather funny, right ?
>>> In the context of designing interfaces, of course.
>>
>> It's not my problem for the same reason that swapping src and dst in
>> memcpy is not glibc's problem. We have to draw the line somewhere. :)
>>
> Was talking about the sizes, not the in/out pointers. There is a
> subtle difference with the possible side-falls and their
> detection/prevention.

We should catch such bugs with tests.

Marek