[Mesa-dev] EGL/android: pbuffer implementation.
Tomasz Figa
tfiga at chromium.org
Tue Nov 22 06:42:39 UTC 2016
On Tue, Nov 22, 2016 at 3:15 PM, Liu, Zhiquan <zhiquan.liu at intel.com> wrote:
> 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.
Yeah, if we change it as you mention it will make perfect sense.
>>
>> > +
>> > + 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.
Yeah, but my understanding is that images->image_mask tells the caller
which members of images has been updated. Anyway, since it seems to be
working fine and we don't have the extension semantics documented
precisely, we can just keep it as is.
>> >
>> > 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 patch from your link will also error out if front buffer is
requested for a window surface. The code before your patch would just
silently ignore such request without setting respective bit in
images->image_mask, but other requested buffers would still be
collected. That patch is also incorrect because it allocates a back
buffer for pbuffer surfaces, but those (in Mesa) are supposed to have
only front 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.
Okay, thanks.
>>
>> 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/
I'm not sure where the code duplication was there, especially if we
remove the back buffer allocation in case of pbuffer. Then
get_back_bo() and get_front_bo() would have almost completely
different code.
>>
>> > 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.
Looks like something else changed in Mesa then. Possibly "4d6d55deef
egl: stop claiming support for pbuffer + msaa", but then it only masks
the problem, because luckily all double-only-buffered configs on i915
are also multisampled.
Best regards,
Tomasz
More information about the mesa-dev
mailing list