On Wed, 2018-08-08 at 17:21 +0100, Daniel Stone wrote:
> Hi Juan,
>
> On Thu, 2 Aug 2018 at 10:02, Juan A. Suarez Romero <jasua...@igalia.com>
> wrote:
> > If color buffer is locked, do not set its wayland buffer to NULL;
> > otherwise it can not be freed later.
>
> It can: see the 'if (i == ARRAY_SIZE(...))' branch inside wl_buffer_release.
I think I didn't explain wrongly :)
If color buffer is locked, we set color_buffer.wl_buffer to NULL, and thus we
can't free wl_buffer later.
>
> > This also fixes dEQP-EGL.functional.swap_buffers_with_damage.* tests.
>
> How does it fix them? Is it just leaks, or is there a functional difference?
Besides this wl_buffer leak, the tests crashes. Found out that fixing the leak
the crash goes away.
>
> > --- a/src/egl/drivers/dri2/platform_wayland.c
> > +++ b/src/egl/drivers/dri2/platform_wayland.c
> > @@ -412,8 +412,11 @@ dri2_wl_release_buffers(struct dri2_egl_surface
> > *dri2_surf)
> >
> > for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> > if (dri2_surf->color_buffers[i].wl_buffer &&
> > - !dri2_surf->color_buffers[i].locked)
> > + !dri2_surf->color_buffers[i].locked) {
> > wl_buffer_destroy(dri2_surf->color_buffers[i].wl_buffer);
> > + dri2_surf->color_buffers[i].wl_buffer = NULL;
> > + }
> > +
> > if (dri2_surf->color_buffers[i].dri_image)
> >
> > dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
> > if (dri2_surf->color_buffers[i].linear_copy)
> > @@ -422,11 +425,9 @@ dri2_wl_release_buffers(struct dri2_egl_surface
> > *dri2_surf)
> > munmap(dri2_surf->color_buffers[i].data,
> > dri2_surf->color_buffers[i].data_size);
> >
> > - dri2_surf->color_buffers[i].wl_buffer = NULL;
> > dri2_surf->color_buffers[i].dri_image = NULL;
> > dri2_surf->color_buffers[i].linear_copy = NULL;
> > dri2_surf->color_buffers[i].data = NULL;
> > - dri2_surf->color_buffers[i].locked = false;
>
> So in this case, we still occupy a slot with (wl_buffer != NULL &&
> locked == true) when we resize a surface when an old buffer is still
> locked.
>
> I _think_ the main functional difference is that the buffer is now
> destroyed inside dri2_wl_destroy_surface() when the surface is
> destroyed, even if it is locked. Destroying a surface with a locked
> buffer is bad: calling wl_buffer_destroy() may provoke a fatal
> protocol error, as destroying whilst still in use is illegal. On the
> other hand, destroying the event queue without destroying the object
> may cause a use-after-free later when the event does arrive, if the
> connection is still alive (we should make that an assert really).
>
> On the balance of things, I think destroying the buffer immediately
> (running the risk of a protocol error), is less bad. It would be even
> better if we could rely on clients to have destroyed their
> surface-wrapper object (causing all buffers to be released) by the
> time they destroy their EGLSurface, but that's of course never going
> to happen. So, assuming there's no unexpected consequences I haven't
> seen, this is:
> Reviewed-by: Daniel Stone <dani...@collabora.com>
>
I understand this patch does not require any further work, right? Except
clarifying the commit messsage as explaining above.
> Cheers,
> Daniel
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
