[Xcb] [PATCH 2/2] glx: Add protocol for GLX_ARB_create_context and GLX_ARB_create_context_profile

[Ian Romanick]

On 11/30/2011 02:22 PM, Peter Harris wrote:
> Thanks for doing all the hard work. Review inline below:
>
> On 2011-11-30 13:01, Ian Romanick wrote:
>>
>> +	<request name="SetClientInfoARB" opcode="33">
>
>> +		<list type="CARD32" name="glversions">
>
> SetClientInfo2ARB uses an underscore in gl_versions. For consistency,
> you should probably add an underscore here, or remove the other one.

I'll add the various missing underscores.

>> +		<list type="char" name="glextensionstring">
>> +			<fieldref>gl_str_len</fieldref>
>> +		</list>
>> +		<list type="char" name="glxextensionstring">
>> +			<fieldref>glx_str_len</fieldref>
>> +		</list>
>
> There is no implicit padding between lists of the same type. This needs
> something like<list type="void" name="alignment_pad">  (see
> xf86vidmode.xml) between the strings.
>
> I apologize for the awkward syntax. Unfortunately, we don't have<pad
> align="4" />  yet.

Is this actually necessary?  Looking at the code generated, it looks 
like character lists are automatically padded to 4-bit alignment:

     /* char glextensionstring */
     xcb_parts[6].iov_base = (char *) glextensionstring;
     xcb_parts[6].iov_len = gl_str_len * sizeof(char);
     xcb_parts[7].iov_base = 0;
     xcb_parts[7].iov_len = -xcb_parts[6].iov_len & 3;
     /* char glxextensionstring */
     xcb_parts[8].iov_base = (char *) glxextensionstring;
     xcb_parts[8].iov_len = glx_str_len * sizeof(char);
     xcb_parts[9].iov_base = 0;
     xcb_parts[9].iov_len = -xcb_parts[8].iov_len & 3;

If possible, I'd really rather not clutter the protocol spec with the 
alignment_pad mess that xf86vidmode.xml uses.  That bit of padding is 
almost as big as all the rest of the protocol spec.

>> +	<request name="CreateContextAttribsARB" opcode="34">
>> +		<field type="glx:CONTEXT" name="context" />
>> +		<field type="CARD32" name="fbconfig" />
>> +		<field type="CARD32" name="screen" />
>> +		<field type="CARD32" name="share_list" />
>
> The spec says type="glx:CONTEXT" name="share_context" instead of
> "share_list". Is this a typo here or in the spec?

Ah, right.  I copied the protocol for CreateNewContext which also has 
this problem.  It also looks like FBCONFIG type should be used several 
places.

>> +	<request name="SetClientInfo2ARB" opcode="35">
>
>> +		<list type="CARD32" name="gl_versions">
>
> SetClientInfoARB doesn't use the underscore in the name. See above.
>
>> +		<list type="char" name="gl_extension_string">
>> +			<fieldref>gl_str_len</fieldref>
>> +		</list>
>> +		<list type="char" name="glx_extension_string">
>> +			<fieldref>glx_str_len</fieldref>
>> +		</list>
>
> This one needs explicit padding too.
>
> Peter Harris