On Tue, 28 Jun 2016 16:02:10 +0200 Armin Krezović <krezovic.ar...@gmail.com> wrote:
> On 27.06.2016 15:08, Pekka Paalanen wrote:
> > Hi Armin,
> >
> > several minor nitpicks on wording here, but the code looks good.
> >
> > On Thu, 23 Jun 2016 11:59:35 +0200
> > Armin Krezović <krezovic.ar...@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.
> >
> > On "first output" rather than "per-output" basis.
> >
> >>
> >> When there are no outputs present, there is no surface available
> >> to attach an EGLContext to with eglMakeCurrent, which makes
> >> any EGL command fail.
> >
> > GL command. Only certain EGL calls use the current context, while all
> > GL calls use it. Specifically, image_target_texture_2d is the GL
> > function glEGLImageTargetTexture2DOES which IIRC was failing without
> > the renderer setup.
> >
> >>
> >> The problem is solved by using EGL_KHR_surfaceless_context to
> >> bind an EGLContext to EGL_NO_SURFACE, or if that is
> >> unavailable, creating a dummy PbufferSurface and binding an
> >> EGLContext to it, so EGL gets set up properly.
> >>
> >> v2:
> >>
> >> - Move PbufferSurface creation into its own function
> >> - Introduce a new EGLConfig with EGL_PBUFFER_BIT set
> >> and use it to create a PbufferSurface
> >> - Make PbufferSurface attributes definition static
> >> - Check for return of gl_renderer_setup and terminate
> >> in case it fails
> >> - Remove redundant gl_renderer_setup call from
> >> gl_renderer_output_create
> >> - Only destroy the dummy surface if it is valid
> >>
> >> Signed-off-by: Armin Krezović <krezovic.ar...@gmail.com>
> >> ---
> >> src/gl-renderer.c | 71
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >> 1 file changed, 65 insertions(+), 6 deletions(-)
> >> @@ -2873,6 +2878,43 @@ platform_to_extension(EGLenum platform)
> >> }
> >>
> >> static int
> >> +gl_renderer_create_pbuffer_surface(struct gl_renderer *gr) {
> >
> > Style-wise, this function could just take a EGLDisplay as an argument,
> > and return the EGLSurface. That way the setting of dummy_surface would
> > be in the caller, which would make it easier to read.
> >
> >> + EGLConfig pbuffer_config;
> >> +
> >> + static const EGLint pbuffer_config_attribs[] = {
> >> + EGL_SURFACE_TYPE, EGL_PBUFFER_BIT,
> >> + EGL_RED_SIZE, 1,
> >> + EGL_GREEN_SIZE, 1,
> >> + EGL_BLUE_SIZE, 1,
> >> + EGL_ALPHA_SIZE, 0,
> >> + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> >> + EGL_NONE
> >> + };
> >> +
> >> + static const EGLint pbuffer_attribs[] = {
> >> + EGL_WIDTH, 10,
> >> + EGL_HEIGHT, 10,
> >> + EGL_NONE
> >> + };
> >> +
> >> + if (egl_choose_config(gr, pbuffer_config_attribs, NULL, 0,
> >> &pbuffer_config) < 0) {
> >> + weston_log("failed to choose EGL config for PbufferSurface");
> >> + return -1;
> >> + }
> >> +
> >> + gr->dummy_surface = eglCreatePbufferSurface(gr->egl_display,
> >> + pbuffer_config,
> >> + pbuffer_attribs);
> >> +
> >> + if (gr->dummy_surface == EGL_NO_SURFACE) {
> >> + weston_log("failed to create PbufferSurface\n");
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> >> void *native_window, const EGLint *attribs,
> >> const EGLint *visual_id, int n_ids)
> >> @@ -2956,10 +2998,27 @@ 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");
> >> +
> >> + if (gl_renderer_create_pbuffer_surface(gr) < 0)
> >> + goto fail_with_error;
> >> + }
> >> +
> >> wl_display_add_shm_format(ec->wl_display, WL_SHM_FORMAT_RGB565);
> >>
> >> wl_signal_init(&gr->destroy_signal);
> >>
> >> + if (gl_renderer_setup(ec, gr->dummy_surface) < 0) {
> >> + if (gr->dummy_surface != EGL_NO_SURFACE)
> >> + eglDestroySurface(gr->egl_display, gr->dummy_surface);
> >
> > To follow the error clean-up style, you'd have a new goto label
> > 'fail_with_dummy' or something, put the eglDestroySurface() call after
> > that, and then let it fall through to fail_with_error etc.
> >
> > But, not a big deal.
> >
> >> + goto fail_with_error;
> >> + }
> >> +
> >> return 0;
> >>
> >> fail_with_error:
> >
> > I filed a Mesa bug about the "libEGL warning: FIXME: egl/x11 doesn't
> > support front buffer rendering." because I think it's wrong:
> > https://bugs.freedesktop.org/show_bug.cgi?id=96694
> >
> > I don't think it should hurt anything though, so pushed:
> > ea0316a..28d240f master -> master
> >
> > The rest of the series is for another day.
> >
> >
> > Thanks,
> > pq
> >
>
> Hi, thanks for merging most of the patches.
>
> Do you want me to send another patch which fixes minor issues you've outlined?
If you want to, sure.
Thanks,
pq
pgpty9EiX8s9w.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
