[PATCH weston 07/11] gl-renderer: Always setup gl-renderer

[Pekka Paalanen]

On Sat, 18 Jun 2016 19:15:22 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> Currently, the gl-renderer setup is being done on per-output
> basis. This isn't desirable when trying to make weston run
> with zero outputs.
> 
> When there are no outputs present, there is no EGLContext or
> EGLCurrent available, and trying to use an EGL extension will
> result in a crash.

Hi Armin,

what is that EGLCurrent you also mentioned in your blog?

I'm not aware of any such object. If you refer to eglMakeCurrent(), it
means "make the given EGLContext and the read and write EGLSurfaces
current in this thread", which will affect all context-sensitive
function calls, including everything of GL and several EGL functions,
most notably eglSwapBuffers.

> The problem is solved by creating a dummy surface at startup,
> either by using EGL_KHR_surfaceless_context or PbufferSurface
> to setup a context, and creating an EGLContext and EGLCurrent
> with the help of dummy surface.

Surfaceless is not really creating a dummy surface. ;-)

> When an output gets plugged in, its configuration will replace
> dummy buffer usage as the setup will be run again.

Hmm, re-running setup does not sound right, but let's see below.

> Signed-off-by: Armin Krezović <krezovic.armin at gmail.com>
> ---
>  src/gl-renderer.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index 23c0cd7..7e2a787 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -175,6 +175,8 @@ struct gl_renderer {
>  	EGLContext egl_context;
>  	EGLConfig egl_config;
>  
> +	EGLSurface dummy_surface;
> +
>  	struct wl_array vertices;
>  	struct wl_array vtxcnt;
>  
> @@ -201,6 +203,8 @@ struct gl_renderer {
>  
>  	int has_configless_context;
>  
> +	int has_surfaceless_context;
> +
>  	int has_dmabuf_import;
>  	struct wl_list dmabuf_images;
>  
> @@ -2654,6 +2658,8 @@ gl_renderer_destroy(struct weston_compositor *ec)
>  	wl_list_for_each_safe(image, next, &gr->dmabuf_images, link)
>  		dmabuf_image_destroy(image);
>  
> +	eglDestroySurface(gr->egl_display, gr->dummy_surface);

I think this call needs to be conditional, since you are not guaranteed
to create the dummy surface. EGL 1.4 says for eglDestroySurface: "An
EGL_BAD_SURFACE error is generated if surface is not a valid rendering
surface."

> +
>  	eglTerminate(gr->egl_display);
>  	eglReleaseThread();
>  
> @@ -2765,6 +2771,9 @@ gl_renderer_setup_egl_extensions(struct weston_compositor *ec)
>  		gr->has_configless_context = 1;
>  #endif
>  
> +	if (check_extension(extensions, "EGL_KHR_surfaceless_context"))
> +		gr->has_surfaceless_context = 1;
> +

Good.

>  #ifdef EGL_EXT_image_dma_buf_import
>  	if (check_extension(extensions, "EGL_EXT_image_dma_buf_import"))
>  		gr->has_dmabuf_import = 1;
> @@ -2881,6 +2890,12 @@ gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
>  	EGLint major, minor;
>  	int supports = 0;
>  
> +	const EGLint pbuffer_attribs[] = {
> +		EGL_WIDTH, 10,
> +		EGL_HEIGHT, 10,
> +		EGL_NONE
> +	};

This attrib list is fine. Could be static, too.

> +
>  	if (platform) {
>  		supports = gl_renderer_supports(
>  			ec, platform_to_extension(platform));
> @@ -2956,6 +2971,23 @@ gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
>  	if (gr->has_dmabuf_import)
>  		gr->base.import_dmabuf = gl_renderer_import_dmabuf;
>  
> +	if (gr->has_surfaceless_context) {
> +		weston_log("EGL_KHR_surfaceless_context available\n");
> +		gr->dummy_surface = EGL_NO_SURFACE;
> +	} else {
> +		weston_log("EGL_KHR_surfaceless_context unavailable "
> +			   "trying PbufferSurface\n");
> +		gr->dummy_surface = eglCreatePbufferSurface(gr->egl_display,
> +							    gr->egl_config,
> +							    pbuffer_attribs);

You will need to find a new EGLConfig for this call, one that is
indicated suitable with EGL_PBUFFER_BIT. IOW, you need the same set as
gl_renderer_opaque_attribs except EGL_SURFACE_TYPE has to be
EGL_PBUFFER_BIT. You can call egl_choose_config() with visual_id NULL
to find one, or eglChooseConfig() directly to just get the first one.
We don't really care what the config looks like, as long as we can make
a pbuffer with it.

The gr->egl_config is only suitable the kinds of outputs the backend
will be using, and I don't think that ever includes EGL_PBUFFER_BIT.
You can get lucky, of course. Some configs work for both windows and
pbuffers.

> +		if (gr->dummy_surface == EGL_NO_SURFACE) {
> +			weston_log("failed to create dummy pbuffer\n");
> +			goto fail_terminate;

Why not fail_with_error?

> +		}

It might make sense to split this path out into a new function, as you
will need a temporary EGLConfig and then the pbuffer_attribs can be
local there too.

> +	}
> +
> +	gl_renderer_setup(ec, gr->dummy_surface);
> +

This would be better just before the return statement, so gr is fully
initialized.

You forgot to check it succeeds.

>  	wl_display_add_shm_format(ec->wl_display, WL_SHM_FORMAT_RGB565);
>  
>  	wl_signal_init(&gr->destroy_signal);

Finally, you forgot to remove the gl_renderer_setup() call from
gl_renderer_output_create(). That call is now dead, because
gr->egl_context is guaranteed to be set.

If you want extra fun, you could check if dynamic renderer switching
still works. ;-)

(Start DRM-backend with --use-pixman, then pressing Mod+Shift+Space,
'w' should make Weston switch to the GL-renderer. If it doesn't work
even before your patch, then don't worry about it.)

The overall logic in your patch looks good.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160621/a4f80c6d/attachment.sig>