On 11/21/2016 04:15 PM, Emil Velikov wrote:
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.
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
