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

[Marek Olšák]

On Mon, Apr 11, 2016 at 8:58 PM, Emil Velikov  wrote:
> On 9 April 2016 at 01:21, Marek Olšák  wrote:
>> On Fri, Apr 1, 2016 at 2:13 PM, Emil Velikov  
>> wrote:
>>> On 16 March 2016 at 10:40, Marek Olšák  wrote:
>>>
 "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), , sizeof(out), )

>>> A nice list of arguments:
>>>  - If the majority of people like offset_after, the question "Why
>>> barely any projects use it (from a quick search) ?" comes to mind.
>>>  - I wasn't the only one advocating for versioned interfaces ;-)
>>>  - They will just work in an identical way and the code will be less.
>>>  - Regardless of how ugly/nasty/etc, mesa uses versioned interfaces
>>> throughout. "Consistency is the key" a wise man have said once.
>>>  - The interfaces using explicit size argument, that I'm aware of, are
>>> not designed with extensibility in mind.
>>
>> Updated branch with your suggestions applied:
>> https://cgit.freedesktop.org/~mareko/mesa/log/?h=interop
>>
> Thank you very much.
>
> Hopefully, some of the arguments made sense, as opposed to doing this
> just to make me happy (read 'shut me up').
>
> From a quick look:
>  - Food for thought: An feeling about "can we get away without
> including glx.h and EGL.h in mesa_glinterop.h" kicks in. As-is
> specifics from one winsys are leaked while building the other and
> vice-versa. I'm wondering about incomplete/forward declaration of the
> needed types, although that might be a bad idea ?

Maybe. I can't really make forward declarations if I don't know if
they are pointers to structs, pointers to void, or int handles.
Suggestions welcome.

>  - The struct_version checks seem off -> they are == 0, while they
> should be < 1. The idea is for newer versions to be backward
> compatible, correct ?

Version 0 isn't allowed. The first supported version is 1.

It is backward compatible as far as I can see.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 9 April 2016 at 01:21, Marek Olšák  wrote:
> On Fri, Apr 1, 2016 at 2:13 PM, Emil Velikov  wrote:
>> On 16 March 2016 at 10:40, Marek Olšák  wrote:
>>
>>> "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), , sizeof(out), )
>>>
>> A nice list of arguments:
>>  - If the majority of people like offset_after, the question "Why
>> barely any projects use it (from a quick search) ?" comes to mind.
>>  - I wasn't the only one advocating for versioned interfaces ;-)
>>  - They will just work in an identical way and the code will be less.
>>  - Regardless of how ugly/nasty/etc, mesa uses versioned interfaces
>> throughout. "Consistency is the key" a wise man have said once.
>>  - The interfaces using explicit size argument, that I'm aware of, are
>> not designed with extensibility in mind.
>
> Updated branch with your suggestions applied:
> https://cgit.freedesktop.org/~mareko/mesa/log/?h=interop
>
Thank you very much.

Hopefully, some of the arguments made sense, as opposed to doing this
just to make me happy (read 'shut me up').

From a quick look:
 - Food for thought: An feeling about "can we get away without
including glx.h and EGL.h in mesa_glinterop.h" kicks in. As-is
specifics from one winsys are leaked while building the other and
vice-versa. I'm wondering about incomplete/forward declaration of the
needed types, although that might be a bad idea ?
 - The struct_version checks seem off -> they are == 0, while they
should be < 1. The idea is for newer versions to be backward
compatible, correct ?

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Fri, Apr 1, 2016 at 2:13 PM, Emil Velikov  wrote:
> On 16 March 2016 at 10:40, Marek Olšák  wrote:
>
>> "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), , sizeof(out), )
>>
> A nice list of arguments:
>  - If the majority of people like offset_after, the question "Why
> barely any projects use it (from a quick search) ?" comes to mind.
>  - I wasn't the only one advocating for versioned interfaces ;-)
>  - They will just work in an identical way and the code will be less.
>  - Regardless of how ugly/nasty/etc, mesa uses versioned interfaces
> throughout. "Consistency is the key" a wise man have said once.
>  - The interfaces using explicit size argument, that I'm aware of, are
> not designed with extensibility in mind.

