[Mesa-dev] [PATCH] Added pbuffer hooks for surfaceless platform

[Chad Versace]

+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:
> 
> http://pastebin.com/28ddebWF

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