[PATCH 05/11] glx: Optionally call DRI2 createContextAttribs from __glXDRIscreenCreateContext

[Ian Romanick]

On 01/03/2012 04:22 PM, Jesse Barnes wrote:
> On Fri, 23 Dec 2011 15:18:23 -0800
> "Ian Romanick"<idr at freedesktop.org>  wrote:
>
>> From: Ian Romanick<ian.d.romanick at intel.com>
>>
>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>> ---
>>   glx/glxdri2.c |  143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 files changed, 137 insertions(+), 6 deletions(-)
>>
>> diff --git a/glx/glxdri2.c b/glx/glxdri2.c
>> index 18b5aad..4f112b1 100644
>> --- a/glx/glxdri2.c
>> +++ b/glx/glxdri2.c
>> @@ -47,6 +47,7 @@
>>   #include "glxserver.h"
>>   #include "glxutil.h"
>>   #include "glxdricommon.h"
>> +#include<GL/glxtokens.h>
>>
>>   #include "glapitable.h"
>>   #include "glapi.h"
>> @@ -383,6 +384,56 @@ __glXDRIscreenDestroy(__GLXscreen *baseScreen)
>>       free(screen);
>>   }
>>
>> +static Bool
>> +dri2_convert_glx_attribs(unsigned num_attribs, const uint32_t *attribs,
>> +			 unsigned *major_ver, unsigned *minor_ver,
>> +			 uint32_t *flags, unsigned *error)
>> +{
>> +    unsigned i;
>> +
>> +    if (num_attribs == 0)
>> +	return True;
>> +
>> +    if (attribs == NULL) {
>> +	*error = BadImplementation;
>> +	return False;
>> +    }
>> +
>> +    *major_ver = 1;
>> +    *minor_ver = 0;
>> +
>> +    for (i = 0; i<  num_attribs; i++) {
>> +	switch (attribs[i * 2]) {
>> +	case GLX_CONTEXT_MAJOR_VERSION_ARB:
>> +	    *major_ver = attribs[i * 2 + 1];
>> +	    break;
>> +	case GLX_CONTEXT_MINOR_VERSION_ARB:
>> +	    *minor_ver = attribs[i * 2 + 1];
>> +	    break;
>> +	case GLX_CONTEXT_FLAGS_ARB:
>> +	    *flags = attribs[i * 2 + 1];
>> +	    break;
>> +	case GLX_RENDER_TYPE:
>> +	    break;
>> +	default:
>> +	    /* If an unknown attribute is received, fail.
>> +	     */
>> +	    *error = BadValue;
>> +	    return False;
>> +	}
>> +    }
>
> Do you want to catch the case where multiple versions and/or flags are
> provided and error out?  Pretty esoteric I guess but I'm not sure what
> the semantics are supposed to be.

The spec doesn't say anything about this case, so I don't think there's 
any behavior that an application could rely on.  It might be interesting 
to see what the other guys do, but I'm not very concerned about that 
right now.

>> +
>> +    /* Unknown flag value.
>> +     */
>> +    if (*flags&  ~(__DRI_CTX_FLAG_DEBUG | __DRI_CTX_FLAG_FORWARD_COMPATIBLE)) {
>> +	*error = BadValue;
>> +	return False;
>> +    }
>> +
>> +    *error = Success;
>> +    return True;
>> +}
>> +
>>   static __GLXcontext *
>>   __glXDRIscreenCreateContext(__GLXscreen *baseScreen,
>>   			    __GLXconfig *glxConfig,
>> @@ -403,8 +454,10 @@ __glXDRIscreenCreateContext(__GLXscreen *baseScreen,
>>   	driShare = NULL;
>>
>>       context = calloc(1, sizeof *context);
>> -    if (context == NULL)
>> +    if (context == NULL) {
>> +	*error = BadAlloc;
>>   	return NULL;
>> +    }
>>
>>       context->base.destroy           = __glXDRIcontextDestroy;
>>       context->base.makeCurrent       = __glXDRIcontextMakeCurrent;
>> @@ -413,12 +466,82 @@ __glXDRIscreenCreateContext(__GLXscreen *baseScreen,
>>       context->base.textureFromPixmap =&__glXDRItextureFromPixmap;
>>       context->base.wait              = __glXDRIcontextWait;
>>
>> -    context->driContext =
>> -	(*screen->dri2->createNewContext)(screen->driScreen,
>> -					  config->driConfig,
>> -					  driShare, context);
>> +#if __DRI_DRI2_VERSION>= 3
>> +    if (screen->dri2->base.version>= 3) {
>> +	uint32_t ctx_attribs[3 * 2];
>> +	unsigned num_ctx_attribs = 0;
>> +	unsigned dri_err = 0;
>> +	unsigned major_ver;
>> +	unsigned minor_ver;
>> +	uint32_t flags;
>> +
>> +	if (num_attribs != 0) {
>> +	    if (!dri2_convert_glx_attribs(num_attribs, attribs,
>> +					&major_ver,&minor_ver,
>> +					&flags, error))
>> +		return NULL;
>> +
>> +	    ctx_attribs[num_ctx_attribs++] = __DRI_CTX_ATTRIB_MAJOR_VERSION;
>> +	    ctx_attribs[num_ctx_attribs++] = major_ver;
>> +	    ctx_attribs[num_ctx_attribs++] = __DRI_CTX_ATTRIB_MINOR_VERSION;
>> +	    ctx_attribs[num_ctx_attribs++] = minor_ver;
>> +
>> +	    if (flags != 0) {
>> +		ctx_attribs[num_ctx_attribs++] = __DRI_CTX_ATTRIB_FLAGS;
>> +
>> +		/* The current __DRI_CTX_FLAG_* values are identical to the
>> +		 * GLX_CONTEXT_*_BIT values.
>> +		 */
>> +		ctx_attribs[num_ctx_attribs++] = flags;
>> +	    }
>> +	}
>> +
>> +	context->driContext =
>> +	    (*screen->dri2->createContextAttribs)(screen->driScreen,
>> +						  __DRI_API_OPENGL,
>> +						  config->driConfig,
>> +						  driShare,
>> +						  num_ctx_attribs / 2,
>> +						  ctx_attribs,
>> +						&dri_err,
>> +						  context);
>> +
>> +	switch (dri_err) {
>> +	case __DRI_CTX_ERROR_SUCCESS:
>> +	    *error = Success;
>> +	    break;
>> +	case __DRI_CTX_ERROR_NO_MEMORY:
>> +	    *error = BadAlloc;
>> +	    break;
>> +	case __DRI_CTX_ERROR_BAD_API:
>> +	    *error = __glXError(GLXBadProfileARB);
>> +	    break;
>> +	case __DRI_CTX_ERROR_BAD_VERSION:
>> +	case __DRI_CTX_ERROR_BAD_FLAG:
>> +	    *error = __glXError(GLXBadFBConfig);
>> +	    break;
>> +	case __DRI_CTX_ERROR_UNKNOWN_ATTRIBUTE:
>> +	case __DRI_CTX_ERROR_UNKNOWN_FLAG:
>> +	default:
>> +	    *error = BadValue;
>> +	    break;
>> +	}
>> +    } else
>> +#endif
>
> Maybe stuff this into a separate function that's a no-op in the<  3
> case?  That would clean things up a little and save an #ifdef in the
> middle of a function (always a nice thing).
>
> Or just require updated DRI2 bits to build and avoid the ifdefs
> altogether, since they tend to cause trouble anyway.

I had thought about both of these, and ultimately we just want at later. 
  However, that requires a new Mesa release.  I thought it was better to 
leave the "obvious" clutter of the #ifdef in the code for later clean-up 
than an extra function.

What do you think?