Updated branch with your suggestions applied:
https://cgit.freedesktop.org/~mareko/mesa/log/?h=interop

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 16 March 2016 at 10:40, Marek Olšák  wrote:

> "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), , sizeof(out), )
>
A nice list of arguments:
 - If the majority of people like offset_after, the question "Why
barely any projects use it (from a quick search) ?" comes to mind.
 - I wasn't the only one advocating for versioned interfaces ;-)
 - They will just work in an identical way and the code will be less.
 - Regardless of how ugly/nasty/etc, mesa uses versioned interfaces
throughout. "Consistency is the key" a wise man have said once.
 - The interfaces using explicit size argument, that I'm aware of, are
not designed with extensibility in mind.

If the above are not enough(independently or together) so make you
reconsider, so be it. I won't bother you any more on the topic.

Regards,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  wrote:
> On 9 March 2016 at 18:36, Marek Olšák  wrote:
>> On Wed, Mar 9, 2016 at 6:51 PM, Emil Velikov  
>> wrote:
>>> On 9 March 2016 at 17:11, Marek Olšák  wrote:
 On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  
 wrote:
> On 8 March 2016 at 22:29, Marek Olšák  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), , sizeof(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
> 

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

[Emil Velikov]

On 9 March 2016 at 18:36, Marek Olšák  wrote:
> On Wed, Mar 9, 2016 at 6:51 PM, Emil Velikov  wrote:
>> On 9 March 2016 at 17:11, Marek Olšák  wrote:
>>> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  
>>> wrote:
 On 8 March 2016 at 22:29, Marek Olšák  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. 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.

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.

>>
>>> 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.

Regards,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 9 March 2016 at 18:17, Marek Olšák  wrote:
> On Wed, Mar 9, 2016 at 6:58 PM, Emil Velikov  wrote:
>> On 9 March 2016 at 17:28, Marek Olšák  wrote:
>>> On Wed, Mar 9, 2016 at 4:47 PM, Emil Velikov  
>>> wrote:
 On 8 March 2016 at 15:39, Marek Olšák  wrote:
> On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  
> wrote:
>> Hi Marek,
>>
>> A small question, and a few trivial suggestions. Hopefully I'm not too
>> late for the party.
>>
>> On 3 March 2016 at 19:46, Marek Olšák  wrote:
>>
>>> +typedef struct _mesa_glinterop_device_info {
>>> +   uint32_t size; /* size of this structure */
>>> +
>> I believe Michel suggested a similar thing: Wouldn't it be better to
>> use a version one just like we do for the DRI extensions ? Many other
>> interfaces also use version, some with a combination of size, but this
>> is the first one in my experience that does only size.
>>
>>
>>> +typedef struct _mesa_glinterop_export_in {
>>
>>> +   /* Size of memory pointed to by out_driver_data. */
>>> +   uint32_t out_driver_data_size;
>>> +
>>> +   /* If the caller wants to query driver-specific data about the 
>>> OpenGL
>>> +* object, this should point to the memory where that data will be 
>>> stored.
>>> +*/
>>> +   void *out_driver_data;
>> I take it that the structure and format of this data will be
>> internal/implementation specific, correct ? As on each side there will
>
> Yes.
>
>> be some sanity checking, wouldn't to be better to have size (version
>> and/or other) within that structure format.
>
> Since amdgpu isn't going to use this feature, I don't care too much about 
> it.
>
> Having the size outside of the driver-specific structure seems safer.
>
 Trying future proof things does not work nicely, most of the time.
 Imho it should be added as there is a user for it.
>>>
>>> I agree, but:
>>> 1) Intel want it, so there is a future user.
>>> 2) One of our OpenCL guys and I have agreed to keep it in case we need
>>> in the future too.
>>>
>> Don't mean to sound snarky - two sentences, each consisting the word
>> "future" :-)
>> There was a song somewhere called "Tomorrow never comes".
>
> OK. If Intel say they don't want it, I'll remove it.
>
Pretty sure anyone can add it back as we get a user or two.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Thu, Mar 10, 2016 at 9:34 AM, Michel Dänzer  wrote:
> On 10.03.2016 02:11, Marek Olšák wrote:
>> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  
>> wrote:
>>> On 8 March 2016 at 22:29, Marek Olšák  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. Also,
>> the implementation of DRI extensions allocates the structures, while
>> in the interop the user allocates the structures. It's a totally
>> different model.
>
> BTW, any particular reason against allocating the memory for the out
> structs in the callees? It seems like that could simplify things quite a
> bit.

