Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Stop importing DMA-BUFs on wl_surface.commit #2664

Closed
emersion opened this issue Jan 19, 2021 · 7 comments
Closed

Stop importing DMA-BUFs on wl_surface.commit #2664

emersion opened this issue Jan 19, 2021 · 7 comments

Comments

@emersion
Copy link
Member

emersion commented Jan 19, 2021

Right now, we create wlr_client_buffer objects on surface commit. This works fine for wl_shm buffers (especially when we can just update the previous buffer), however this imports DMA-BUFs via EGL too often. We'd only need to import them once when the wl_buffer is created, and then we could re-use the same EGLImage for all subsequent commits.

Some drivers are known to have slow DMA-BUF import paths. This also makes us import too often the DMA-BUFs to KMS when using direct scan-out.

@emersion
Copy link
Member Author

Well, apparently re-importing the DMA-BUFs is necessary on some drivers to see the changes to the underlying buffer.

@emersion
Copy link
Member Author

However, when we use external EGL textures, the underlying changes are guaranteed to be visible without re-importing.

Also, it seems like to see the external changes for a regular texture (not external), we don't need to re-import the DMA-BUF as an EGLImage -- we can just re-create the texture from the same EGLImage.

@J0nnyMak0
Copy link
Contributor

This proposal sounds like it can potentially fix #2636?

@emersion
Copy link
Member Author

emersion commented Feb 1, 2021

I don't think so. Before the regression we were already importing DMA-BUFs on each commit.

@emersion
Copy link
Member Author

emersion commented Mar 28, 2021

We could add a wlr_texture_invalidate function. However, it's important to note that shm buffers work pretty differently from DMA-BUFs.

With the GLES2 renderer and a DMA-BUF texture:

  • On wl_buffer creation: import as an EGLImage
  • On wl_surface.commit: re-create the texture if it's not EXTERNAL_OES

With the GLES2 renderer and a shm texture:

  • On wl_buffer creation: do nothing
  • On wl_surface.commit: upload the damaged region to the texture

With the Pixman renderer and a shm texture:

  • On wl_buffer creation: we can't hook into that, it's all handled inside libwayland
  • On wl_surface.commit: create the pixman image
  • When accessing the pixman image, make sure to call wl_shm_buffer_{begin,end}_access

emersion added a commit to emersion/wlroots that referenced this issue Apr 12, 2021
Previously, the same struct was used for linux-dmabuf-v1 params
and buffer. This made the whole logic a little bit awkward, because
a wlr_dmabuf_v1_buffer could either be still being constructed, or
be a complete buffer.

Introduce a separate wlr_linux_buffer_params_v1 struct for buffer
params still being constructed. Once the params are complete (ie.
once the create request is sent), the params struct is destroyed
and the buffer struct is created.

This will help with [1] as well.

[1]: swaywm#2664
@emersion
Copy link
Member Author

#2851 tries to address this issue by passing a wlr_buffer reference to the renderer for upload/import. The renderer can then make the decision to leave the buffer unlocked (upload: shm+GLES2) or lock the buffer (import: DMA-BUF+GLES2 or shm+Pixman).

This does require us to always have a wlr_buffer around for each wl_buffer. This is easy enough to do for linux-dmabuf buffers, but is a little bit more tedious with wl_shm buffers (because it's all implemented by libwayland, and wlr_buffer_impl.get_data_ptr does not play well with wl_shm_buffer_begin_access/end_access)

emersion added a commit that referenced this issue Apr 19, 2021
Previously, the same struct was used for linux-dmabuf-v1 params
and buffer. This made the whole logic a little bit awkward, because
a wlr_dmabuf_v1_buffer could either be still being constructed, or
be a complete buffer.

Introduce a separate wlr_linux_buffer_params_v1 struct for buffer
params still being constructed. Once the params are complete (ie.
once the create request is sent), the params struct is destroyed
and the buffer struct is created.

This will help with [1] as well.

[1]: #2664
@emersion
Copy link
Member Author

Fixed in #2851

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants