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, &rb->Base.Base);
>>if (mesaVis->doubleBufferMode) {
>> rb = intel_create_renderbuffer(rgbFormat, num_samples);
>> _mesa_add_renderbuffer(fb, BUFFER_BACK_LEFT, &rb->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 <chad.vers...@intel.com> wrote:
> On 05/06/2016 03:39 PM, Stéphane Marchesin wrote:
> > On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh
> > <gurchetansi...@chromium.org> wrote:
> >> This change enables the creation of pbuffer
> >> surfaces on the surfaceless platform.
> >>
> >> V2: Use double-buffered pbuffer configuration
> >
> > Reviewed-by: Stéphane Marchesin <marc...@chromium.org>
> >
> > 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 <stdbool.h>
> >> #include <stdlib.h>
> >> #include <stdio.h>
> >> #include <string.h>
> >> @@ -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) {
> >> + dri2_dpy->image->destroyImage(dri2_surf->back);
> >> + dri2_surf->back = NULL;
> >> + }
> >> +}
>
> The style here needs fixing. The Mesa code style uses a space after 'if'.
> Typically,
> separate 'if' statements are also separated by a newline.
>
> if (meow) {
> moo;
> }
>
> if (woof) {
> quack;
> }
>
> >> +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;
> >> +}
>
> As discussed above, I believe the pbuffer should possess only a back
> buffer. And the EGL spec mandates eglSwapBuffers be a no-op for
> pbuffers.
>
> However, if you were going to swap buffers like this, the EGL spec would
> require that you flush the current drawable before swapping. To
> accomplish the flush, this function would need to call
> egl_dri2.c:dri2_flush_drawable_for_swapbuffers(). But that's all moot,
> as this function should actually be a no-op.
>
> >> +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;
>
> Minor style issue. Please insert a newline between the "block" of variable
> declarations and the first non-declaration line. Consistency with other
> parts of Mesa helps everyone involved with the project.
>
> >> +
> >> + 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;
> >> +}
>
> Again, please separate the 'if' statements with a newline.
>
> >> +static _EGLSurface *
> >> +dri2_surfaceless_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
> EGLint type,
> >> + _EGLConfig *conf, void *native_surface,
> >> + const EGLint *attrib_list)
>
> The 'native_surface' parameter is misleading because there exist no native
> surfaces
> in the surfaceless platform. Please remove that parameter.
>
> >> +{
> >> + 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;
>
> The above error handling should be:
>
> if (!dri2_surf) {
> _eglError(EGL_BAD_ALLOC, "eglCreatePbufferSurface");
> return NULL;
> }
>
> >> +
> >> + if (!_eglInitSurface(&dri2_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 = __DRI_IMAGE_FORMAT_XRGB8888;
> >> + else
> >> + dri2_surf->format = __DRI_IMAGE_FORMAT_ARGB8888;
> >> +
> >> + return &dri2_surf->base;
> >> +
> >> + cleanup_surface:
> >> + free(dri2_surf);
> >> + return NULL;
> >> +}
> >> +
> >> +static EGLBoolean
> >> +surfaceless_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp,
> _EGLSurface *surf)
> >> +{
> >> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> >> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> >> +
> >> + if (!_eglPutSurface(surf))
> >> + return EGL_TRUE;
> >> +
> >> + surfaceless_free_images(dri2_surf);
> >> +
> >> + (*dri2_dpy->core->destroyDrawable)(dri2_surf->dri_drawable);
> >> +
> >> + free(dri2_surf);
> >> + return EGL_TRUE;
> >> +}
> >> +
> >> +static _EGLSurface *
> >> +dri2_surfaceless_create_pbuffer_surface(_EGLDriver *drv, _EGLDisplay
> *disp,
> >> + _EGLConfig *conf, const EGLint
> *attrib_list)
> >> +{
> >> + return dri2_surfaceless_create_surface(drv, disp, EGL_PBUFFER_BIT,
> conf,
> >> + NULL, attrib_list);
> >> +}
> >> +
> >> +static EGLBoolean
> >> +surfaceless_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
> >> +{
> >> +
> >> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> >> +
> >> + unsigned int visuals[3][4] = {
> >> + { 0xff0000, 0xff00, 0xff, 0xff000000 }, // ARGB8888
> >> + { 0xff0000, 0xff00, 0xff, 0x0 }, // RGB888
> >> + { 0xf800, 0x7e0, 0x1f, 0x0 }, // RGB565
> >> + };
>
> This function assumes the GL/GLES driver supports these three formats,
> in the specified bit order. That assumption holds for Intel, but may not
> hold for other drivers.
>
> A similar oversight in
> platform_android.c:droid_add_configs_for_visuals(), which also hardcodes
> a list of formats without querying the driver, causes crashes on Android
> on Intel platforms (according to what a coworker told me). Therefore,
> assumptions like this cause real bugs.
>
> Don't advertise configs with these formats without first confirming that
> the driver supports them by querying
> dri2_dpy->core->getConfigAttrib(...,
> __DRI_ATTRIB_RED/GREEN/BLUE/ALPHA_MASK, ...)
> Look at platform_drm.c for a good example.
>
> >> +
> >> + int count, i, j;
> >> +
> >> + 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;
>
> It's incorrect to advertise EGL_WINDOW_BIT. The surfaceless platform is
> only capable of
> eglCreatePbufferSurface(), not eglCreateWindowSurface().
>
> >> + 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]);
> >> + if (dri2_conf)
> >> + count++;
> >> + }
> >> + }
> >> +
> >> + if (!count)
> >> + _eglLog(_EGL_DEBUG, "Can't create surfaceless visuals");
> >> +
> >> + return (count != 0);
> >> +}
> >> +
> >> static struct dri2_egl_display_vtbl dri2_surfaceless_display_vtbl = {
> >> .create_pixmap_surface = dri2_fallback_create_pixmap_surface,
> >> + .create_pbuffer_surface = dri2_surfaceless_create_pbuffer_surface,
> >> + .destroy_surface = surfaceless_destroy_surface,
> >> .create_image = dri2_create_image_khr,
> >> + .swap_buffers = surfaceless_swap_buffers,
> >> .swap_interval = dri2_fallback_swap_interval,
> >> .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage,
> >> .swap_buffers_region = dri2_fallback_swap_buffers_region,
> >> @@ -48,6 +249,7 @@ static struct dri2_egl_display_vtbl
> dri2_surfaceless_display_vtbl = {
> >> .query_buffer_age = dri2_fallback_query_buffer_age,
> >> .create_wayland_buffer_from_image =
> dri2_fallback_create_wayland_buffer_from_image,
> >> .get_sync_values = dri2_fallback_get_sync_values,
> >> + .get_dri_drawable = dri2_surface_get_dri_drawable,
> >> };
> >>
> >> static void
> >> @@ -72,6 +274,12 @@ surfaceless_get_buffers_with_format(__DRIdrawable *
> driDrawable,
> >> return dri2_surf->buffers;
> >> }
> >>
> >> +static const __DRIimageLoaderExtension image_loader_extension = {
> >> + .base = { __DRI_IMAGE_LOADER, 1 },
> >> + .getBuffers = surfaceless_image_get_buffers,
> >> + .flushFrontBuffer = surfaceless_flush_front_buffer,
> >> +};
> >> +
> >> #define DRM_RENDER_DEV_NAME "%s/renderD%d"
> >>
> >> EGLBoolean
> >> @@ -127,7 +335,7 @@ dri2_initialize_surfaceless(_EGLDriver *drv,
> _EGLDisplay *disp)
> >> dri2_dpy->dri2_loader_extension.getBuffersWithFormat =
> >> surfaceless_get_buffers_with_format;
> >>
> >> - dri2_dpy->extensions[0] = &dri2_dpy->dri2_loader_extension.base;
> >> + dri2_dpy->extensions[0] = &image_loader_extension.base;
> >> dri2_dpy->extensions[1] = &image_lookup_extension.base;
> >> dri2_dpy->extensions[2] = &use_invalidate.base;
> >> dri2_dpy->extensions[3] = NULL;
> >> @@ -137,9 +345,9 @@ dri2_initialize_surfaceless(_EGLDriver *drv,
> _EGLDisplay *disp)
> >> goto cleanup_driver;
> >> }
> >>
> >> - for (i = 0; dri2_dpy->driver_configs[i]; i++) {
> >> - dri2_add_config(disp, dri2_dpy->driver_configs[i],
> >> - i + 1, EGL_WINDOW_BIT, NULL, NULL);
> >> + if (!surfaceless_add_configs_for_visuals(drv, disp)) {
> >> + err = "DRI2: failed to add configs";
> >> + goto cleanup_screen;
> >> }
> >>
> >> disp->Extensions.KHR_image_base = EGL_TRUE;
> >> @@ -151,6 +359,9 @@ dri2_initialize_surfaceless(_EGLDriver *drv,
> _EGLDisplay *disp)
> >>
> >> return EGL_TRUE;
> >>
> >> +cleanup_screen:
> >> + dri2_dpy->core->destroyScreen(dri2_dpy->dri_screen);
> >> +
> >> cleanup_driver:
> >> dlclose(dri2_dpy->driver);
> >> free(dri2_dpy->driver_name);
> >> --
> >> 2.1.2
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
