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

[Chad Versace]

On Mon 16 May 2016, Gurchetan Singh wrote:
> This change enables the creation of pbuffer
> surfaces on the surfaceless platform.
> 
> v3: Going back to single-buffered pbuffer
> plus additional code review changes
> ---
>  src/egl/drivers/dri2/egl_dri2.h |   7 +-
>  src/egl/drivers/dri2/platform_surfaceless.c | 213 
> +++-
>  2 files changed, 215 insertions(+), 5 deletions(-)


Hi Gurchetan,

I've returned from sabbatical. Please see my reply to your v2 thread.

Thanks for the changes.

Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Chad Versace]

On Mon 16 May 2016, Gurchetan Singh wrote:
> Hi Chad,
> 
> 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.

Hi Gurchetan,

I've returned from sabbatical.

Knowing that back-buffered pbuffer + the pastebin changes fixes a large
number of GLES3 tests, I'm interested in examining those changes more
closely. (You're pastebin link has expired, though, so I don't recall
the details).  If it's possible to apply your pastebin fixes and fix the
dEQP tests, while not regressing i965 under X11, then we should try to
do it.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Gurchetan Singh]

This change enables the creation of pbuffer
surfaces on the surfaceless platform.

v3: Going back to single-buffered pbuffer
plus additional code review changes
---
 src/egl/drivers/dri2/egl_dri2.h |   7 +-
 src/egl/drivers/dri2/platform_surfaceless.c | 213 +++-
 2 files changed, 215 insertions(+), 5 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index ddb5f39..938ba05 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -291,8 +291,13 @@ struct dri2_egl_surface
/* EGL-owned buffers */
__DRIbuffer   *local_buffers[__DRI_BUFFER_COUNT];
 #endif
-};
 
