[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

GitLab Migration User  changed:

   What|Removed |Added

 Resolution|--- |MOVED
 Status|REOPENED|RESOLVED

--- Comment #18 from GitLab Migration User  ---
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been
closed from further activity.

You can subscribe and participate further through the new bug through this link
to our GitLab instance:
https://gitlab.freedesktop.org/wayland/wayland/issues/46.

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #17 from Daniel Stone  ---
(In reply to Pekka Paalanen from comment #15)
> I think the proposal is to make it undefined.

That's my suggestion.

(In reply to Pekka Paalanen from comment #16)
> Ahumm... wl_cursor_image_get_buffer() always returns the same wl_buffer. Not
> sure there is a way in the API to get separate wl_buffers, and multiple
> wl_surfaces for the same image is very possible by having multiple wl_seats.

Ouch. We should definitely fix that.

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #16 from Pekka Paalanen  ---
Ahumm... wl_cursor_image_get_buffer() always returns the same wl_buffer. Not
sure there is a way in the API to get separate wl_buffers, and multiple
wl_surfaces for the same image is very possible by having multiple wl_seats.

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #15 from Pekka Paalanen  ---
I think the proposal is to make it undefined.

You mean one release per attach+commit in mutter, right?

So Weston and Mutter implement the opposite behaviours. This means that if a
client ever depended on either behaviour, it would malfunction on the other.

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #14 from Jonas Ådahl  ---
FWIW, mutter has implemented the one-release-per-attach semantics, which I've
read as the correct way. Is it now meant to be illegal to have a single buffer
attached at multiple surfaces?

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #13 from Pekka Paalanen  ---
That's actually a good point that even wl_shm clients can simply create
multiple wl_buffers for the same memory.

Yeah, I'm fine with just documenting the current limitation and not solving the
issue by complicating protocol.

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #12 from Daniel Stone  ---
At this point, I think it would be more simple to document it as a limitation
that attaching the same wl_buffer to two wl_surfaces has inherent race
conditions and is thus a bad idea, and move on with our lives.

EGL clients can't ever access the buffer. dmabuf clients can just create a new
buffer referring to the same storage and do refcounting client side; ditto SHM
clients. So do we have a good usecase for multi-attach with a hard limit of one
buffer object?

-- 
You are receiving this mail because:
You are the assignee for the bug.___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

Jonas Ådahl  changed:

   What|Removed |Added

 CC||jad...@gmail.com

--- Comment #6 from Jonas Ådahl  ---
Why can't we just let wl_buffer.release simply be a notification about the
buffer being reusable, meaning,

After a client commits a surface with a wl_buffer@1 attached, it should be
consider busy until it receives the release event. If the client attaches the
same wl_buffer@1 to multiple surfaces, its still up to the server to let the
client know when the buffer is no longer used, and may be reused by the client.

I.e.

1. surface1.attach(buffer1)
2. surface1.commit  (client should consider buffer1 busy until further notice)
3. surface2.attach(buffer1)
4. surface2.commit  (this deosn't change the buffer1 busy state)

..would result in a single buffer1.release() event being emitted whenever the
server doesn't use buffer1 for neither surfaces. This is how I'd interpret the
current wording in the specification as well; being more or less orthogonal to
commits and frame callbacks.

Or is there something I'm missing? Why would it be important to get release
events per commits?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #10 from Jonas Ådahl  ---
(In reply to Pekka Paalanen from comment #9)
> (In reply to Jonas Ådahl from comment #8)
> > Ok, yes I see it now.(In reply to Pekka Paalanen from comment #7)
> > (In reply to Pekka Paalanen from comment #5)
> > > Client side reference counting, that is, for every committed attach there
> > > will be a release, seems like the preferred solution to me at the moment.
> > 
> > How do you expect the client to get the new semantics (one release per
> > commit) or do you mean to change it for everyone?
> 
> For everyone, yes.
> 
> I would hope that attaching the same wl_buffer to multiple surfaces is rare
> enough that we can get away with it. Committing the same wl_buffer to a
> surface before waiting for a release or a frame callback is I hope equally
> rare. (If you wait for frame callback, you're probably going to draw, so
> using a busy buffer is a mistake to begin with. If you wait for a release,
> there is no race.)
> 
> Since you cannot get the wl_buffer out of EGL, EGL should never hit a case
> where the client-side reference count would be more than one.
> 
> Also judging by krh's comment, it was probably expected to work in refcount
> manner to begin with, we (probably just me) just screwed up the spec and
> Weston.
> 
> Bumping the wl_surface version for this is IMO a backup plan, in case we
> can't just change the semantics for everyone.
> 
> For now, this is a quite theoretical race, which is why I think we can solve
> this now. With Presentation queueing that would change, so this must be
> solved before queueing can get further.

Reading between the lines in the description, one could assume only one surface
was considered, since it only talks about that scenario, so I agree it should
be a reasonable change to do.

The only issue would be that it's hard for the client (especially EGL) to know
when it can rely on the multi-surface case to work properly, especially if it's
not possible to get the version of the bound (wl_surface) proxy (or the newest
version supported by the server). I experimented once with implementing a
"wl_version" interface to solve a similar issue (request version information
from the server given an existing object) without needing to change any client
API, but feels a bit hacky to need to do such a roundtrip.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #11 from Pekka Paalanen  ---
(In reply to Jonas Ådahl from comment #10)
> The only issue would be that it's hard for the client (especially EGL) to
> know when it can rely on the multi-surface case to work properly, especially
> if it's not possible to get the version of the bound (wl_surface) proxy (or
> the newest version supported by the server).

EGL should never hit this. App cannot get the wl_buffer used by EGL, and EGL
won't push the same wl_buffer to multiple surfaces or without waiting for
release.

How to know that the refcounting actually works, that's an important question.
For EGL it should be irrelevant, but other libraries might perhaps care.
Looking at the wl_proxy_get_version() issue would be well worth it. FWIW, I
don't think we have any case where inheritance wouldn't work. For things like
wl_callback that can never be bumped beyond version 1 it doesn't really matter
if get_version() returns 5. It's still larger than 1.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #4 from Bill Spitzak  ---
Could the buffer release include some kind of serial number indicating which
attach was the last one done to the buffer?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
wayland-bugs mailing list
wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

Pekka Paalanen ppaala...@gmail.com changed:

   What|Removed |Added

 Blocks||83092

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
Wayland-bugs mailing list
Wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

Kristian Høgsberg k...@bitplanet.net changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |NOTABUG

--- Comment #2 from Kristian Høgsberg k...@bitplanet.net ---
(In reply to comment #0)
 Consider a client doing this:
 1. surface1.attach(buffer1)
 2. surface1.commit
 3. surface2.attach(buffer1)
 4. surface2.commit
 
 Then the client receives buffer1.release.
 
 It is ambiguous, whether the release corresponds to step 2 or step 4. That
 is, the client cannot know if the latter commit still holds the buffer
 reserved in the server. The server may have had time to process and release
 the buffer before step 4, in which case the buffer would be reserved again
 after step 4.
 
 The problem is the same also, if surface1 and surface2 were both just
 surface1. If the compositor has time to repaint in between the commits, the
 client may get a release it most likely interprets as the release
 corresponding to step 4, which would be wrong.
 
 How should this be resolved?
 
 A suggestion from Jason Ekstrand was to modify the protocol to guarantee one
 release event for each commit that makes the buffer reserved. This would
 allow clients to use simple reference counting.

We do that now.  Every attach+commit is followed by a release.  If a client
attaches a buffer to two surfaces, it needs to receive two release events
before it can use the buffer again without causing artifacts.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
Wayland-bugs mailing list
Wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs

[Wayland-bugs] [Bug 75303] Protocol: wl_buffer.release is racy

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=75303

--- Comment #1 from Jason Ekstrand ja...@jlekstrand.net ---
Pekka already noted my suggestion of reference counting.  However, this raises
the issue of compatibility between clients and servers.  In particular, if
clients start to implement buffer reference counting, then we need some sort of
a guarantee by the server that it will aid in the reference counting. 
Otherwise, in the scenario Pekka describes, the client will wait for two
releases and may only get one
It's worth noting that the usual case of waiting for wl_buffer.release before
attaching again is the same regardless of reference counting because it means
the server has at most one reference.  The issue comes in the other cases where
the server may have more references.

One option to solve this would be to add a comment to wl_surface.attach and
bump the wl_surface version.  Technically, one would like this to be part of
wl_buffer.  However, wl_buffer is typically implemented by either libwayland's
SHM implementation or the system EGL implementation and those never touch
wl_buffer.release except for EGL receiving client-side.  There is a slight
issue with EGL not knowing the wl_surface version, however EGL usually has one
buffer pool per surface and commits one buffer at a time.  I think we'll
probably be ok here.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
Wayland-bugs mailing list
Wayland-bugs@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-bugs