On Thu, Nov 24, 2016 at 12:00 PM, Tomasz Figa <tf...@chromium.org> wrote:
> Hi Zhifang,
>
> On Thu, Nov 24, 2016 at 11:39 AM, Long, Zhifang <zhifang.l...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
