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

[Pekka Paalanen]

On Tue, 28 Jun 2016 16:02:10 +0200
Armin Krezović <krezovic.armin at 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.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.  
> > 
> > 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.armin at 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
-------------- 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/20160628/88d93da4/attachment-0001.sig>