Not really. First, it's very uncommon to do that. I've never seen an
API doing it this way. Second, the parameter would have to be "**out".
Last but not least, the callee could only do the allocation with
malloc, while the caller would have to use free to release it. It
complicates things.

The current implementation is more straightforward.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 10.03.2016 02:11, Marek Olšák wrote:
> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  wrote:
>> On 8 March 2016 at 22:29, Marek Olšák  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. Also,
> the implementation of DRI extensions allocates the structures, while
> in the interop the user allocates the structures. It's a totally
> different model.

BTW, any particular reason against allocating the memory for the out
structs in the callees? It seems like that could simplify things quite a
bit.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Wed, Mar 9, 2016 at 6:51 PM, Emil Velikov  wrote:
> On 9 March 2016 at 17:11, Marek Olšák  wrote:
>> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  
>> wrote:
>>> On 8 March 2016 at 22:29, Marek Olšák  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.

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

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Wed, Mar 9, 2016 at 6:58 PM, Emil Velikov  wrote:
> On 9 March 2016 at 17:28, Marek Olšák  wrote:
>> On Wed, Mar 9, 2016 at 4:47 PM, Emil Velikov  
>> wrote:
>>> On 8 March 2016 at 15:39, Marek Olšák  wrote:
 On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  
 wrote:
> Hi Marek,
>
> A small question, and a few trivial suggestions. Hopefully I'm not too
> late for the party.
>
> On 3 March 2016 at 19:46, Marek Olšák  wrote:
>
>> +typedef struct _mesa_glinterop_device_info {
>> +   uint32_t size; /* size of this structure */
>> +
> I believe Michel suggested a similar thing: Wouldn't it be better to
> use a version one just like we do for the DRI extensions ? Many other
> interfaces also use version, some with a combination of size, but this
> is the first one in my experience that does only size.
>
>
>> +typedef struct _mesa_glinterop_export_in {
>
>> +   /* Size of memory pointed to by out_driver_data. */
>> +   uint32_t out_driver_data_size;
>> +
>> +   /* If the caller wants to query driver-specific data about the OpenGL
>> +* object, this should point to the memory where that data will be 
>> stored.
>> +*/
>> +   void *out_driver_data;
> I take it that the structure and format of this data will be
> internal/implementation specific, correct ? As on each side there will

 Yes.

> be some sanity checking, wouldn't to be better to have size (version
> and/or other) within that structure format.

 Since amdgpu isn't going to use this feature, I don't care too much about 
 it.

 Having the size outside of the driver-specific structure seems safer.

>>> Trying future proof things does not work nicely, most of the time.
>>> Imho it should be added as there is a user for it.
>>
>> I agree, but:
>> 1) Intel want it, so there is a future user.
>> 2) One of our OpenCL guys and I have agreed to keep it in case we need
>> in the future too.
>>
> Don't mean to sound snarky - two sentences, each consisting the word
> "future" :-)
> There was a song somewhere called "Tomorrow never comes".

OK. If Intel say they don't want it, I'll remove it.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 9 March 2016 at 17:28, Marek Olšák  wrote:
> On Wed, Mar 9, 2016 at 4:47 PM, Emil Velikov  wrote:
>> On 8 March 2016 at 15:39, Marek Olšák  wrote:
>>> On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  
>>> wrote:
 Hi Marek,

 A small question, and a few trivial suggestions. Hopefully I'm not too
 late for the party.

 On 3 March 2016 at 19:46, Marek Olšák  wrote:

