[Mesa-dev] EGL/android: pbuffer implementation.
Liu, Zhiquan
zhiquan.liu at intel.com
Tue Nov 22 06:15:38 UTC 2016
Hi Tomasz,
Thanks for you commends.
> >
>
> 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]
I just know the script, I will run next time.
>
> > @@ -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.
IMHO,droid_window_cancel_buffer should not call droid_window_enqueue_buffer() It should call dri2_surf->window->cancelBuffer, this should change in another patch.
So we add destroyImage here.
>
> > +
> > + 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?
>
Here, I just follow the image_mask operation.
> >
> > 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.").
https://patchwork.freedesktop.org/patch/120682/ do you thinks this patch is good?
it deal pbuffer in front and back buffer.
>
> > + }
> > +
> > + /* 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.
I will send a follow patch fixing this.
>
> 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.
I made a version of get_front_bo, this will have duplicate code.
https://patchwork.freedesktop.org/patch/120682/
>
> > 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?
>
I add log to print dpy->Configs->Elements[i]
add at end of droid_add_configs_for_visuals in platform_android.
for (i = 0; i < dpy->Configs->Size; i++) {
struct dri2_egl_config *dri2_conf = dri2_egl_config(dpy->Configs->Elements[i]);
_eglLog(_EGL_DEBUG, "check dpy->Configs->Elements[%d]",i);
for (srgb = 0; srgb < 2; srgb++) {
if (!dri2_conf->dri_single_config[srgb] && !dri2_conf->dri_double_config[srgb])
continue;
if (dri2_conf->dri_single_config[srgb])
_eglLog(_EGL_DEBUG, "dri2_conf->dri_single_config[%d] SurfaceType is 0x%x", srgb,dri2_conf->base.SurfaceType);
if (dri2_conf->dri_double_config[srgb])// should not add else here.
_eglLog(_EGL_DEBUG, "dri2_conf->dri_double_config[%d] SurfaceType is 0x%x", srgb,dri2_conf->base.SurfaceType);
}
}
from Elements[0] to Elements[10], surfacetype is 0x5 EGL_WINDOW_BIT | EGL_PBUFFER_BIT, both single and double have config.
from Elements[11] to Elements[30], surfacetype is 0x4 EGL_WINDOW_BIT, only double have config.
11-22 02:46:56.711 1369 1587 D EGL-DRI2: check dpy->Configs->Elements[9]
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_single_config[0] SurfaceType is 0x5
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_double_config[0] SurfaceType is 0x5
11-22 02:46:56.711 1369 1587 D EGL-DRI2: check dpy->Configs->Elements[10]
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_single_config[0] SurfaceType is 0x5
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_double_config[0] SurfaceType is 0x5
11-22 02:46:56.711 1369 1587 D EGL-DRI2: check dpy->Configs->Elements[11]
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_double_config[0] SurfaceType is 0x4
11-22 02:46:56.711 1369 1587 D EGL-DRI2: check dpy->Configs->Elements[12]
11-22 02:46:56.711 1369 1587 D EGL-DRI2: dri2_conf->dri_double_config[0] SurfaceType is 0x4
About Deqp test, it passed all pbuffer cases on my devices.
Best regards,
Zhiquanl
More information about the mesa-dev
mailing list