On 21 November 2016 at 07:23, Tomasz Figa <tf...@chromium.org> wrote:
> Hi,
>
> On Wed, Nov 16, 2016 at 11:11 AM, Liu Zhiquan <zhiquan....@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....@intel.com>
>> Signed-off-by: Kalyan Kondapally <kalyan.kondapa...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
