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

[Emil Velikov]

On 21 November 2016 at 07:23, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.)
>
I'll add you and update the documentation to reference the script.

Rob H let me know if you'd like to be in there as well which files
(platform_egl.c, Android build and/or other) and which email you'd
prefer.

> [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.").
>
Fwiw -

>> +      }
>> +
>> +      /* 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?
>
Afaict the patch has (should have?) gone through the Intel CI,
although I'm not sure if the latter builds/runs Android/ARC.

Randy, others, please take a careful look at the issues pointed out by Tomasz.

Emil