> +typedef struct _mesa_glinterop_device_info {
> +   uint32_t size; /* size of this structure */
> +
 I believe Michel suggested a similar thing: Wouldn't it be better to
 use a version one just like we do for the DRI extensions ? Many other
 interfaces also use version, some with a combination of size, but this
 is the first one in my experience that does only size.


> +typedef struct _mesa_glinterop_export_in {

> +   /* Size of memory pointed to by out_driver_data. */
> +   uint32_t out_driver_data_size;
> +
> +   /* If the caller wants to query driver-specific data about the OpenGL
> +* object, this should point to the memory where that data will be 
> stored.
> +*/
> +   void *out_driver_data;
 I take it that the structure and format of this data will be
 internal/implementation specific, correct ? As on each side there will
>>>
>>> Yes.
>>>
 be some sanity checking, wouldn't to be better to have size (version
 and/or other) within that structure format.
>>>
>>> Since amdgpu isn't going to use this feature, I don't care too much about 
>>> it.
>>>
>>> Having the size outside of the driver-specific structure seems safer.
>>>
>> Trying future proof things does not work nicely, most of the time.
>> Imho it should be added as there is a user for it.
>
> I agree, but:
> 1) Intel want it, so there is a future user.
> 2) One of our OpenCL guys and I have agreed to keep it in case we need
> in the future too.
>
Don't mean to sound snarky - two sentences, each consisting the word
"future" :-)
There was a song somewhere called "Tomorrow never comes".

>>

 IMHO it's worth mentioning any of that, plus some information about
 the lifetime expectancy of the data. Thus it's perfectly clear to the
 user how to manage/use it.
>>>
>>> The data pointer should only be used for querying stuff from Mesa. The
>>> same rules as for the "out" pointer apply.
>>>
>> I think there is some misunderstanding here. I wasn't asking "Who is
>> going to use this data ?", but "Can they use the pointer reliably, or
>> should they copy the data from it before using it. Copy, because the
>> opposite end will discard/free the block shortly after the call". I've
>> seen some people referring to this as lifetime expectancy, not sure if
>> it's the correct terminology to use here.
>
> I'm not sure I fully understand. It can be a local variable in the
> OpenCL stack or a variable in a long-living OpenCL object. Mesa/GL can
> only write data to it inside the interop call.
>
It was a misunderstanding from my end. Sorry for the noise.

Thanks again, for keeping up with my question/suggestions.

Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 9 March 2016 at 17:11, Marek Olšák  wrote:
> On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  wrote:
>> On 8 March 2016 at 22:29, Marek Olšák  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 ?

> 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.

> Testing should show pretty quickly that's it's broken.
>
Definitely. I just hope we see less cases where proper testing occurs,
months after things are out.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Wed, Mar 9, 2016 at 4:47 PM, Emil Velikov  wrote:
> On 8 March 2016 at 15:39, Marek Olšák  wrote:
>> On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  
>> wrote:
>>> Hi Marek,
>>>
>>> A small question, and a few trivial suggestions. Hopefully I'm not too
>>> late for the party.
>>>
>>> On 3 March 2016 at 19:46, Marek Olšák  wrote:
>>>
 +typedef struct _mesa_glinterop_device_info {
 +   uint32_t size; /* size of this structure */
 +
>>> I believe Michel suggested a similar thing: Wouldn't it be better to
>>> use a version one just like we do for the DRI extensions ? Many other
>>> interfaces also use version, some with a combination of size, but this
>>> is the first one in my experience that does only size.
>>>
>>>
 +typedef struct _mesa_glinterop_export_in {
>>>
 +   /* Size of memory pointed to by out_driver_data. */
 +   uint32_t out_driver_data_size;
 +
 +   /* If the caller wants to query driver-specific data about the OpenGL
 +* object, this should point to the memory where that data will be 
 stored.
 +*/
 +   void *out_driver_data;
>>> I take it that the structure and format of this data will be
>>> internal/implementation specific, correct ? As on each side there will
>>
>> Yes.
>>
>>> be some sanity checking, wouldn't to be better to have size (version
>>> and/or other) within that structure format.
>>
>> Since amdgpu isn't going to use this feature, I don't care too much about it.
>>
>> Having the size outside of the driver-specific structure seems safer.
>>
> Trying future proof things does not work nicely, most of the time.
> Imho it should be added as there is a user for it.

I agree, but:
1) Intel want it, so there is a future user.
2) One of our OpenCL guys and I have agreed to keep it in case we need
in the future too.

>
>>>
>>> IMHO it's worth mentioning any of that, plus some information about
>>> the lifetime expectancy of the data. Thus it's perfectly clear to the
>>> user how to manage/use it.
>>
>> The data pointer should only be used for querying stuff from Mesa. The
>> same rules as for the "out" pointer apply.
>>
> I think there is some misunderstanding here. I wasn't asking "Who is
> going to use this data ?", but "Can they use the pointer reliably, or
> should they copy the data from it before using it. Copy, because the
> opposite end will discard/free the block shortly after the call". I've
> seen some people referring to this as lifetime expectancy, not sure if
> it's the correct terminology to use here.

