wayland: add support for linux-dmabuf #790
Conversation
|
Ubuntu 20.04 has a very old libdrm. Should I make this new backend optional, or should I bump the Ubuntu version, or something else? |
|
Gentle ping, any chance to get this reviewed? |
emersion
force-pushed
the
linux-dmabuf
branch
from
922ce7d
to
f670038
Compare
|
Gentle ping, would you have the time to have another look at this? |
| struct zwp_linux_dmabuf_v1 *linux_dmabuf; | ||
| struct zwp_linux_dmabuf_feedback_v1 *feedback; | ||
|
|
||
| if (strcmp(interface, zwp_linux_dmabuf_v1_interface.name) == 0 && |
There was a problem hiding this comment.
TBH, I am not familiar with this interface, but from code logic,
looks , it could be combined together with va_wayland_drm? add interface check inside registry_handle_global
if it is linuxdmabuf ,xxxx ; if it is wl_drm xxxx?
from libva perspective: it should retrieve the device fd though these function, then get the driver name though device fd. looks both wl_drm and linuxdma buffer could do it. but wl_drm interface will be deprecated?
from driver implementation perspective. what's the difference to implement vaGetSurfaceBufferWl and vaGetImageBufferWl? previously, we use "wl_proxy_create" and "wl_proxy_marshal" interface to share the dma buffer fd from vaSurface to wl_buffer, just like https://github.com/intel/intel-vaapi-driver/blob/master/src/i965_output_wayland.c#L231-L263. looks we need new implementation now?
There was a problem hiding this comment.
looks , it could be combined together with va_wayland_drm? add interface check inside registry_handle_global
if it is linuxdmabuf ,xxxx ; if it is wl_drm xxxx?
That's possible indeed, but not sure it's a good idea. It would entangle the two implementations and make it more complicated to understand the code and drop the wl_drm part in the future.
from libva perspective: it should retrieve the device fd though these function, then get the driver name though device fd. looks both wl_drm and linuxdma buffer could do it. but wl_drm interface will be deprecated?
Yes, wl_drm was never supposed to be used by clients such as libva, it was supposed to be an internal Mesa protocol.
Some Wayland compositors like Sway have already dropped support for wl_drm.
from driver implementation perspective. what's the difference to implement vaGetSurfaceBufferWl and vaGetImageBufferWl? previously, we use "wl_proxy_create" and "wl_proxy_marshal" interface to share the dma buffer fd from vaSurface to wl_buffer, just like https://github.com/intel/intel-vaapi-driver/blob/master/src/i965_output_wayland.c#L231-L263. looks we need new implementation now?
Hm, yeah. This would need a new implementation indeed.
mpv seems to work just fine without this new implementation though. Maybe because they grab the DMA-BUFs directly, instead of vaGetImageBufferWl/vaGetSurfaceBufferWl?
There is nothing driver-specific in vaGetSurfaceBufferWl/vaGetImageBufferWl by the way. The implementation should be identical for all VA-API drivers. Turning a DMA-BUF into a wl_buffer can be done generically.
There was a problem hiding this comment.
FWIW, it seems like Mesa doesn't implement VADriverVTableWayland.
There was a problem hiding this comment.
More details: it seems only intel-vaapi-driver implements vaGetSurfaceBufferWl, intel-media-driver doesn't. So, think it's reasonable to loose this functionality when wl_drm is not available. The fix for intel-vaapi-driver is intel/intel-vaapi-driver#566.
|
Gentle ping |
|
Gentle ping |
|
Any chance you'd have time to look at this again @XinfengZhang @xhaihao? |
sorry for slow response, TBH, I am not familiar with this part, so, I plan to release 2.21.0, then pay some time to check the details. then merge it and keep it in master branch , and it will be a part of next release. suppose I could finish at early of next week. |
|
Thanks for the reply. It's not super urgent, but the next wlroots release planned in May will not have |
|
Hi again, please have another look at this PR when you get this chance. |
| uint32_t size | ||
| ) | ||
| { | ||
| close(fd); |
There was a problem hiding this comment.
close the fd here? not record the modifier and format?
There was a problem hiding this comment.
We don't need the modifiers/formats in libva, we just need the DRM device.
| if (!(dev->available_nodes & (1 << DRM_NODE_RENDER))) | ||
| goto end; | ||
|
|
||
| dev_path = dev->nodes[DRM_NODE_RENDER]; |
There was a problem hiding this comment.
always render node? will not touch drm master node
There was a problem hiding this comment.
Yeah, DRM master nodes are for KMS only nowadays.
| wl_registry_bind(registry, name, &zwp_linux_dmabuf_v1_interface, 4); | ||
| feedback = zwp_linux_dmabuf_v1_get_default_feedback(linux_dmabuf); | ||
| zwp_linux_dmabuf_feedback_v1_add_listener(feedback, &feedback_listener, data); | ||
| zwp_linux_dmabuf_v1_destroy(linux_dmabuf); |
There was a problem hiding this comment.
destroy here? or in handle_global_remove
There was a problem hiding this comment.
handle_global_remove is irrelevant here, the linux-dmabuf global is never removed by the compositor.
We destroy the zwp_linux_dmabuf_v1 as soon as we don't need it anymore (ie, as soon as we've created a feedback object).
wl_drm is a legacy protocol, and wlroots is getting rid of it [1]. Use the newer and standard linux-dmabuf protocol if available to get the DRM device. [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4397 Signed-off-by: Simon Ser <contact@emersion.fr>
XinfengZhang
force-pushed
the
linux-dmabuf
branch
from
f670038
to
d5bef6b
Compare
|
Thank you! |
wl_drm is a legacy protocol, and wlroots is getting rid of it 1. Use the newer and standard linux-dmabuf protocol if available to get the DRM device.