[PATCH weston 1/3] clients: Drop deadlock circumvention hack now that we don't need it

[Jonas Ådahl]

On Fri, Aug 21, 2015 at 04:09:20PM +0300, Pekka Paalanen wrote:
> On Wed, 19 Aug 2015 11:15:21 -0500
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
> > On 26/01/15 04:19 AM, Jonas Ådahl wrote:
> > > mesa supports EGLSwapInterval 0 now, so lets remove this hack. As a
> > > bonus we don't conflict with the XDG shell protocol that doesn't allow
> > > committing a null-buffer, which was a side effect of this hack.
> 
> Hmm, but the code removed here is *not* committing a null buffer.
> Instead, it is doing a commit without an attach. Surely that is legal
> even with an xdg_surface?

There is a check in mutter which terminates a client if a surface is
committed without a buffer. If a client has never attached a buffer to a
surface, this could happen, and is what happened here. Though now that I
think about it should be allowed, since one might want to negotiate
initial state before attaching an initial buffer, and one needs to commit
the state before that. *adds item to todo list*.

What probably shouldn't be allowed is to attach a NULL buffer and then
commit it. That is what the intention with the check in mutter is I
assume. So I guess the commit message part about the bonus there is
incorrect.

> 
> Btw. did we have any patches to xdg-shell spec to say that you cannot
> attach and commit a null buffer? ISTR it has been discussed, yes, but I
> couldn't see it in the spec on a quick look. Some patch in the queue?

It is undefined behavior. mutter defines it as illegal and weston
doesn't. Yes, it should be specified IMO. I don't know about such a
patch.

> 
> > Unreviewed for far too long. :(
> > 
> > The question I guess is whether toytoolkit needs to maintain this hack
> > forever because mesa used to have a problem with this.  As far as I can
> > tell from reading the EGL spec, eglSwapInterval(0) has been defined for
> > as long as eglSwapInterval() has existed (EGL 1.1)...
> > 
> > You've stated your preference - and I'm convinced.  I don't think
> > toytoolkit apps should be a reference for avoiding old mesa bugs.  I'm
> > also not hugely concerned about other broken EGL stacks... (if you're
> > making a custom IVI with a nasty driver stack, you probably shouldn't be
> > using toytoolkit at all, and if you are you can re-invent this nastiness.)
> > 
> > Besides, the comments even say we should kill it now.
> > 
> > All that said,
> > Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> > 
> > Can we kill this hack? :)
> 
> Yeah, I think we should. Let's see if anyone complains afterwards.
> 
> Btw. I added a reference to
> e9297f8e7ee09fa39b1d4293fad6e97705ccff21 which was the patch adding
> this hack, it contains an elaborate explanation.
> 
> > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > ---
> > >  clients/window.c | 32 --------------------------------
> > >  1 file changed, 32 deletions(-)
> 
> I was very close to pushing this, when I realized that only subsurfaces
> client actually sets eglSwapInterval(0). Testing nested.c is also a bit
> difficult because it requires cairo-glesv2, and it seems my distro has
> dropped the possibility of that option from Cairo.
> 
> Looking at nested.c closer, I suspect it might be ok as is. The
> sub-surface renderer does not call eglSwapbuffers on sub-surfaces
> (right?).

As far as I can see, we just get a wl_buffer from EGL, then attach and
commit it. No going through EGL for the wl_surface interaction. And we
don't block on the frame callback either for that matter.


Jonas

> 
> Pushed:
>    1a912a9..decc965  master -> master
> 
> 
> Thanks,
> pq