Use DMA-BUF with HW encoding #206
Conversation
|
Hi, and thank you for working on this! I have briefly looked through the code and it looks ok. However, if we go for the dmabuf approach, wouldn't it make sense to use the wlr-export-dmabuf protocol to avoid copying the pixel data altogether? |
Even with wlr-export-dmabuf, the client is still supposed to copy the frame? And copying the frame in wf-recorder would mean doing it in OpenGL. <enum name="flags">
<description summary="frame flags">
Special flags that should be respected by the client.
</description>
<entry name="transient" value="0x1"
summary="clients should copy frame before processing"/>
</enum> |
Yeah, I see, I thought the flag isn't always set but looking at the wlroots code, it always is. I'll read the code in a bit more depth soon and will merge if everything is ok. |
|
Doesn't work, tried both vulkan and opengl backends on wlroots Also I think there should be an option to not use dmabuf, similar to |
|
It should be but it seems to fail even before starting encoding for you. |
|
Tried that, still the same error. fwiw i'm on sway/wlroots both built from latest git |
|
Does this help? diff --git a/src/main.cpp b/src/main.cpp
index 26d73fd..34cc43d 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -305,7 +305,7 @@ static void frame_handle_linux_dmabuf(void *, struct zwlr_screencopy_frame_v1 *f
if (!buffer.wl_buffer) {
buffer.bo = gbm_bo_create(gbm_device, buffer.width,
- buffer.height, format, GBM_BO_USE_RENDERING);
+ buffer.height, format, GBM_BO_USE_LINEAR | GBM_BO_USE_RENDERING);
if (buffer.bo == NULL)
{
std::cerr << "Failed to create gbm bo" << std::endl; |
|
This works now but only with scale_vaapi=format=nv12, which results in yuvj420p videos that have incorrect colors. Changing the filter to |
|
Yes, software filters won't work now because there's no transfer to CPU anymore. |
|
6600xt + mesa 22.3.3 screenshot with grim: screencopy with dmabuf: |
|
Actually this should work: |
Sounds quite suboptimal to download the data just to scale it and upload again .. But unfortunately I have no idea what filters or options need to be set for this to work. |
|
It doesn't work, I still get the same incorrect colors |
Indeed, but if someones wants that .... (but the filter format is wrong and it doesn't work)
In any case, this is not related to this pull request. Maybe changing some of these options can fix the colors for you? |
Yes, this shouldn't block the PR but there should be a |
nowrep
force-pushed
the
dmabuf
branch
2 times, most recently
from
5a471fd
to
97a2f4e
Compare
|
It will now map the dmabuf to memory if there is a Also I've managed to reproduce the wrong colors, if I use the sw |
|
Incorrect colors are fixed by #208 |
nowrep
force-pushed
the
dmabuf
branch
2 times, most recently
from
ae8864a
to
3f16a47
Compare
|
wf-recorder segfaults when either width or height is an odd number with dmabuf, and gets stuck on |
|
Hangs when bad geometry is provided, happens with or without any other options. Used to select whole output before properly |
Can't reproduce. Setting geometry to invalid value behaves exactly the same as if you don't set any geometry at all. |
|
Ah, sorry that has nothing to do with geometry, I misunderstood. It fails and gets stuck on when using dmabuf, but the provided rendering device isn't the one that wlroots-based compositor started with on multigpu systems. Perhaps a message that lets the user know when its using dmabuf or screencopy would be nice. Also, you can't Ctrl+C out of wf-recorder when it's stuck on "Failed to copy frame, retrying...", you have to send SIGKILL. It never reaches MAX_FRAME_FAILURES when using dmabuf |
That's because it will never try again to capture the frame - not related to this PR. diff --git a/src/main.cpp b/src/main.cpp
index 49ee1bc..9761da4 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -253,6 +253,9 @@ static void frame_handle_failed(void *, struct zwlr_screencopy_frame_v1 *) {
{
std::cerr << "Failed to copy frame too many times, exiting!" << std::endl;
exit_main_loop = true;
+ } else
+ {
+ buffer_copy_done = true;
}
}
|
|
that works, but there's no need for it to be an else statement is there? looks good to me now |
There was a problem hiding this comment.
Thanks once more for working on this! I finally got the time to test, I can reproduce the washed out colors, but that is a problem from before, so it can be fixed separately. All else seems to be working fine, I am no dmabuf expert, but the implementation looks reasonable.
I have one small comment regarding logging, but that should be easy to fix :)
Are there any issues with this PR as of now? I am not sure how the problems on @llyyr 's system are to be resolved.
| @@ -857,6 +1026,57 @@ int main(int argc, char *argv[]) | |||
| wl_registry_add_listener(registry, ®istry_listener, NULL); | |||
| sync_wayland(); | |||
|
|
|||
| if (params.codec.find("vaapi") != std::string::npos) | |||
There was a problem hiding this comment.
There are many cases here, some of which may not be apparent to the user. I think it would be nice to print messages not just in the error cases, but basically every time we make a decision: therefore the user can see what is going on internally (e.g. when we use dmabuf, whether we use the same device as compositor, etc).
This is mesa bug, but it has been already fixed.
There are no issues I am aware of. It does use way more memory than is needed, but that's because it uses 16 buffers and the same issue is with software encoding too. |
|
Fixed one mistake + added info messages about enabling/disabling dmabuf capture. |
|
The CI also needs an update ^ |
I just described my issue 1/3rd of an screen above your comment. With the current git master it's not possible for me to use hardware encoding without making my system freeze to the point where even the reset button doesn't work and I have to turn off power and turn it back on. I have no interest in trying to debug AMD's garbage but there should still be an option to not always use dmabuf when using hardware encoding |
I did read your message but for some reason assumed the problem was somehow resolved ... Yeah, if drivers are that broken, we should have a workaround. I thought however some of the earlier versions of this PR worked for you? Do you happen to know which change triggers the crash? |
The same version of this PR that worked before doesn't work now, so I can only assume it's a bug somewhere in wlroots/mesa(radeonsi)/amdgpu. However having to do a full reset for every single bisect is not something I have time for, so someone else will have to figure out what's broken and where and why. |
I wouldn't say that drivers are that broken, I can say that I had no GPU reset while developing this 😃 Without kernel and compositor logs it's really hard to say what's wrong, but if you get full system freeze then it sounds like kernel issue. Usually kernel can recover from GPU hang, although you'll have to restart your session because compositors mostly don't handle GPU resets well. |
|
@llyyr Does this work? |
|
That still doesn't work but at least it doesn't produce a gpu hang.
Citation needed. AMD recovers from gpu resets maybe 5-10% of the time, their own devs tell you that it's the userspace application's fault if it causes AMD gpus to hang and reset unsuccessfully, not that nothing in the userspace should be able to do that in the first place. |
|
Thanks for the log, it's the screencopy capture that fails. It shouldn't send it to encoder if the copy failed, so that has to be fixed.
If this is your experience then there is definitely something very wrong. |
sway and wlroots built from git, with the vulkan backend but it also happens on the gles backend. edit: it appears to work on the gles backend if I specify |
|
Thanks, now I can see what's wrong. Also word of advice, when you are using very non-standard setup (vulkan, 10bit, rc kernel, ...) it's usually a good idea to mention that ;) |
Doesn't work on gles either. Not on 10bit. I am on 6.5-rc3 but it doesn't work on older kernels either. |
|
This should fix the INVALID modifier being sent and the compositor should import it correctly. diff --git a/src/main.cpp b/src/main.cpp
index 093070d..92e3c9e 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -317,8 +317,9 @@ static void frame_handle_linux_dmabuf(void *, struct zwlr_screencopy_frame_v1 *f
buffer.height = height;
if (!buffer.wl_buffer) {
- buffer.bo = gbm_bo_create(gbm_device, buffer.width,
- buffer.height, format, GBM_BO_USE_LINEAR | GBM_BO_USE_RENDERING);
+ const uint64_t modifier = 0; // DRM_FORMAT_MOD_LINEAR
+ buffer.bo = gbm_bo_create_with_modifiers(gbm_device, buffer.width,
+ buffer.height, format, &modifier, 1);
if (buffer.bo == NULL)
{
std::cerr << "Failed to create gbm bo" << std::endl;
|
|
That fixes it on vulkan when inserting hwupload to the filters, however I still get the system freeze without it on both gles and vulkan, on both 8 bit and 10 bit, on both 6.3.9 and 6.5-rc3, the only remaining variable here is me using mesa-git I'll downgrade to an older version later |
|
Thanks for testing. So now the copy works but trying to import the dmabuf into vaapi doesn't. Unfortunately it's going to be more difficult to debug with the system freeze, and I assume no dmesg errors. Also the 10bit formats should be added to format table. It probably still works, but may confuse some filters. |
|
I was able to bisect it down to https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/24151 |
|
|
|
Should I open an issue on Mesa for this? |
|
Yes, please. |
|
Would be helpful if you could attach necessary information that I might've missed here https://gitlab.freedesktop.org/mesa/mesa/-/issues/9497 |
No description provided.