[Mesa-dev] [PATCH] Added pbuffer hooks for surfaceless platform
Chad Versace
chad.versace at intel.com
Sat May 7 04:27:14 UTC 2016
On 05/06/2016 03:39 PM, Stéphane Marchesin wrote:
> On Fri, May 6, 2016 at 3:32 PM, Gurchetan Singh
> <gurchetansingh at 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 <marcheu at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list