+#if defined(HAVE_SURFACELESS_PLATFORM)
+  __DRIimage   *front;
+  unsigned int visual;
+#endif
+
+};
 
 struct dri2_egl_config
 {
diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
b/src/egl/drivers/dri2/platform_surfaceless.c
index e0ddc12..8ce6162 100644
--- a/src/egl/drivers/dri2/platform_surfaceless.c
+++ b/src/egl/drivers/dri2/platform_surfaceless.c
@@ -37,8 +37,203 @@
 #include "egl_dri2_fallbacks.h"
 #include "loader.h"
 
+static __DRIimage*
+surfaceless_alloc_image(struct dri2_egl_display *dri2_dpy,
+ struct dri2_egl_surface *dri2_surf)
+{
+   return dri2_dpy->image->createImage(
+dri2_dpy->dri_screen,
+dri2_surf->base.Width,
+dri2_surf->base.Height,
+dri2_surf->visual,
+0,
+NULL);
+}
+
+static void
+surfaceless_free_images(struct dri2_egl_surface *dri2_surf)
+{
+   struct dri2_egl_display *dri2_dpy =
+  dri2_egl_display(dri2_surf->base.Resource.Display);
+
+   if (dri2_surf->front) {
+  dri2_dpy->image->destroyImage(dri2_surf->front);
+  dri2_surf->front = NULL;
+   }
+}
+
+static int
+surfaceless_image_get_buffers(__DRIdrawable *driDrawable,
+unsigned int format,
+uint32_t *stamp,
+void *loaderPrivate,
+uint32_t buffer_mask,
+struct __DRIimageList *buffers)
+{
+   struct dri2_egl_surface *dri2_surf = loaderPrivate;
+   struct dri2_egl_display *dri2_dpy =
+  dri2_egl_display(dri2_surf->base.Resource.Display);
+
+   buffers->image_mask = 0;
+   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 =
+surfaceless_alloc_image(dri2_dpy, dri2_surf);
+
+  buffers->image_mask |= __DRI_IMAGE_BUFFER_FRONT;
+  buffers->front = dri2_surf->front;
+   }
+
+   return 1;
+}
+
+static _EGLSurface *
+dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint 
type,
+_EGLConfig *conf, const EGLint *attrib_list)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+   struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
+   struct dri2_egl_surface *dri2_surf;
+   const __DRIconfig *config;
+
+   /* Make sure to calloc so all pointers
+* are originally NULL.
+*/
+   dri2_surf = calloc(1, sizeof *dri2_surf);
+
+   if (!dri2_surf) {
+  _eglError(EGL_BAD_ALLOC, "eglCreatePbufferSurface");
+  return NULL;
+   }
+
+   if (!_eglInitSurface(_surf->base, disp, type, conf, attrib_list))
+  goto cleanup_surface;
+
+   config = dri2_get_dri_config(dri2_conf, type,
+dri2_surf->base.GLColorspace);
+
+   if (!config)
+  goto cleanup_surface;
+
+   dri2_surf->dri_drawable =
+  (*dri2_dpy->dri2->createNewDrawable)(dri2_dpy->dri_screen, config,
+   dri2_surf);
+   if (dri2_surf->dri_drawable == NULL) {
+  _eglError(EGL_BAD_ALLOC, "dri2->createNewDrawable");
+  goto cleanup_surface;
+}
+
+   if (conf->RedSize == 5)
+  dri2_surf->visual = __DRI_IMAGE_FORMAT_RGB565;
+   else if (conf->AlphaSize == 0)
+  dri2_surf->visual = __DRI_IMAGE_FORMAT_XRGB;
+   else
+  dri2_surf->visual = __DRI_IMAGE_FORMAT_ARGB;
+
+   return _surf->base;
+
+   cleanup_surface:
+  free(dri2_surf);
+  return NULL;
+}
+
+static EGLBoolean
+surfaceless_destroy_surface(_EGLDriver *drv, _EGLDisplay 

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

[Gurchetan Singh]

Hi Chad,

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.

Best wishes,
Gurchetan

On Mon, May 16, 2016 at 12:36 PM, Chad Versace 
wrote:

> +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, >Base.Base);
> >
> > >>if (mesaVis->doubleBufferMode) {
> > >>  rb = intel_create_renderbuffer(rgbFormat, num_samples);
> > >>  _mesa_add_renderbuffer(fb, BUFFER_BACK_LEFT, >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, 

Re: [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, >Base.Base);
> 
> >>if (mesaVis->doubleBufferMode) {
> >>  rb = intel_create_renderbuffer(rgbFormat, num_samples);
> >>  _mesa_add_renderbuffer(fb, BUFFER_BACK_LEFT, >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 

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

[Gurchetan Singh]

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.

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, >Base.Base);

>>if (mesaVis->doubleBufferMode) {
>>  rb = intel_create_renderbuffer(rgbFormat, num_samples);
>>  _mesa_add_renderbuffer(fb, BUFFER_BACK_LEFT, >Base.Base)
>>}

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

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 just changed the single buffer defaults in that code snippet so I could
test on my surfaceless platform, we'll have query from the updated visuals
in a proper implementation)

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?

On Fri, May 6, 2016 at 9:27 PM, Chad Versace  wrote:

> On 05/06/2016 03:39 PM, Stéphane Marchesin wrote:
> > On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh
> >  wrote:
> >> This change enables the creation of pbuffer
> >> surfaces on the surfaceless platform.
> >>
> >> V2: Use double-buffered pbuffer configuration
> >
> > Reviewed-by: Stéphane Marchesin 
> >
> > Chad, do you also want to take a look at it?
>
> On a philosophical note, it's ironic that we're now creating a *surface*
> in the
> *surfaceless* platform. Don't you agree? Anyway, let's ignore the irony and
> focus on practical matters.
>
> There are a few bugs and minor style issues. See the comments below.
>
> There is also one major issue that needs discussion. I believe pbuffers
> are single-buffered, and that the single buffer is the back buffer.
>
> As precedent, platform_x11.c implements pbuffers as single-buffered.
>
> The relevant language in the EGL 1.5 spec is phrased badly, and could be
> interpreted either way: (a) pbuffers are double-buffered, or (b) pbuffers
> have
> only a back buffer.  If I recall correctly, an internal Khronos
> conversation in
> 2014 arrived at conclusion (b).  Here are the relevant quotes from the
> spec:
>
> - Some platforms may not allow rendering directly to the front buffer
> of
>   a window surface. When such windows are made current to a context,
> the
>   context will always have an EGL_RENDER_BUFFER attribute value of
>   EGL_BACK_BUFFER. From the client API point of view these surfaces
> have
>   only a back buffer and no front buffer, similar to pbuffer rendering
> (see
>   section 2.2.2).
>
> - Querying EGL_RENDER_BUFFER [with eglQueryContext()] returns the
> buffer
>   which client API rendering is requested to use. [...] For a pbuffer
>   surface, it is always EGL_BACK_BUFFER.
>
> - [When eglSwapBuffers() is called,] If surface is a back-buffered
> window
>   surface, then the color buffer is copied to the native window
> associated
>   with that surface. [Otherwise, if] surface is a single-buffered
> window,
>   pixmap, or pbuffer surface, eglSwapBuffers has no effect.
>
> The single-buffered nature has an impact on the implementation of
> eglSwapBuffers, according the last bullet. It's a no-op. As precedent,
> platform_x11.c correctly does nothing when swapping a pbuffer.
>
> If you think my interpretation of the spec is wrong, and that
> platform_x11.c is incorrect, then I'm open to discussing it. I'm
> especially interested to learn whether any non-Mesa EGL implementations
> treat pbuffers as double-buffered.
>
> (Also, this patch should probably set EGL_MIN/MAX_SWAP_INTERVAL to 0/0
> for pbuffer configs. But let's overlook that for now, as I don't think
> it's important for the initial implementation. Depending on how Google
> uses this patch, perhaps the swap interval bounds are never relevant.)
>
> >> ---
> >>  src/egl/drivers/dri2/egl_dri2.h |   8 +-
> >>  src/egl/drivers/dri2/platform_surfaceless.c | 219
> +++-
> >>  2 files changed, 222 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/egl/drivers/dri2/egl_dri2.h
> b/src/egl/drivers/dri2/egl_dri2.h
> >> index ddb5f39..ecf9c76 100644
> >> --- a/src/egl/drivers/dri2/egl_dri2.h
> >> +++ b/src/egl/drivers/dri2/egl_dri2.h
> >> @@ -291,8 +291,14 @@ struct dri2_egl_surface
> >> /* EGL-owned buffers */
> >> 

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

[Mark Janes]

Gurchetan Singh  writes:

> This change enables the creation of pbuffer
> surfaces on the surfaceless platform.
>
> V2: Use double-buffered pbuffer configuration
> ---
>  src/egl/drivers/dri2/egl_dri2.h |   8 +-
>  src/egl/drivers/dri2/platform_surfaceless.c | 219 
> +++-
>  2 files changed, 222 insertions(+), 5 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ddb5f39..ecf9c76 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -291,8 +291,14 @@ struct dri2_egl_surface
> /* EGL-owned buffers */
> __DRIbuffer   *local_buffers[__DRI_BUFFER_COUNT];
>  #endif
> -};
>  
> +#if defined(HAVE_SURFACELESS_PLATFORM)
> +  __DRIimage   *front;
> +  __DRIimage   *back;

The 'back' member clashes with the color_buffers members of the same
name, when HAVE_DRM_PLATFORM is defined.

> +  unsigned int format;
> +#endif
> +
> +};
>  
>  struct dri2_egl_config
>  {
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
> b/src/egl/drivers/dri2/platform_surfaceless.c
> index e0ddc12..08d5448 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -23,6 +23,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -37,9 +38,209 @@
>  #include "egl_dri2_fallbacks.h"
>  #include "loader.h"
>  
> +static __DRIimage*
> +surfaceless_alloc_image(struct dri2_egl_display *dri2_dpy,
> + struct dri2_egl_surface *dri2_surf)
> +{
> +   __DRIimage *img = NULL;
> +
> +   img = dri2_dpy->image->createImage(
> +dri2_dpy->dri_screen,
> +dri2_surf->base.Width,
> +dri2_surf->base.Height,
> +dri2_surf->format,
> +0,
> +NULL);
> +
> +   return img;
> +}
> +
> +static void
> +surfaceless_free_images(struct dri2_egl_surface *dri2_surf)
> +{
> +   struct dri2_egl_display *dri2_dpy =
> +  dri2_egl_display(dri2_surf->base.Resource.Display);
> +
> +   if(dri2_surf->front) {
> +  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;
> +}
> +
> +static int
> +surfaceless_image_get_buffers(__DRIdrawable *driDrawable,
> +unsigned int format,
> +uint32_t *stamp,
> +void *loaderPrivate,
> +uint32_t buffer_mask,
> +struct __DRIimageList *buffers)
> +{
> +   struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +   struct dri2_egl_display *dri2_dpy =
> +  dri2_egl_display(dri2_surf->base.Resource.Display);
> +   buffers->image_mask = 0;
> +   buffers->front = NULL;
> +   buffers->back = NULL;
> +
> +   if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
> +  if (!dri2_surf->front)
> + dri2_surf->front =
> +surfaceless_alloc_image(dri2_dpy, dri2_surf);
> +
> +  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;
> +}
> +
> +static _EGLSurface *
> +dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint 
> type,
> +_EGLConfig *conf, void *native_surface,
> +const EGLint *attrib_list)
> +{
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
> +   struct dri2_egl_surface *dri2_surf;
> +   const __DRIconfig *config;
> +   bool srgb;
> +
> +   /* Make sure to calloc so all pointers
> +* are originally NULL.
> +*/
> +   dri2_surf = calloc(1, sizeof *dri2_surf);
> +
> +   if(!dri2_surf)
> +  return NULL;
> +
> +   if (!_eglInitSurface(_surf->base, disp, type, conf, attrib_list))
> +  goto cleanup_surface;
> +
> +   /* Only double buffered configurations exist at this point (single 
> buffered
> +* 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];
> +
> +   if (!config)
> +  goto 

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

[Chad Versace]

On 05/06/2016 03:39 PM, Stéphane Marchesin wrote:
> On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh
>  wrote:
>> This change enables the creation of pbuffer
>> surfaces on the surfaceless platform.
>>
>> V2: Use double-buffered pbuffer configuration
> 
> Reviewed-by: Stéphane Marchesin 
> 
> Chad, do you also want to take a look at it?

On a philosophical note, it's ironic that we're now creating a *surface* in the
*surfaceless* platform. Don't you agree? Anyway, let's ignore the irony and
focus on practical matters.

There are a few bugs and minor style issues. See the comments below.

There is also one major issue that needs discussion. I believe pbuffers
are single-buffered, and that the single buffer is the back buffer.

As precedent, platform_x11.c implements pbuffers as single-buffered.

The relevant language in the EGL 1.5 spec is phrased badly, and could be
interpreted either way: (a) pbuffers are double-buffered, or (b) pbuffers have
only a back buffer.  If I recall correctly, an internal Khronos conversation in
2014 arrived at conclusion (b).  Here are the relevant quotes from the spec:

- Some platforms may not allow rendering directly to the front buffer of
  a window surface. When such windows are made current to a context, the
  context will always have an EGL_RENDER_BUFFER attribute value of
  EGL_BACK_BUFFER. From the client API point of view these surfaces have
  only a back buffer and no front buffer, similar to pbuffer rendering (see
  section 2.2.2).

- Querying EGL_RENDER_BUFFER [with eglQueryContext()] returns the buffer
  which client API rendering is requested to use. [...] For a pbuffer
  surface, it is always EGL_BACK_BUFFER.

- [When eglSwapBuffers() is called,] If surface is a back-buffered window
  surface, then the color buffer is copied to the native window associated
  with that surface. [Otherwise, if] surface is a single-buffered window,
  pixmap, or pbuffer surface, eglSwapBuffers has no effect.

The single-buffered nature has an impact on the implementation of
eglSwapBuffers, according the last bullet. It's a no-op. As precedent,
platform_x11.c correctly does nothing when swapping a pbuffer.

If you think my interpretation of the spec is wrong, and that
platform_x11.c is incorrect, then I'm open to discussing it. I'm
especially interested to learn whether any non-Mesa EGL implementations
treat pbuffers as double-buffered.

(Also, this patch should probably set EGL_MIN/MAX_SWAP_INTERVAL to 0/0
for pbuffer configs. But let's overlook that for now, as I don't think
it's important for the initial implementation. Depending on how Google
uses this patch, perhaps the swap interval bounds are never relevant.)

>> ---
>>  src/egl/drivers/dri2/egl_dri2.h |   8 +-
>>  src/egl/drivers/dri2/platform_surfaceless.c | 219 
>> +++-
>>  2 files changed, 222 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.h 
>> b/src/egl/drivers/dri2/egl_dri2.h
>> index ddb5f39..ecf9c76 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.h
>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>> @@ -291,8 +291,14 @@ struct dri2_egl_surface
>> /* EGL-owned buffers */
>> __DRIbuffer   *local_buffers[__DRI_BUFFER_COUNT];
>>  #endif
>> -};
>>
>> +#if defined(HAVE_SURFACELESS_PLATFORM)
>> +  __DRIimage   *front;
>> +  __DRIimage   *back;
>> +  unsigned int format;
>> +#endif
>> +
>> +};
>>
>>  struct dri2_egl_config
>>  {
>> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
>> b/src/egl/drivers/dri2/platform_surfaceless.c
>> index e0ddc12..08d5448 100644
>> --- a/src/egl/drivers/dri2/platform_surfaceless.c
>> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
>> @@ -23,6 +23,7 @@
>>   * DEALINGS IN THE SOFTWARE.
>>   */
>>
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -37,9 +38,209 @@
>>  #include "egl_dri2_fallbacks.h"
>>  #include "loader.h"
>>
>> +static __DRIimage*
>> +surfaceless_alloc_image(struct dri2_egl_display *dri2_dpy,
>> + struct dri2_egl_surface *dri2_surf)
>> +{
>> +   __DRIimage *img = NULL;
>> +
>> +   img = dri2_dpy->image->createImage(
>> +dri2_dpy->dri_screen,
>> +dri2_surf->base.Width,
>> +dri2_surf->base.Height,
>> +dri2_surf->format,
>> +0,
>> +NULL);
>> +
>> +   return img;
>> +}

The img variable doesn't do much here. This function would be cleaner
if it had no local variable.

>> +static void
>> +surfaceless_free_images(struct dri2_egl_surface *dri2_surf)
>> +{
>> +   struct dri2_egl_display *dri2_dpy =
>> +  dri2_egl_display(dri2_surf->base.Resource.Display);
>> +
>> +   if(dri2_surf->front) {
>> +  dri2_dpy->image->destroyImage(dri2_surf->front);
>> +  dri2_surf->front = NULL;
>> +   }
>> +   if(dri2_surf->back) {
>> +  

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

[Stéphane Marchesin]

On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh
 wrote:
> This change enables the creation of pbuffer
> surfaces on the surfaceless platform.
>
> V2: Use double-buffered pbuffer configuration

Reviewed-by: Stéphane Marchesin 

Chad, do you also want to take a look at it?


> ---
>  src/egl/drivers/dri2/egl_dri2.h |   8 +-
>  src/egl/drivers/dri2/platform_surfaceless.c | 219 
> +++-
>  2 files changed, 222 insertions(+), 5 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ddb5f39..ecf9c76 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -291,8 +291,14 @@ struct dri2_egl_surface
> /* EGL-owned buffers */
> __DRIbuffer   *local_buffers[__DRI_BUFFER_COUNT];
>  #endif
> -};
>
> +#if defined(HAVE_SURFACELESS_PLATFORM)
> +  __DRIimage   *front;
> +  __DRIimage   *back;
> +  unsigned int format;
> +#endif
> +
> +};
>
>  struct dri2_egl_config
>  {
> diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
> b/src/egl/drivers/dri2/platform_surfaceless.c
> index e0ddc12..08d5448 100644
> --- a/src/egl/drivers/dri2/platform_surfaceless.c
> +++ b/src/egl/drivers/dri2/platform_surfaceless.c
> @@ -23,6 +23,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -37,9 +38,209 @@
>  #include "egl_dri2_fallbacks.h"
>  #include "loader.h"
>
> +static __DRIimage*
> +surfaceless_alloc_image(struct dri2_egl_display *dri2_dpy,
> + struct dri2_egl_surface *dri2_surf)
> +{
> +   __DRIimage *img = NULL;
> +
> +   img = dri2_dpy->image->createImage(
> +dri2_dpy->dri_screen,
> +dri2_surf->base.Width,
> +dri2_surf->base.Height,
> +dri2_surf->format,
> +0,
> +NULL);
> +
> +   return img;
> +}
> +
> +static void
> +surfaceless_free_images(struct dri2_egl_surface *dri2_surf)
> +{
> +   struct dri2_egl_display *dri2_dpy =
> +  dri2_egl_display(dri2_surf->base.Resource.Display);
> +
> +   if(dri2_surf->front) {
> +  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;
> +}
> +
> +static int
> +surfaceless_image_get_buffers(__DRIdrawable *driDrawable,
> +unsigned int format,
> +uint32_t *stamp,
> +void *loaderPrivate,
> +uint32_t buffer_mask,
> +struct __DRIimageList *buffers)
> +{
> +   struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +   struct dri2_egl_display *dri2_dpy =
> +  dri2_egl_display(dri2_surf->base.Resource.Display);
> +   buffers->image_mask = 0;
> +   buffers->front = NULL;
> +   buffers->back = NULL;
> +
> +   if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
> +  if (!dri2_surf->front)
> + dri2_surf->front =
> +surfaceless_alloc_image(dri2_dpy, dri2_surf);
> +
> +  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;
> +}
> +
> +static _EGLSurface *
> +dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint 
> type,
> +_EGLConfig *conf, void *native_surface,
> +const EGLint *attrib_list)
> +{
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +   struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
> +   struct dri2_egl_surface *dri2_surf;
> +   const __DRIconfig *config;
> +   bool srgb;
> +
> +   /* Make sure to calloc so all pointers
> +* are originally NULL.
> +*/
> +   dri2_surf = calloc(1, sizeof *dri2_surf);
> +
> +   if(!dri2_surf)
> +  return NULL;
> +
> +   if (!_eglInitSurface(_surf->base, disp, type, conf, attrib_list))
> +  goto cleanup_surface;
> +
> +   /* Only double buffered configurations exist at this point (single 
> buffered
> +* 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];
> +
> +   if (!config)
> +

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

[Gurchetan Singh]

This change enables the creation of pbuffer
surfaces on the surfaceless platform.

V2: Use double-buffered pbuffer configuration
---
 src/egl/drivers/dri2/egl_dri2.h |   8 +-
 src/egl/drivers/dri2/platform_surfaceless.c | 219 +++-
 2 files changed, 222 insertions(+), 5 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index ddb5f39..ecf9c76 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -291,8 +291,14 @@ struct dri2_egl_surface
/* EGL-owned buffers */
__DRIbuffer   *local_buffers[__DRI_BUFFER_COUNT];
 #endif
-};
 
+#if defined(HAVE_SURFACELESS_PLATFORM)
+  __DRIimage   *front;
+  __DRIimage   *back;
+  unsigned int format;
+#endif
+
+};
 
 struct dri2_egl_config
 {
diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
b/src/egl/drivers/dri2/platform_surfaceless.c
index e0ddc12..08d5448 100644
--- a/src/egl/drivers/dri2/platform_surfaceless.c
+++ b/src/egl/drivers/dri2/platform_surfaceless.c
@@ -23,6 +23,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -37,9 +38,209 @@
 #include "egl_dri2_fallbacks.h"
 #include "loader.h"
 
+static __DRIimage*
+surfaceless_alloc_image(struct dri2_egl_display *dri2_dpy,
+ struct dri2_egl_surface *dri2_surf)
+{
+   __DRIimage *img = NULL;
+
+   img = dri2_dpy->image->createImage(
+dri2_dpy->dri_screen,
+dri2_surf->base.Width,
+dri2_surf->base.Height,
+dri2_surf->format,
+0,
+NULL);
+
+   return img;
+}
+
+static void
+surfaceless_free_images(struct dri2_egl_surface *dri2_surf)
+{
+   struct dri2_egl_display *dri2_dpy =
+  dri2_egl_display(dri2_surf->base.Resource.Display);
+
+   if(dri2_surf->front) {
+  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;
+}
+
+static int
+surfaceless_image_get_buffers(__DRIdrawable *driDrawable,
+unsigned int format,
+uint32_t *stamp,
+void *loaderPrivate,
+uint32_t buffer_mask,
+struct __DRIimageList *buffers)
+{
+   struct dri2_egl_surface *dri2_surf = loaderPrivate;
+   struct dri2_egl_display *dri2_dpy =
+  dri2_egl_display(dri2_surf->base.Resource.Display);
+   buffers->image_mask = 0;
+   buffers->front = NULL;
+   buffers->back = NULL;
+
+   if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
+  if (!dri2_surf->front)
+ dri2_surf->front =
+surfaceless_alloc_image(dri2_dpy, dri2_surf);
+
+  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;
+}
+
+static _EGLSurface *
+dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint 
type,
+_EGLConfig *conf, void *native_surface,
+const EGLint *attrib_list)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+   struct dri2_egl_config *dri2_conf = dri2_egl_config(conf);
+   struct dri2_egl_surface *dri2_surf;
+   const __DRIconfig *config;
+   bool srgb;
+
+   /* Make sure to calloc so all pointers
+* are originally NULL.
+*/
+   dri2_surf = calloc(1, sizeof *dri2_surf);
+
+   if(!dri2_surf)
+  return NULL;
+
+   if (!_eglInitSurface(_surf->base, disp, type, conf, attrib_list))
+  goto cleanup_surface;
+
+   /* Only double buffered configurations exist at this point (single buffered
+* 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];
+
+   if (!config)
+  goto cleanup_surface;
+
+   dri2_surf->dri_drawable =
+  (*dri2_dpy->dri2->createNewDrawable)(dri2_dpy->dri_screen, config,
+   dri2_surf);
+   if (dri2_surf->dri_drawable == NULL) {
+  _eglError(EGL_BAD_ALLOC, "dri2->createNewDrawable");
+  goto cleanup_surface;
+}
+
+   if (conf->RedSize == 5)
+  dri2_surf->format = __DRI_IMAGE_FORMAT_RGB565;
+   else if (conf->AlphaSize == 0)
+  dri2_surf->format =