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

[Pekka Paalanen]

On Wed, 22 Jun 2016 12:05:01 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> On 21.06.2016 11:13, Pekka Paalanen wrote:
> > 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.
> >   
> 
> This is a misinterpretation from my side. It happens when you spend
> too much time looking at weston's surface/view mapping code :).
> 
> After your brief explanation on IRC, I've updated my blog post
> and will update this description too for the next round.
> 
> >> 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. ;-)
> >   
> 
> Heh, yeah. I'll change it to change that to "EGL_NO_SURFACE or
> PbufferSurface". Does that make more sense?

Hi,

not sure exatly how you'd edit that, but something like this:

"The problem is solved by using EGL_KRH_surfaceless_context, or if that
is unavailable, creating a dummy pbuffer surface for the setup code."

> 
> >> 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.
> >   
> 
> This was again a misunderstanding and an oversight from my side.
> gl_renderer_setup is just ran once. gl_renderer_create_output
> won't run it if there's a context present.
> 
> >> 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."
> >   
> 
> I didn't see the spec mention anything about the behavior when
> surfaceless context is available and EGL_NO_SURFACE is being used.

I base my interpretation on that EGL_NO_SURFACE is not a valid
rendering surface.

I simply want to avoid calling eglDestroySurface(disp, EGL_NO_SURFACE)
so we don't accidentally set an error state in EGL if it turns out
illegal.

> In case of pbuffersurface, the renderer would terminate anyways if
> it got an invalid surface from eglCreatePbufferSurface.
> 
> >> +
> >>  	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.
> >   
> 
> Is it necessary? If so, I can add it, no big deal.

Would make it go into the constant data section of the binary without a
runtime copy for sure. Peanuts, really.


> > 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.)  
> 
> I'll try that when I get to test the drm backend. Currently all my
> work is based on X11 backend, as I have no sane way to unplug all
> outputs on drm backend.

Yeah, no worries. And that was really just to avoid regressing
anything, so with outputs present.


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/20160622/d2079d80/attachment.sig>