<div dir="ltr">Hi Chad,<div> </div><div>Yes, we are running dEQP on the surfaceless platform. The front-buffered pbuffer approach mostly works, though with a double-buffered pbuffer or back-buffered pbuffer + the pastebin changes we do pass an additional 70 GLES3 tests. It is probably best to follow what X11 does to avoid breaking things. I'll have an updated patch shortly.</div><div> </div><div>Best wishes,</div><div>Gurchetan</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, May 16, 2016 at 12:36 PM, Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+Kristian, who also has an interest EGL On Tue 10 May 2016, Gurchetan Singh wrote: > Hi Chad, > > Thanks for the review and good suggestions. I am interested to know your > opinion regarding an issue that comes up when implementing a back-buffered > pbuffer. The problem is we'll be creating a front renderbuffer, not a back > renderbuffer if we pass in single buffered visuals. I was afraid that this would cause a chain of issues. > For example, in intelCreateBuffer in the i965 driver, the back buffer is > only created when double-buffered visuals are passed in: > > >>_mesa_add_renderbuffer(fb, BUFFER_FRONT_LEFT, &rb->Base.Base); > > >>if (mesaVis->doubleBufferMode) { > >> rb = intel_create_renderbuffer(rgbFormat, num_samples); > >> _mesa_add_renderbuffer(fb, BUFFER_BACK_LEFT, &rb->Base.Base) > >>} Right, that's a problem. > Due to this, with a single-buffered configuration, we'll have to do the > following in surfaceless_image_get_buffers: > > >>buffers->image_mask |= __DRI_IMAGE_BUFFER_FRONT > > not this: > > >>buffers->image_mask |= __DRI_IMAGE_BUFFER_BACK I think the above snippet is the correct solution. The surfaceless platform should assign single-buffered configs to the pbuffers, and create a *front* buffer for the pbuffer but no back buffer. > If this a problem, we'll have to pass in a parameter in the visual > indicating we're trying to create an EGL pbuffer (i.e, > mesaVis->is_egl_pbuffer). Then we'll set the proper renderbuffer and > framebuffer options from that config in several places. Some these places > include: > > <a href="http://pastebin.com/28ddebWF" rel="noreferrer" target="_blank">http://pastebin.com/28ddebWF</a> I think you should avoid fixing all the things the pastebin patch. In attempting to fix the world, I believe there is a lot of risk that the "fixed" code will break existing users of pbuffers and, more importantly, pixmaps. The Mesa/i965 code around pixmaps and pbuffers have many hidden assumptions, I'm afraid of breaking those assumptions. > Is the fact we'll work with the default front render buffer a problem? If > so, what's the best way to fix the issue? Yes, it's a small problem. But I think it's an acceptable compromise, and the correct way forward. I investigated the EGL/X11 platform more deeply, to learn exactly how it handles pbuffers. After a good hour in gdb, I discovered that it handles pbuffers exactly as you suggested in this email: the X11 platform assigns single-buffered configs to pbuffers, and creates __DRI_IMAGE_BUFFER_FRONT but never __DRI_IMAGE_BUFFER_BACK. In experimenting with gdb, I played with the approach you suggested by modifying your patch. Below is my experimental diff against your patch. As far as I can tell, your patch + the diff works as expected. It succeeds in making Piglit's egl-create-pbuffer-surface test pass (though I had to also hack the Piglit test to remove the X11 dependency). How are you testing this patch? Even more importantly, what prompted you to write this patch?!? Is this patch related to dEQP somehow? By the way, I'm currently on my sabbatical, so I'm not checking mesa-dev daily. I'll do my best to check my email bi-daily, though, until we get this patch resolved. ------ Hacky experimental diff diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h index ecf9c76..5fe7697 100644 --- a/src/egl/drivers/dri2/egl_dri2.h +++ b/src/egl/drivers/dri2/egl_dri2.h @@ -294,7 +294,10 @@ struct dri2_egl_surface #if defined(HAVE_SURFACELESS_PLATFORM) __DRIimage *front; - __DRIimage *back; + + /* FIXME: The 'format' field breaks the build because it conflicts with + * the same field in the Wayland section. + * */ unsigned int format; #endif diff --git a/src/egl/drivers/dri2/platform_surfaceless.c b/src/egl/drivers/dri2/platform_surfaceless.c index 08d5448..745d481 100644 --- a/src/egl/drivers/dri2/platform_surfaceless.c +++ b/src/egl/drivers/dri2/platform_surfaceless.c @@ -65,20 +65,13 @@ surfaceless_free_images(struct dri2_egl_surface *dri2_surf) dri2_dpy->image->destroyImage(dri2_surf->front); dri2_surf->front = NULL; } - if(dri2_surf->back) { - dri2_dpy->image->destroyImage(dri2_surf->back); - dri2_surf->back = NULL; - } } static EGLBoolean surfaceless_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *draw) { - struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw); - __DRIimage *temp = dri2_surf->back; - - dri2_surf->back = dri2_surf->front; - dri2_surf->front = temp; + /* The EGL spec mandates that eglSwapBuffers is a no-op for pbuffers. */ + return EGL_TRUE; } static int @@ -96,6 +89,19 @@ surfaceless_image_get_buffers(__DRIdrawable *driDrawable, buffers->front = NULL; buffers->back = NULL; + /* The EGL 1.5 spec states that pbuffers are single-buffered. Specifically, + * the spec states that they have a back buffer but no front buffer, in + * contrast to pixmaps, which have a front buffer but no back buffer. + * + * Single-buffered surfaces with no front buffer confuse Mesa; so We deviate + * from the spec, following the precedent of Mesa's EGL X11 platform. The + * X11 platform correctly assigns pbuffers to single-buffered configs, but + * assigns the pbuffer a front buffer instead of a back buffer. + * + * Pbuffers in the X11 platform mostly work today, so let's just copy its + * behavior instead of trying to fix (and hence potentially breaking) the + * world. + */ if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) { if (!dri2_surf->front) dri2_surf->front = @@ -104,14 +110,6 @@ surfaceless_image_get_buffers(__DRIdrawable *driDrawable, buffers->image_mask |= __DRI_IMAGE_BUFFER_FRONT; buffers->front = dri2_surf->front; } - if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) { - if (!dri2_surf->back) - dri2_surf->back = - surfaceless_alloc_image(dri2_dpy, dri2_surf); - - buffers->image_mask |= __DRI_IMAGE_BUFFER_BACK; - buffers->back = dri2_surf->back; - } return 1; } @@ -142,7 +140,7 @@ dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type, * configs were filtered out in surfaceless_add_configs_for_visuals). */ srgb = (dri2_surf->base.GLColorspace == EGL_GL_COLORSPACE_SRGB_KHR); - config = dri2_conf->dri_double_config[srgb]; + config = dri2_conf->dri_single_config[srgb]; if (!config) goto cleanup_surface; @@ -211,19 +209,10 @@ surfaceless_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy) count = 0; for (i = 0; i < ARRAY_SIZE(visuals); i++) { for (j = 0; dri2_dpy->driver_configs[j]; j++) { - const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT; struct dri2_egl_config *dri2_conf; - unsigned int double_buffered = 0; - - dri2_dpy->core->getConfigAttrib(dri2_dpy->driver_configs[j], - __DRI_ATTRIB_DOUBLE_BUFFER, &double_buffered); - - /* support only double buffered configs */ - if (!double_buffered) - continue; dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[j], - count + 1, surface_type, NULL, visuals[i]); + count + 1, EGL_PBUFFER_BIT, NULL, visuals[i]); if (dri2_conf) count++; } -- 2.8.1 </blockquote></div> </div>