[Mesa-dev] EGL/android: pbuffer implementation.
Tapani Pälli
tapani.palli at intel.com
Tue Nov 22 05:44:27 UTC 2016
On 11/21/2016 04:15 PM, Emil Velikov wrote:
> 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.
It did go through CI but it does not run any Android/ARC tests. AFAIK
Zhiquan did run pbuffer related tests and reported them passing on
Android. Which dEQP tests were regressed?
> Randy, others, please take a careful look at the issues pointed out by Tomasz.
>
> Emil
> _______________________________________________
> 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