[Mesa-dev] EGL/android: pbuffer implementation.

Tomasz Figa tfiga at chromium.org
Thu Nov 24 03:00:09 UTC 2016


Hi Zhifang,

On Thu, Nov 24, 2016 at 11:39 AM, Long, Zhifang <zhifang.long at intel.com> wrote:
>> >> > @@ -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.
>
> We will submit a new patch to correct droid_window_cancel_buffer().
> But since cancel buffer is only for window surface,  while the dri_image_front/back
> are not only used for window surface,   no matter droid_window_cancel_buffer()
> will destroy those images or not,  we still need to check and clean those data members
> in common destroy surface path.

dri_image_back is supposed to be used for window surface only.

Despite this, I agree that having both images handled in the same
place is cleaner, so let's just fix droid_window_cancel_buffer().

>> >> >
>> >> >     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.
>>
>
> Our original v1 patch is https://patchwork.freedesktop.org/patch/120682/, with
> some review suggestions, it changed to current landed v3 version in
>  https://patchwork.freedesktop.org/patch/121634/ .
>
> Let's explain the original consideration :
> 1) From EGL spec point of view,  window surface is double buffer and only "back buffer"
> can be used.  So we add an error log when trying allocating front buffer for window surface.

There are 2 specs involved here - EGL spec and DRI image loader spec.
EGL spec only says that the client app can only operate on the back
buffer. It doesn't imply any implementation details. Now DRI image
loader spec (which unfortunately doesn't seem to be written down; I'm
just judging by existing driver code) seems to allow drivers to set
more bits in buffer_mask just to probe which buffers are available. So
even if the driver sets back buffer bit, if the buffer is not
available, it might still expect success from the getBuffers callback,
just without the unsupported buffer bit set in images->image_mask.

> 2) Some special driver may extend the capability beyond EGL spec scope to support
> front buffer for window surface (like GLX or WGL).  But our change is only in platform_android.c,
> and current platform_android.c does not include such implementation for window surface
> front buffer rendering,  so adding error message here won't harm anything.  If someone
> added front buffer rendering in his own code, he can simply remove this error message.

My comment is not about some special extensions, but about the
"probing mechanism" I mentioned above, which seems to be used by
drivers, including Gallium.

>
> From EGL spec point of view,  pbuffer is single buffer and only "back buffer" is used.

Again, as far as I know, EGL governs only the behavior visible to the
client app. It doesn't control implementation details. For consistency
with GLX and company, Mesa seems to have adopted FRONT buffer
convention for all single buffered surfaces, but this is only an
implementation detail, invisible to client apps.

> Just like the discussion Emil mentioned in  https://patchwork.freedesktop.org/patch/85913/,
> current  problem is i965 dri driver will pass down FRONT or BACK  buffer bit mask according the
> config is  single or double,  so it passed down FRONT buffer mask for pbuffer.  But this behavior
> somehow is not correct according to the EGL spec.

This isn't really incorrect, since it's not visible to client apps.
It's just maybe a bit confusing for Mesa developers, but assuming that
it's documented properly and behavior observed by client apps is
correct, there should be no problem.

> We don't what to introduce more re-arch work
> in this patch, so we added two branches in v1 version, no matter caller passed down FRONT or BACK
> mask for pbuffer,  we always can handle it.  Then if the behavior in i965 driver is corrected by other
> patch in future, this code for pbuffer still can work.
>
> The current " if (!dri2_surf->dri_image_back)"  check is not correct for pbuffer + back buffer mask path,
> but  such case will not really happen now. Do you want we to change it back to the original v1 version logic ?

Nope, the correct way AFAICT is to keep the behavior from before this
patch, no matter if it's v1 or the merged v3. More specifically, the
getBuffers() callback shouldn't fail if an unsupported buffer is
requested - it just shouldn't set images->image_mask bit of that
buffer and continue collecting remaining requested buffers.

Best regards,
Tomasz


More information about the mesa-dev mailing list