<div dir="ltr">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: <a href="http://pastebin.com/28ddebWF">http://pastebin.com/28ddebWF</a> (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?</div><div class="gmail_extra"> <div class="gmail_quote">On Fri, May 6, 2016 at 9:27 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">On 05/06/2016 03:39 PM, Stéphane Marchesin wrote: > On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh > <<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> wrote: >> This change enables the creation of pbuffer >> surfaces on the surfaceless platform. >> >> V2: Use double-buffered pbuffer configuration > > Reviewed-by: Stéphane Marchesin <<a href="mailto:marcheu@chromium.org">marcheu@chromium.org</a>> > > 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.) <div><div class="h5"> >> --- >> 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; >> +} </div></div>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"); <div><div class="h5"> 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 >> + }; </div></div>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(). <div class="HOEnZb"><div class="h5"> >> + 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 >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div>