Augment wlr_buffer to support swapchains #2044
Conversation
With [1], wlr_buffer has the required infrastructure to handle swapchains. [1]: swaywm/wlroots#2044
emersion
changed the title
|
This is now ready for review. |
With [1], wlr_buffer has the required infrastructure to handle swapchains. [1]: swaywm/wlroots#2044
With [1], wlr_buffer has the required infrastructure to handle swapchains. [1]: swaywm/wlroots#2044
Update for [1]. [1]: swaywm/wlroots#2044
| */ | ||
| struct wlr_buffer *wlr_buffer_ref(struct wlr_buffer *buffer); | ||
| void wlr_buffer_drop(struct wlr_buffer *buffer); |
There was a problem hiding this comment.
Conflicts with the concept of "releasing" a buffer when all consumers are done with it (wording used in the Wayland protocol: wl_buffer.release).
There was a problem hiding this comment.
Eh. Don't have any better ideas.
There was a problem hiding this comment.
But maybe we should s/lock/acquire/ and s/unlock/release/?
There was a problem hiding this comment.
Also, chose to use "drop" instead of keeping "unref" so that old wlr_buffer_unref users don't forget to update their code to use wlr_buffer_unlock.
Consumers call wlr_buffer_lock. Once all consumers are done with the buffer, only the producer should have a reference to the buffer. In this case, we can release the buffer (and let the producer re-use it).
|
I don't really want to turn this into bikeshedding the function names, but would something a bit more explicit about "reading" be clearer?
|
|
Actually, locking is necessary for write access too. In the case of a swapchain:
|
|
Yeah, that makes sense. |
| */ | ||
| struct wlr_buffer *wlr_buffer_ref(struct wlr_buffer *buffer); | ||
| void wlr_buffer_drop(struct wlr_buffer *buffer); |
There was a problem hiding this comment.
Is this function really necessary? There are only 2 potential users of this function, which is wlr_client_buffer, and a swapchain.
wlr_client_buffer doesn't need it, because it can just destroy the buffer on the release signal.
The swapchain can just implement this itself, taking the release event into account.
There was a problem hiding this comment.
Do you mean that we can replace wlr_buffer_drop with wlr_buffer_destroy?
This is necessary when e.g. resizing a swapchain: the old buffers (with the old size) can be dropped by the swapchain while still being used by e.g. DRM for scanout.
There was a problem hiding this comment.
And in wlr_surface, we'd need to have a list of buffers the compositor is still using and a bunch of release listeners to only destroy buffers when we can.
There was a problem hiding this comment.
This is necessary when e.g. resizing a swapchain: the old buffers (with the old size) can be dropped by the swapchain while still being used by e.g. DRM for scanout.
My point is that you'd still keep roughly the same logic, but it would be implement in the swapchain rather than in wlr_buffer.
And in wlr_surface, we'd need to have a list of buffers the compositor is still using and a bunch of release listeners to only destroy buffers when we can.
Isn't that just done by the reference counting?
There was a problem hiding this comment.
My point is that you'd still keep roughly the same logic, but it would be implement in the swapchain rather than in wlr_buffer.
If the swapchain has 3 slots and is resized, there are 3 "dangling buffers" still used by DRM that will need to be destroyed when released.
Isn't that just done by the reference counting?
What do you suggest actually? I realize I may have misunderstood.
Do you want to remove wlr_buffer_drop and replace it with a wlr_buffer_destroy function which just immediately destroys the buffer? If that's the case, someone needs to keep track of the release event and destroy the buffer when it happens. I don't want to duplicate this logic in wlr_surface and in the swapchain.
|
@ascent12 Ping? Any better naming ideas? (e.g. lock → acquire / unlock → release?) |
|
Bump |
|
So… What do we want to do with this pull request? Are there any actionable tasks to unblock it? |
With [1], wlr_buffer has the required infrastructure to handle swapchains. [1]: swaywm/wlroots#2044
With [1], wlr_buffer has the required infrastructure to handle swapchains. [1]: swaywm/wlroots#2044
Update for [1]. [1]: swaywm/wlroots#2044
Depends on #2043This PR adds the required infrastructure to
wlr_bufferto support swapchains. glider has been updated to use it: emersion/glider#7.Once this PR is merged, it will be possible to add DRM support to #1985 via libliftoff.
wlr_buffernow has a concept of "released", "producer" and "consumer". A buffer is created by a producer (the allocator or the client). It's then referenced by consumers (e.g. the backend or the compositor). Once all consumers are done with the buffer (ie. once they have un-referenced the buffer), the buffer is released and can be re-used (by the swapchain or the client).This is inspired from glider's buffer: https://github.com/emersion/glider/blob/c66847dd1cf8ae5e68666ce7cb3be41427b38dc7/include/allocator.h#L17.
One difference is that this PR is less explicit: both consumers and producers callRenamed.wlr_buffer_unref. On glider, there's one API for producers (glider_buffer_init,glider_buffer_unref) and a separate API for consumers (glider_buffer_lock,glider_buffer_unlock). This makes it more obvious when the buffer is released. Is it worth it to make this breaking change?glider's buffer is itself inspired from the renderer v6's
wlr_imagetype: https://github.com/swaywm/wlroots/pull/1355/files#diff-6105fb2a3a3620a8a1aabaa7b11beb84R16Breaking changes:
wlr_bufferto read it later should callwlr_buffer_lockandwlr_buffer_unlockinstead ofwlr_buffer_refandwlr_buffer_unref. Same applies to backends.wlr_client_buffer_createhas been renamed towlr_client_buffer_import. Callers need towlr_buffer_unlockthe resulting buffer once they're done with it.wlr_buffer_dropinstead ofwlr_buffer_unref.Sway PR: swaywm/sway#5088