Initial Atomic KMS platform #3525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RAOF
force-pushed
the
initial-atomic-kms-platform
branch
from
34f45ee to
8bf1aca
Compare
RAOF
force-pushed
the
initial-atomic-kms-platform
branch
from
8bf1aca to
280f6ed
Compare
RAOF
changed the base branch from
main
to
MIRENG-653/platform-specific-options
RAOF
force-pushed
the
MIRENG-653/platform-specific-options
branch
from
415a9c3 to
9367dc7
Compare
RAOF
force-pushed
the
initial-atomic-kms-platform
branch
2 times, most recently
from
a20a6cf to
ae1a6ac
Compare
RAOF
force-pushed
the
MIRENG-653/platform-specific-options
branch
from
5297983 to
7fd62d0
Compare
RAOF
force-pushed
the
initial-atomic-kms-platform
branch
2 times, most recently
from
d172bfa to
6e64d62
Compare
|
Hmm with dual outputs I get: And an occasional SIGABRT after. Yes. I. Have. Disk. Space. |
|
@tarek-y-ismail's ploughed through drm debug logs and found: This suggests there's a problem with cloning, as it tries to map the smaller buffer onto a higher resolution display. |
By "dual outputs", do you mean builtin-monitor + external monitor, or two external monitors? |
Built-in + external.
As discussed, to get output through your Nvidia GPU, you'll need |
|
Just tried it, both monitors work fine. I assume this is because the second output is not using atomic-kms and thus not triggering the bug |
Yup, that could well be it - they're not sharing the platform like in my case. |
|
Trying heterogeneous refresh rates I got: |
This is initially a copy of the display half of `gbm-kms`, quickly ported to use only the atomic KMS APIs. It shall be further developed to usefully use the atomic APIs to fix various TODOs, and provide support for extra performance features
`DisplayConfigurationOutput.pixel_formats` now contains the list of accepted pixel formats (or, at least, those pixel formats that are representable in the `MirPixelFormats` enum; many aren't). --------- Co-authored-by: tarek-y-ismail <[email protected]> Co-authored-by: Alan Griffiths <[email protected]>
tarek-y-ismail
force-pushed
the
initial-atomic-kms-platform
branch
from
24b446d to
0dc388b
Compare
|
Oh! It's not heterogeneous refresh rates, it's heterogeneous resolutions! |
The dance required to actually disable an output (and free its resources) is surprisingly involved.
We might not actually have a `current_crtc` when `set_power_mode` is called (notably, when disabling an output we set `mir_power_mode_off`). Rather than crashing, log an error if we try and turn on an unconfigured output. Silently ignore trying to turn *off* an unconfigured output, as it's already off.
The way changing the display mode works is that the `configure` sets the desired `mode_index`, and then the *next* `set_crtc` is meant to actually set the mode. This means that we can wait for correctly-sized content to present at the new mode, rather than showing a single black frame before the correct content. But it also means that `set_crtc` needs to look at the requested mode, rather than the current mode!
AlanGriffiths
requested changes
Oct 23, 2024
There was a problem hiding this comment.
This appears to work with one or two monitors (I'm using Miriway/mir-pr3525)
However, when I plug or unplug an external monitor I get:
<- ERROR - > atomic-kms: Failed to schedule page flip: Invalid arguement (22)
Fatal signal received
...
[update 0]
Built Mir locally, with the "cleanup" code disabled, it didn't crash as consistently, but did after a couple of cycles.
The core wasn't as useful as I hoped: it died in an "out of order" shutdown (which is secondary to the problem causing the shutdown, but also worthy of investigation).
[update 1]
Got ssh working (with Saviq's kit), and reproduced under the debugger...
But the "page flip" message wasn't hit. Just the "out of order" shotdown
Mir fatal error: XWaylandConnector was not stopped before being destroyed (is_started: yes, spawner: exists, server: null, wm: exists, wm_event_thread: null)
But still not clear why we're shutting down.
[update 2]
There's an exception thrown...
(gdb) bt
#0 0x00007475508bb35a in __cxxabiv1::__cxa_throw (obj=0x5717b6dcfc70, tinfo=0x74754f10d508 <typeinfo for boost::wrapexcept<std::system_error>>, dest=0x74754f0383ae <boost::wrapexcept<std::system_error>::~wrapexcept()>)
at ../../../../src/libstdc++-v3/libsupc++/eh_throw.cc:81
#1 0x000074754f0384e2 in boost::throw_exception<std::system_error> (e=..., loc=...) at /usr/include/boost/throw_exception.hpp:171
#2 0x000074754f02f798 in (anonymous namespace)::PropertyBlobData::PropertyBlobData (this=0x7fff9b0ef078, drm_fd=..., handle=105) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp:76
#3 0x000074754f032882 in mir::graphics::atomic::AtomicKMSOutput::update_from_hardware_state (this=0x5717b6682df0, output=...) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp:761
#4 0x000074754f0738a5 in operator()<std::shared_ptr<mir::graphics::atomic::KMSOutput> > (__closure=0x7fff9b0ef4c0, output=std::shared_ptr<mir::graphics::atomic::KMSOutput> (use count 4, weak count 0) = {...})
at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/real_kms_display_configuration.cpp:183
#5 0x000074754f07442d in std::__invoke_impl<void, mir::graphics::atomic::RealKMSDisplayConfiguration::update()::<lambda(const auto:42&)>&, const std::shared_ptr<mir::graphics::atomic::KMSOutput>&>(std::__invoke_other, struct {...} &) (__f=...)
at /usr/include/c++/14/bits/invoke.h:61
#6 0x000074754f074296 in std::__invoke_r<void, mir::graphics::atomic::RealKMSDisplayConfiguration::update()::<lambda(const auto:42&)>&, const std::shared_ptr<mir::graphics::atomic::KMSOutput>&>(struct {...} &) (__fn=...)
at /usr/include/c++/14/bits/invoke.h:111
#7 0x000074754f073f83 in std::_Function_handler<void(const std::shared_ptr<mir::graphics::atomic::KMSOutput>&), mir::graphics::atomic::RealKMSDisplayConfiguration::update()::<lambda(const auto:42&)> >::_M_invoke(const std::_Any_data &, const std::shared_ptr<mir::graphics::atomic::KMSOutput> &) (__functor=..., __args#0=std::shared_ptr<mir::graphics::atomic::KMSOutput> (use count 4, weak count 0) = {...}) at /usr/include/c++/14/bits/std_function.h:290
#8 0x000074754f07faaf in std::function<void(std::shared_ptr<mir::graphics::atomic::KMSOutput> const&)>::operator() (this=0x7fff9b0ef4c0, __args#0=std::shared_ptr<mir::graphics::atomic::KMSOutput> (use count 4, weak count 0) = {...})
at /usr/include/c++/14/bits/std_function.h:591
#9 0x000074754f07efb0 in mir::graphics::atomic::RealKMSOutputContainer::for_each_output (this=0x5717b679c7a0, functor=...) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/real_kms_output_container.cpp:36
#10 0x000074754f073a01 in mir::graphics::atomic::RealKMSDisplayConfiguration::update (this=0x5717b665c0f8) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/real_kms_display_configuration.cpp:161
#11 0x000074754f05e767 in mir::graphics::atomic::Display::configuration (this=0x5717b665c050) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/display.cpp:193
#12 0x000074754fcf5a81 in mir::graphics::MultiplexingDisplay::configuration (this=0x5717b66f3a60) at /home/alan/CLionProjects/mir/src/server/graphics/multiplexing_display.cpp:248
#13 0x000074754fab63fc in mir::DisplayServer::Private::configure_display (this=0x5717b65bd880) at /home/alan/CLionProjects/mir/src/server/display_server.cpp:191
#14 0x000074754fab5347 in mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}::operator()() const (__closure=0x7474f817b670) at /home/alan/CLionProjects/mir/src/server/display_server.cpp:91
#15 0x000074754fab952c in std::__invoke_impl<void, mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}&>(std::__invoke_other, mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}&) (__f=...)
at /usr/include/c++/14/bits/invoke.h:61
#16 0x000074754fab8bbb in std::__invoke_r<void, mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}&>(mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}&) (__fn=...)
at /usr/include/c++/14/bits/invoke.h:111
#17 0x000074754fab7996 in std::_Function_handler<void (), mir::DisplayServer::Private::Private(mir::ServerConfiguration&)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (__functor=...) at /usr/include/c++/14/bits/std_function.h:290
#18 0x000074754f063bce in std::function<void()>::operator() (this=0x7474f817b670) at /usr/include/c++/14/bits/std_function.h:591
#19 0x000074754f05ea16 in operator() (__closure=0x7474f817b670, type=mir::udev::Monitor::CHANGED, device=...) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/display.cpp:230
#20 0x000074754f0612e6 in std::__invoke_impl<void, mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)>::<lambda(mir::udev::Monitor::EventType, const mir::udev::Device&)>&, mir::udev::Monitor::EventType, const mir::udev::Device&>(std::__invoke_other, struct {...} &) (__f=...) at /usr/include/c++/14/bits/invoke.h:61
#21 0x000074754f060b59 in std::__invoke_r<void, mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)>::<lambda(mir::udev::Monitor::EventType, const mir::udev::Device&)>&, mir::udev::Monitor::EventType, const mir::udev::Device&>(struct {...} &) (__fn=...) at /usr/include/c++/14/bits/invoke.h:111
#22 0x000074754f060483 in std::_Function_handler<void(mir::udev::Monitor::EventType, const mir::udev::Device&), mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)>::<lambda(mir::udev::Monitor::EventType, const mir::udev::Device&)> >::_M_invoke(const std::_Any_data &, mir::udev::Monitor::EventType &&, const mir::udev::Device &)
(__functor=..., __args#0=@0x7fff9b0ef7f4: mir::udev::Monitor::CHANGED, __args#1=...) at /usr/include/c++/14/bits/std_function.h:290
#23 0x00007475503aa2ec in std::function<void(mir::udev::Monitor::EventType, mir::udev::Device const&)>::operator() (this=0x7fff9b0ef890, __args#0=mir::udev::Monitor::CHANGED, __args#1=...) at /usr/include/c++/14/bits/std_function.h:591
#24 0x00007475503a95bd in mir::udev::Monitor::process_events (this=0x5717b665c0c0, handler=...) at /home/alan/CLionProjects/mir/src/platform/udev/udev_wrapper.cpp:355
#25 0x000074754f05eac2 in operator() (__closure=0x5717b6c179a0) at /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/display.cpp:225
#26 0x000074754f061a80 in std::__invoke_impl<void, mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)>&, int>(std::__invoke_other, struct {...} &) (__f=...) at /usr/include/c++/14/bits/invoke.h:61
#27 0x000074754f061467 in std::__invoke_r<void, mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)>&, int>(struct {...} &)
(__fn=...) at /usr/include/c++/14/bits/invoke.h:111
#28 0x000074754f060c9f in std::_Function_handler<void(int), mir::graphics::atomic::Display::register_configuration_change_handler(mir::graphics::EventHandlerRegister&, const mir::graphics::DisplayConfigurationChangeHandler&)::<lambda(int)> >::_M_invoke(const std::_Any_data &, int &&) (__functor=..., __args#0=@0x7fff9b0ef994: 117) at /usr/include/c++/14/bits/std_function.h:290
#29 0x000074754fad493a in std::function<void(int)>::operator() (this=0x5717b6c55c60, __args#0=117) at /usr/include/c++/14/bits/std_function.h:591
#30 0x000074754faccef7 in operator() (__closure=0x5717b6dfabf0, fd=117) at /home/alan/CLionProjects/mir/src/server/glib_main_loop.cpp:232
#31 0x000074754fad22ee in std::__invoke_impl<void, mir::GLibMainLoop::register_fd_handler(std::initializer_list<int>, void const*, mir::UniqueModulePtr<std::function<void(int)> >)::<lambda(int)>&, int>(std::__invoke_other, struct {...} &) (__f=...)
at /usr/include/c++/14/bits/invoke.h:61
#32 0x000074754fad0e41 in std::__invoke_r<void, mir::GLibMainLoop::register_fd_handler(std::initializer_list<int>, void const*, mir::UniqueModulePtr<std::function<void(int)> >)::<lambda(int)>&, int>(struct {...} &) (__fn=...)
at /usr/include/c++/14/bits/invoke.h:111
#33 0x000074754facf8e9 in std::_Function_handler<void(int), mir::GLibMainLoop::register_fd_handler(std::initializer_list<int>, void const*, mir::UniqueModulePtr<std::function<void(int)> >)::<lambda(int)> >::_M_invoke(const std::_Any_data &, int &&)
(__functor=..., __args#0=@0x7fff9b0efa94: 117) at /usr/include/c++/14/bits/std_function.h:290
#34 0x000074754fad493a in std::function<void(int)>::operator() (this=0x5717b6e14a80, __args#0=117) at /usr/include/c++/14/bits/std_function.h:591
#35 0x000074754fadf106 in mir::detail::FdSources::FdContext::static_call (fd=117, ctx=0x5717b6e14a80) at /home/alan/CLionProjects/mir/src/server/glib_main_loop_sources.cpp:390
#36 0x0000747550c27397 in g_main_dispatch (context=0x5717b65ad090) at ../../../glib/gmain.c:3357
#37 0x0000747550c87dc7 in g_main_context_dispatch_unlocked (context=0x5717b65ad090) at ../../../glib/gmain.c:4208
#38 g_main_context_iterate_unlocked.isra.0 (context=context@entry=0x5717b65ad090, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../../../glib/gmain.c:4273
#39 0x0000747550c268b3 in g_main_context_iteration (context=0x5717b65ad090, may_block=1) at ../../../glib/gmain.c:4338
#40 0x000074754facc407 in mir::GLibMainLoop::run (this=0x5717b65aa7b0) at /home/alan/CLionProjects/mir/src/server/glib_main_loop.cpp:142
#41 0x000074754fab49ac in mir::DisplayServer::run (this=0x7fff9b0efd20) at /home/alan/CLionProjects/mir/src/server/display_server.cpp:241
#42 0x000074754faaafdc in mir::run_mir (config=..., init=..., terminator_=...) at /home/alan/CLionProjects/mir/src/server/run_mir.cpp:281
#43 0x000074754fae8cbc in mir::Server::run (this=0x5717b6516770) at /home/alan/CLionProjects/mir/src/server/server.cpp:410
#44 0x0000747550f183c3 in miral::MirRunner::Self::run_with (this=0x5717b6514d30, options=std::initializer_list of length 14 = {...}) at /home/alan/CLionProjects/mir/src/miral/runner.cpp:183
#45 0x0000747550f18c0c in miral::MirRunner::run_with (this=0x7fff9b0f0010, options=std::initializer_list of length 14 = {...}) at /home/alan/CLionProjects/mir/src/miral/runner.cpp:254
#46 0x00005717ad34c251 in main (argc=2, argv=0x7fff9b0f0658) at /home/alan/CLionProjects/mir/examples/miral-shell/shell_main.cpp:136
[update 3]
This patch avoids the crash, just need to understand the context to decide how sane it is...
diff --git a/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp b/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp
index 275aa7d320..e35eecf025 100644
--- a/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp
+++ b/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp
@@ -758,16 +758,23 @@ void mga::AtomicKMSOutput::update_from_hardware_state(
GammaCurves gamma;
if (current_crtc && crtc_props->has_property("GAMMA_LUT") && crtc_props->has_property("GAMMA_LUT_SIZE"))
{
- PropertyBlobData gamma_lut{drm_fd_, static_cast<uint32_t>((*crtc_props)["GAMMA_LUT"])};
- auto const gamma_size = gamma_lut.data<struct drm_color_lut>().size();
- gamma.red.reserve(gamma_size);
- gamma.green.reserve(gamma_size);
- gamma.blue.reserve(gamma_size);
- for (auto const& entry : gamma_lut.data<struct drm_color_lut>())
+ try
+ {
+ PropertyBlobData gamma_lut{drm_fd_, static_cast<uint32_t>((*crtc_props)["GAMMA_LUT"])};
+ auto const gamma_size = gamma_lut.data<struct drm_color_lut>().size();
+ gamma.red.reserve(gamma_size);
+ gamma.green.reserve(gamma_size);
+ gamma.blue.reserve(gamma_size);
+ for (auto const& entry : gamma_lut.data<struct drm_color_lut>())
+ {
+ gamma.red.push_back(entry.red);
+ gamma.green.push_back(entry.green);
+ gamma.blue.push_back(entry.blue);
+ }
+ }
+ catch (...)
{
- gamma.red.push_back(entry.red);
- gamma.green.push_back(entry.green);
- gamma.blue.push_back(entry.blue);
+ log(logging::Severity::warning, MIR_LOG_COMPONENT, std::current_exception(), "Failed to set gamma curves");
}
}The error is:
[2024-10-24 09:38:50.657726] < -warning- > atomic-kms: Failed to set gamma curves: /home/alan/CLionProjects/mir/src/platforms/atomic-kms/server/kms/atomic_kms_output.cpp(76): Throw in function {anonymous}::PropertyBlobData::PropertyBlobData(const mir::Fd&, uint32_t)
Dynamic exception type: boost::wrapexcept<std::system_error>
std::exception::what: Failed to read DRM property blob: No such file or directory
…pdate_from_hardware_state()`
AlanGriffiths
approved these changes
Oct 24, 2024
Looks legit to me, @RAOF feel free to merge if you agree with where the exception is caught. |
*Almost* all of this is only accessed on a single thread, the compositor's submission thread. *Almost*. Unfortunately, `configure` is generally called from the ServerAction loop, which is a different thread to the composition thread. `ensure_crtc` can also (transitively) be called from `configuration`, which also happens off-composition-thread. Wrap this ball of wax up in a `mir::Synchronised<>`, to ensure we're not racy. (This also affects `gbm-kms`, but to a much lesser extent)
Just to loop back to this; I can trivially reproduce this on |
|
Here are a couple of extra fixes - one trivial, and one thread race use-after-free. Finally, I've identified a problem when disconnecting and re-connecting an external display to a different port on my laptop - we're not disassociating the CRTC from the old connector, so we run into resource conflicts. The fix there will be to update |
|
Bah, that doesn't fix it for some reason; I still see a CRTC connected to my disconnected output. |
…state` When we refresh the hardware state of each `KMSOutput` check whether it has resources currently assigned but is now not connected. If so, free the resources before continuing.
| @@ -262,6 +262,8 @@ void mga::Display::clear_connected_unused_outputs() | |||
| { | |||
| current_display_configuration.for_each_output([&](DisplayConfigurationOutput const& conf_output) | |||
| { | |||
| auto kms_output = current_display_configuration.get_output_for(conf_output.id); | |||
There was a problem hiding this comment.
Suggested change
| auto kms_output = current_display_configuration.get_output_for(conf_output.id); | |
| auto const kms_output = current_display_configuration.get_output_for(conf_output.id); |
AlanGriffiths
approved these changes
Nov 6, 2024
OTOH, I've tried using this a while and found that docking [update 0] This crash happens regardless of whether the I suspect it is unrelated to this PR, and is related to the point at which it branched from main [update 1] Raised as #3663 - not sure why I only hit it testing this branch, looks like it has been there a while. [update 2] I guess the reason |
Otherwise we *can* be in a position where hardware resources are bound to a disconnected output, preventing them being used if a display is plugged in to a different connector.
RAOF
force-pushed
the
initial-atomic-kms-platform
branch
from
130bb1b to
5efbaef
Compare
|
Now properly supports unplugging a monitor and plugging it in to a different port! |
Comment on lines
+372
to
+385
| try | ||
| { | ||
| ensure_crtc(*conf); | ||
| } | ||
| catch (...) | ||
| { | ||
| /* | ||
| * In order to actually clear the output, we need to have a crtc | ||
| * connected to the output/connector so that we can disconnect | ||
| * it. However, not being able to get a crtc is OK, since it means | ||
| * that the output cannot be displaying anything anyway. | ||
| */ | ||
| return; | ||
| } |
There was a problem hiding this comment.
A try block around a single line of code followed by a catch block that silently eats everything suggests we are using the wrong reporting mechanism.
This isn't my experience. I have to explicitly choose |
As that's been confirmed by @Saviq, I'll fix the text |
AlanGriffiths
approved these changes
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a minimal-changes copy of the display half of the
gbm-kmsplatform, switched over to all Atomic KMS API.It's parallel installable with the existing
gbm-kmsplatform, which will still be preferentially used on devices with atomic support.Currently, over the existing
gbm-kmsdisplay platform it only has improved gamma support.Immediate missing features: