[Mesa-dev] EGL/android: pbuffer implementation.
Tomasz Figa
tfiga at chromium.org
Thu Nov 24 03:04:59 UTC 2016
On Thu, Nov 24, 2016 at 12:00 PM, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.
FYI, my patch I sent some time ago should have the correct behavior in
this aspect (although it was breaking virgl driver for some reason):
https://patchwork.freedesktop.org/patch/102544/
Best regards,
Tomasz
More information about the mesa-dev
mailing list