I'm not sure I fully understand. It can be a local variable in the
OpenCL stack or a variable in a long-living OpenCL object. Mesa/GL can
only write data to it inside the interop call.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Wed, Mar 9, 2016 at 4:31 PM, Emil Velikov  wrote:
> On 8 March 2016 at 22:29, Marek Olšák  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. Also,
the implementation of DRI extensions allocates the structures, while
in the interop the user allocates the structures. It's a totally
different model.

>
> 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. 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.
Testing should show pretty quickly that's it's broken.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 8 March 2016 at 15:39, Marek Olšák  wrote:
> On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  
> wrote:
>> Hi Marek,
>>
>> A small question, and a few trivial suggestions. Hopefully I'm not too
>> late for the party.
>>
>> On 3 March 2016 at 19:46, Marek Olšák  wrote:
>>
>>> +typedef struct _mesa_glinterop_device_info {
>>> +   uint32_t size; /* size of this structure */
>>> +
>> I believe Michel suggested a similar thing: Wouldn't it be better to
>> use a version one just like we do for the DRI extensions ? Many other
>> interfaces also use version, some with a combination of size, but this
>> is the first one in my experience that does only size.
>>
>>
>>> +typedef struct _mesa_glinterop_export_in {
>>
>>> +   /* Size of memory pointed to by out_driver_data. */
>>> +   uint32_t out_driver_data_size;
>>> +
>>> +   /* If the caller wants to query driver-specific data about the OpenGL
>>> +* object, this should point to the memory where that data will be 
>>> stored.
>>> +*/
>>> +   void *out_driver_data;
>> I take it that the structure and format of this data will be
>> internal/implementation specific, correct ? As on each side there will
>
> Yes.
>
>> be some sanity checking, wouldn't to be better to have size (version
>> and/or other) within that structure format.
>
> Since amdgpu isn't going to use this feature, I don't care too much about it.
>
> Having the size outside of the driver-specific structure seems safer.
>
Trying future proof things does not work nicely, most of the time.
Imho it should be added as there is a user for it.

>>
>> IMHO it's worth mentioning any of that, plus some information about
>> the lifetime expectancy of the data. Thus it's perfectly clear to the
>> user how to manage/use it.
>
> The data pointer should only be used for querying stuff from Mesa. The
> same rules as for the "out" pointer apply.
>
I think there is some misunderstanding here. I wasn't asking "Who is
going to use this data ?", but "Can they use the pointer reliably, or
should they copy the data from it before using it. Copy, because the
opposite end will discard/free the block shortly after the call". I've
seen some people referring to this as lifetime expectancy, not sure if
it's the correct terminology to use here.

Would be nice to add a comment about this in the API.

>>
>>
>>> +GLAPI int GLAPIENTRY
>>> +MesaGLInteropGLXExportObject(Display *dpy, GLXContext context,
>>> + mesa_glinterop_export_in *in,
>>> + mesa_glinterop_export_out *out);
>> Annotating EGL/GLX display and context as const is very uncommon,
>> although we should do that for 'in'. Shouldn't we ?
>
> We can do that, yes.
>
Thanks.

Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 8 March 2016 at 22:29, Marek Olšák  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 ;-)

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.
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.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 09.03.2016 07:29, Marek Olšák wrote:
> On Tue, Mar 8, 2016 at 4:39 PM, Marek Olšák  wrote:
>> On Sat, Mar 5, 2016 at 9:53 AM, Michel Dänzer  wrote:
>>> On 04.03.2016 04:46, Marek Olšák wrote:

 +/**
 + * Device information returned by Mesa.
 + */
 +typedef struct _mesa_glinterop_device_info {
 +   uint32_t size; /* size of this structure */
>>>
>>> Callees determine how much data they can write by looking at these size
>>> members of the *out parameters. That's pretty error-prone: If the
>>> callers just pass in a pointer they received from malloc, forgetting to
>>> initialize the size member, it'll have a random value, which is quite
>>> likely larger than the actual size and what the callee expects as a
>>> minimum, in which case the callee will write past the end of the
>>> allocated memory => memory corruption, if not a security issue.
> 
> Wait. I wouldn't like to optimize for incorrect API usage.

Of course not, just trying to make sure it won't result in a worse
failure mode than necessary.

> Setting "out=rand()" is likely to have fun behavior too. What then?

That's not really the same thing, so it's kind of a strawman argument.


>>> If you still don't want to go for a version based scheme instead, I'd
>>> suggest passing in the size as an explicit function parameter. (Or at
>>> the very least, it needs to be documented very prominently that callers
>>> must initialize out->size before calling in; but we know no matter how
>>> well that is documented, it'll probably be ignored anyway sooner or
>>> later...)
>>
>> Version based it is then.
> 
> 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 the sizes of the out structs are only written by callers and only
read by callees, removing the sizes from the structs and passing them in
as separate function parameters instead would be much less ugly and
error prone.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Tue, Mar 8, 2016 at 4:39 PM, Marek Olšák  wrote:
> On Sat, Mar 5, 2016 at 9:53 AM, Michel Dänzer  wrote:
>> On 04.03.2016 04:46, Marek Olšák wrote:
>>>
>>> +/**
>>> + * Device information returned by Mesa.
>>> + */
>>> +typedef struct _mesa_glinterop_device_info {
>>> +   uint32_t size; /* size of this structure */
>>
>> Callees determine how much data they can write by looking at these size
>> members of the *out parameters. That's pretty error-prone: If the
>> callers just pass in a pointer they received from malloc, forgetting to
>> initialize the size member, it'll have a random value, which is quite
>> likely larger than the actual size and what the callee expects as a
>> minimum, in which case the callee will write past the end of the
>> allocated memory => memory corruption, if not a security issue.

Wait. I wouldn't like to optimize for incorrect API usage. Setting
"out=rand()" is likely to have fun behavior too. What then? This is a
battle we can't win.

>>
>> If you still don't want to go for a version based scheme instead, I'd
>> suggest passing in the size as an explicit function parameter. (Or at
>> the very least, it needs to be documented very prominently that callers
>> must initialize out->size before calling in; but we know no matter how
>> well that is documented, it'll probably be ignored anyway sooner or
>> later...)
>
> Version based it is then.

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.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Thu, Mar 3, 2016 at 11:56 PM, Emil Velikov  wrote:
> Hi Marek,
>
> A small question, and a few trivial suggestions. Hopefully I'm not too
> late for the party.
>
> On 3 March 2016 at 19:46, Marek Olšák  wrote:
>
>> +typedef struct _mesa_glinterop_device_info {
>> +   uint32_t size; /* size of this structure */
>> +
> I believe Michel suggested a similar thing: Wouldn't it be better to
> use a version one just like we do for the DRI extensions ? Many other
> interfaces also use version, some with a combination of size, but this
> is the first one in my experience that does only size.
>
>
>> +typedef struct _mesa_glinterop_export_in {
>
>> +   /* Size of memory pointed to by out_driver_data. */
>> +   uint32_t out_driver_data_size;
>> +
>> +   /* If the caller wants to query driver-specific data about the OpenGL
>> +* object, this should point to the memory where that data will be 
>> stored.
>> +*/
>> +   void *out_driver_data;
> I take it that the structure and format of this data will be
> internal/implementation specific, correct ? As on each side there will

Yes.

> be some sanity checking, wouldn't to be better to have size (version
> and/or other) within that structure format.

Since amdgpu isn't going to use this feature, I don't care too much about it.

Having the size outside of the driver-specific structure seems safer.

>
> IMHO it's worth mentioning any of that, plus some information about
> the lifetime expectancy of the data. Thus it's perfectly clear to the
> user how to manage/use it.

The data pointer should only be used for querying stuff from Mesa. The
same rules as for the "out" pointer apply.

>
>
>> +GLAPI int GLAPIENTRY
>> +MesaGLInteropGLXExportObject(Display *dpy, GLXContext context,
>> + mesa_glinterop_export_in *in,
>> + mesa_glinterop_export_out *out);
> Annotating EGL/GLX display and context as const is very uncommon,
> although we should do that for 'in'. Shouldn't we ?

We can do that, yes.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

On Sat, Mar 5, 2016 at 9:53 AM, Michel Dänzer  wrote:
> On 04.03.2016 04:46, Marek Olšák wrote:
>>
>> +/**
>> + * Device information returned by Mesa.
>> + */
>> +typedef struct _mesa_glinterop_device_info {
>> +   uint32_t size; /* size of this structure */
>
> Callees determine how much data they can write by looking at these size
> members of the *out parameters. That's pretty error-prone: If the
> callers just pass in a pointer they received from malloc, forgetting to
> initialize the size member, it'll have a random value, which is quite
> likely larger than the actual size and what the callee expects as a
> minimum, in which case the callee will write past the end of the
> allocated memory => memory corruption, if not a security issue.
>
> If you still don't want to go for a version based scheme instead, I'd
> suggest passing in the size as an explicit function parameter. (Or at
> the very least, it needs to be documented very prominently that callers
> must initialize out->size before calling in; but we know no matter how
> well that is documented, it'll probably be ignored anyway sooner or
> later...)

Version based it is then.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Michel Dänzer]

On 04.03.2016 04:46, Marek Olšák wrote:
> 
> +/**
> + * Device information returned by Mesa.
> + */
> +typedef struct _mesa_glinterop_device_info {
> +   uint32_t size; /* size of this structure */

Callees determine how much data they can write by looking at these size
members of the *out parameters. That's pretty error-prone: If the
callers just pass in a pointer they received from malloc, forgetting to
initialize the size member, it'll have a random value, which is quite
likely larger than the actual size and what the callee expects as a
minimum, in which case the callee will write past the end of the
allocated memory => memory corruption, if not a security issue.

If you still don't want to go for a version based scheme instead, I'd
suggest passing in the size as an explicit function parameter. (Or at
the very least, it needs to be documented very prominently that callers
must initialize out->size before calling in; but we know no matter how
well that is documented, it'll probably be ignored anyway sooner or
later...)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

Hi Marek,

A small question, and a few trivial suggestions. Hopefully I'm not too
late for the party.

On 3 March 2016 at 19:46, Marek Olšák  wrote:

> +typedef struct _mesa_glinterop_device_info {
> +   uint32_t size; /* size of this structure */
> +
I believe Michel suggested a similar thing: Wouldn't it be better to
use a version one just like we do for the DRI extensions ? Many other
interfaces also use version, some with a combination of size, but this
is the first one in my experience that does only size.


> +typedef struct _mesa_glinterop_export_in {

> +   /* Size of memory pointed to by out_driver_data. */
> +   uint32_t out_driver_data_size;
> +
> +   /* If the caller wants to query driver-specific data about the OpenGL
> +* object, this should point to the memory where that data will be stored.
> +*/
> +   void *out_driver_data;
I take it that the structure and format of this data will be
internal/implementation specific, correct ? As on each side there will
be some sanity checking, wouldn't to be better to have size (version
and/or other) within that structure format.

IMHO it's worth mentioning any of that, plus some information about
the lifetime expectancy of the data. Thus it's perfectly clear to the
user how to manage/use it.


> +GLAPI int GLAPIENTRY
> +MesaGLInteropGLXExportObject(Display *dpy, GLXContext context,
> + mesa_glinterop_export_in *in,
> + mesa_glinterop_export_out *out);
Annotating EGL/GLX display and context as const is very uncommon,
although we should do that for 'in'. Shouldn't we ?

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Marek Olšák]

From: Marek Olšák 

v2: use "enum" to define stuff
v3: more comments, define MESA_GLINTEROP_UNSUPPORTED
---
 include/GL/mesa_glinterop.h | 259 
 1 file changed, 259 insertions(+)
 create mode 100644 include/GL/mesa_glinterop.h

diff --git a/include/GL/mesa_glinterop.h b/include/GL/mesa_glinterop.h
new file mode 100644
index 000..db19be7
--- /dev/null
+++ b/include/GL/mesa_glinterop.h
@@ -0,0 +1,259 @@
+/*
+ * Mesa 3-D graphics library
+ *
+ * Copyright 2016 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/* Mesa OpenGL inter-driver interoperability interface designed for but not
+ * limited to OpenCL.
+ *
+ * This is a driver-agnostic, backward-compatible interface. The structures
+ * are only allowed to grow. They can never shrink and their members can
+ * never be removed, renamed, or redefined.
+ *
+ * The interface doesn't return a lot of static texture parameters like
+ * width, height, etc. It mainly returns mutable buffer and texture view
+ * parameters that can't be part of the texture allocation (because they are
+ * mutable). If drivers want to return more data or want to return static
+ * allocation parameters, they can do it in one of these two ways:
+ * - attaching the data to the DMABUF handle in a driver-specific way
+ * - passing the data via "out_driver_data" in the "in" structure.
+ *
+ * Mesa is expected to do a lot of error checking on behalf of OpenCL, such
+ * as checking the target, miplevel, and texture completeness.
+ *
+ * OpenCL, on the other hand, needs to check if the display+context combo
+ * is compatible with the OpenCL driver by querying the device information.
+ * It also needs to check if the texture internal format and channel ordering
+ * (returned in a driver-specific way) is supported by OpenCL, among other
+ * things.
+ */
+
+#ifndef MESA_GLINTEROP_H
+#define MESA_GLINTEROP_H
+
+#include 
+#include 
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define MESA_GLINTEROP_VERSION 1
+
+/** Returned error codes. */
+enum {
+   MESA_GLINTEROP_SUCCESS = 0,
+   MESA_GLINTEROP_OUT_OF_RESOURCES,
+   MESA_GLINTEROP_OUT_OF_HOST_MEMORY,
+   MESA_GLINTEROP_INVALID_OPERATION,
+   MESA_GLINTEROP_INVALID_VALUE,
+   MESA_GLINTEROP_INVALID_DISPLAY,
+   MESA_GLINTEROP_INVALID_CONTEXT,
+   MESA_GLINTEROP_INVALID_TARGET,
+   MESA_GLINTEROP_INVALID_OBJECT,
+   MESA_GLINTEROP_INVALID_MIP_LEVEL,
+   MESA_GLINTEROP_UNSUPPORTED
+};
+
+/** Access flags. */
+enum {
+   MESA_GLINTEROP_ACCESS_READ_WRITE = 0,
+   MESA_GLINTEROP_ACCESS_READ_ONLY,
+   MESA_GLINTEROP_ACCESS_WRITE_ONLY
+};
+
+
+/**
+ * Device information returned by Mesa.
+ */
+typedef struct _mesa_glinterop_device_info {
+   uint32_t size; /* size of this structure */
+
+   /* PCI location */
+   uint32_t pci_segment_group;
+   uint32_t pci_bus;
+   uint32_t pci_device;
+   uint32_t pci_function;
+
+   /* Device identification */
+   uint32_t vendor_id;
+   uint32_t device_id;
+} mesa_glinterop_device_info;
+
+
+/**
+ * Input parameters to Mesa interop export functions.
+ */
+typedef struct _mesa_glinterop_export_in {
+   uint32_t size; /* size of this structure */
+
+   /* One of the following:
+* - GL_TEXTURE_BUFFER
+* - GL_TEXTURE_1D
+* - GL_TEXTURE_2D
+* - GL_TEXTURE_3D
+* - GL_TEXTURE_RECTANGLE
+* - GL_TEXTURE_1D_ARRAY
+* - GL_TEXTURE_2D_ARRAY
+* - GL_TEXTURE_CUBE_MAP_ARRAY
+* - GL_TEXTURE_CUBE_MAP
+* - GL_TEXTURE_CUBE_MAP_POSITIVE_X
+* - GL_TEXTURE_CUBE_MAP_NEGATIVE_X
+* - GL_TEXTURE_CUBE_MAP_POSITIVE_Y
+* - GL_TEXTURE_CUBE_MAP_NEGATIVE_Y
+* - GL_TEXTURE_CUBE_MAP_POSITIVE_Z
+* - GL_TEXTURE_CUBE_MAP_NEGATIVE_Z
+* - GL_TEXTURE_2D_MULTISAMPLE
+* - GL_TEXTURE_2D_MULTISAMPLE_ARRAY
+* - GL_TEXTURE_EXTERNAL_OES
+* - GL_RENDERBUFFER
+* - GL_ARRAY_BUFFER
+*/
+   GLenum target;
+
+   /* If target is GL_ARRAY_BUFFER, it's a