[PATCH weston 1/4] gl-renderer: use eglGetPlatformDisplayEXT to get an EGLDisplay

[Jonny Lamb]

Il 23/02/2015 18:44, Derek Foreman ha scritto:
>> -		if (gl_renderer->create(&compositor->base, EGL_DEFAULT_DISPLAY,
>> +		if (gl_renderer->create(&compositor->base, 0, EGL_DEFAULT_DISPLAY,
>>   					gl_renderer->opaque_attribs,
>>   					NULL) < 0) {
>
> Why is fbdev treated differently than the other compositors?  I guess no
> associated platform extension?

Yes exactly, so we fallback to eglGetDisplay.

>>   	if (!c->use_pixman) {
>> -		if (gl_renderer->create(&c->base, c->parent.wl_display,
>> -				gl_renderer->alpha_attribs,
>> -				NULL) < 0) {
>> +		if (!gl_renderer->supports ||
>> +		    (gl_renderer->supports(&c->base, "EGL_KHR_platform_wayland") < 0 &&
>> +		     gl_renderer->supports(&c->base, "EGL_EXT_platform_wayland"))) {
>> +			weston_log("No support for EGL_KHR_platform_wayland; "
>> +			           "falling back to pixman.\n");
>> +			c->use_pixman = 1;
>> +
> extra blank line?  ^

I thought it looked clearer, but okay, removed.

>> +/* returns negative:
>> + *  1. if EGL client extensions aren't supported.
>> + * returns positive:
>> + *  1. if the EGL version is >= 1.5,
>> + *  2. if the supplied EGL client extension is supported,
>> + *  3. in the fallback case to using eglGetDisplay after logging a
>> + *     warning. */
>
> Would prefer to see the above comments in a doxygen format.

OK, done.

>> +#ifdef EGL_EXT_platform_base
>> +		get_platform_display =
>> +			(void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
>> +#endif
>
> It's a little counter intuitive that a function called
> "gl_renderer_supports" actually does initialization.  Please document it
> in the comments above the function (or do it somewhere else :)

Yeah fair, I've put it in gl_renderer_create as that's the first place 
we need it anyway.

>> +	weston_log("warning: unable to check for %s support; "
>> +		   "could fall back to eglGetDisplay.\n", extension);
>
> I kind of don't like having the logging here.  I'd rather see it logged
> in the compositor only if we're actually doing the fallback.  My EGL
> doesn't support the KHR variants and I found it a little confusing to
> see warnings logged despite the EXT variant being available.

Yeah this is a fair point, I've done as you asked.

> Another option would be just to pass in the tail of the string (x11,
> gbm, wayland) and concatenate it (with EXT, MESA, KHR expecting some not
> to exist...) in this function and do all testing here without the
> compositor making potentially multiple calls.  Whether that's a
> reasonable idea or not is up to you. :)

OK yeah this reduces the code duplication; I've done it. Let's see what 
anyone else thinks.

(New patch set to follow.)

-- 
Jonny Lamb