On 21.06.2016 11:13, Pekka Paalanen wrote: > On Sat, 18 Jun 2016 19:15:22 +0200 > Armin Krezović <[email protected]> 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? >> 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ć <[email protected]> >> --- >> 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. 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. >> + >> 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. Okay, that makes sense. > >> + if (gr->dummy_surface == EGL_NO_SURFACE) { >> + weston_log("failed to create dummy pbuffer\n"); >> + goto fail_terminate; > > Why not fail_with_error? > Good question :). I'm not sure. I'll change it to 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. > Alright, makes sense. > You forgot to check it succeeds. > ... yeah. What a silly mistake. Thanks for pointing that out. >> 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. > Yes, that makes sense. > 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. > > The overall logic in your patch looks good. > > > Thanks, > pq > Thank you for the review! Armin
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
