[Mesa-dev] EGL/android: pbuffer implementation.

[Tomasz Figa]

Hi,

On Wed, Nov 16, 2016 at 11:11 AM, Liu Zhiquan <zhiquan.liu at intel.com> wrote:
> mesa android path didn't support pbuffer, so add pbuffer support to
> fix most deqp and cts pbuffer test cases fail;
> add single buffer config to support pbuffer, and create image for
> pbuffer when pbuffer type is front surface.
> The EGL 1.5 spec states that pbuffers have a back buffer but no front
> 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.
>
> Test status: android CTS EGL pbuffer test can run without native crash.
> test:[DEQP,EGL]all deqp pbuffer case passed.
>
> V3: update commit message and code review changes.
>
> Signed-off-by: Liu Zhiquan <zhiquan.liu at intel.com>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
> ---
>  src/egl/drivers/dri2/egl_dri2.h         |  3 +-
>  src/egl/drivers/dri2/platform_android.c | 98 +++++++++++++++++++++++++--------
>  2 files changed, 78 insertions(+), 23 deletions(-)
>

Looks like this patch has already landed, but please let me try to
confirm some things here anyway. Would you mind keeping me on CC for
any future patches for the EGL/Android module? (I believe
scripts/get_reviewer.pl should already include me on the list of
suggested reviewers, similarly for Rob Herring, who did a great job
before helping us with testing on platforms other than i915.)

[snip]

> @@ -353,6 +353,18 @@ droid_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>        dri2_surf->window->common.decRef(&dri2_surf->window->common);
>     }
>
> +   if (dri2_surf->dri_image_back) {
> +      _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_back", __func__, __LINE__);
> +      dri2_dpy->image->destroyImage(dri2_surf->dri_image_back);
> +      dri2_surf->dri_image_back = NULL;
> +   }

Is this a fix for another bug by any chance? Note that we already take
care of dri_image_back for window surfaces in
droid_window_cancel_buffer(), which calls
droid_window_enqueue_buffer(), which does the real free on the image.
It doesn't hurt to have it here as well, though, so treat this just as
a random thought of mine.

> +
> +   if (dri2_surf->dri_image_front) {
> +      _eglLog(_EGL_DEBUG, "%s : %d : destroy dri_image_front", __func__, __LINE__);
> +      dri2_dpy->image->destroyImage(dri2_surf->dri_image_front);
> +      dri2_surf->dri_image_front = NULL;
> +   }
> +
>     (*dri2_dpy->core->destroyDrawable)(dri2_surf->dri_drawable);
>
>     free(dri2_surf);

[snip]

> @@ -439,25 +451,75 @@ droid_image_get_buffers(__DRIdrawable *driDrawable,
>                    struct __DRIimageList *images)
>  {
>     struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +   struct dri2_egl_display *dri2_dpy =
> +      dri2_egl_display(dri2_surf->base.Resource.Display);
>
>     images->image_mask = 0;
> +   images->front = NULL;
> +   images->back = NULL;

I'm not 100% sure this is the expected behavior of this function. My
understanding was that image_mask and error code would guard the other
members. It would make sense since typically if a function fails it
should keep the passed writable arguments unchanged. Is there a
precise description of the semantics used by the image loader
extension written down somewhere?

>
>     if (update_buffers(dri2_surf) < 0)
>        return 0;
>
>     if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
> -      /*
> -       * We don't support front buffers and GLES doesn't require them for
> -       * window surfaces, but some DRI drivers will request them anyway.
> -       * We just ignore such request as other platforms backends do.
> +      if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> +         /* According current EGL spec,
> +          * front buffer rendering for window surface is not supported now */
> +         _eglLog(_EGL_WARNING,
> +               "%s:%d front buffer rendering for window surface is not supported!\n",
> +               __func__, __LINE__);
> +         return 0;

This is a semantic change and according to the old comment it might
break some drivers ("We don't support front buffers and GLES doesn't
require them for window surfaces, but some DRI drivers will request
them anyway.").

> +      }
> +
> +      /* 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.

[snip]

>
>     if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) {
> -      if (get_back_bo(dri2_surf) < 0)
> +      if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
> +         if (get_back_bo(dri2_surf) < 0)
> +            return 0;
> +      }
> +
> +      if (!dri2_surf->dri_image_back) {
> +         _eglLog(_EGL_WARNING,
> +               "%s:%d dri2_image back buffer allocation failed !\n",
> +               __func__, __LINE__);

The error message here is inconsistent. The case of front buffer
requested for window surface clearly says that it's illegal, but this
one pretends that there was an actual allocation attempt.

Also (my subjective point of view) the whole code could be much more
readable if all the allocation of front buffer could be moved to
get_front_bo() function, consistently with back buffer handling.

>           return 0;
> +      }
>
> -      images->back = dri2_surf->dri_image;
> +      images->back = dri2_surf->dri_image_back;
>        images->image_mask |= __DRI_IMAGE_BUFFER_BACK;
>     }
>
> @@ -775,14 +837,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
>     for (i = 0; dri2_dpy->driver_configs[i]; i++) {
>        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[i],
> -         __DRI_ATTRIB_DOUBLE_BUFFER, &double_buffered);
> -
> -      /* support only double buffered configs */
> -      if (!double_buffered)
> -         continue;

Does it really just work like this? Last time I checked the list of
configs generated after such change it contained single-buffered-only
configs with window surface bit set and double-buffered-only configs
with pbuffer bit set, both of which are invalid. Moreover, this caused
some dEQP tests to fail. How was this patch tested?

Best regards,
Tomasz